[LTP] Suggested patch to recvfrom / recvmsg tests

[LTP] Suggested patch to recvfrom / recvmsg tests

[Joseph Beckenbach]

Hi, all!

A tweak to improve recvfrom01.c and recvmsg01.c --
there's a loop in the child server process which walks all available fds and attempts to read those where the relevant fd_set bit is set.
Sadly, when there's more file descriptors available than bits in the fd_set, this loop will happily walk off the end of the fd_set, clearing bits along the way.
This leads to intermittent segfaults and thus false positives in these two tests.

Immediate fix is to force the loop not to look at bits off the end of the fd_set.
I've no idea whether it makes sense to rewrite the server code of these two tests, as only under heavy socket / fd use would the accept() calls in the tests return a file descriptor outside the fd_set.

Thanks!

Joseph
Joseph Beckenbach, senior QA engineer : Automation

=====
diff --git a/testcases/kernel/syscalls/recvfrom/recvfrom01.c b/testcases/kernel/syscalls/recvfrom/recvfrom01.c
index b5ab4a0..5e68d98 100644
--- a/testcases/kernel/syscalls/recvfrom/recvfrom01.c
+++ b/testcases/kernel/syscalls/recvfrom/recvfrom01.c
@@ -319,7 +319,7 @@ void do_child()
 			/* send something back */
 			(void)write(newfd, "hoser\n", 6);
 		}
-		for (fd = 0; fd < nfds; ++fd)
+		for (fd = 0; fd < nfds && fd < FD_SETSIZE; ++fd)
 			if (fd != sfd && FD_ISSET(fd, &rfds)) {
 				cc = read(fd, buf, sizeof(buf));
 				if (cc == 0 || (cc < 0 && errno != EINTR)) {
diff --git a/testcases/kernel/syscalls/recvmsg/recvmsg01.c b/testcases/kernel/syscalls/recvmsg/recvmsg01.c
index dd6b3d5..5492c1d 100644
--- a/testcases/kernel/syscalls/recvmsg/recvmsg01.c
+++ b/testcases/kernel/syscalls/recvmsg/recvmsg01.c
@@ -481,7 +481,7 @@ void do_child()
 			if (newfd >= 0)
 				FD_SET(newfd, &afds);
 		}
-		for (fd = 0; fd < nfds; ++fd)
+		for (fd = 0; fd < nfds && fd < FD_SETSIZE; ++fd)
 			if (fd != sfd && fd != ufd && FD_ISSET(fd, &rfds)) {
 				char rbuf[1024];


------------------------------------------------------------------------------
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] Suggested patch to recvfrom / recvmsg tests

[chrubis]

Hi!
> A tweak to improve recvfrom01.c and recvmsg01.c --
> there's a loop in the child server process which walks all available fds and attempts to read those where the relevant fd_set bit is set.
> Sadly, when there's more file descriptors available than bits in the fd_set, this loop will happily walk off the end of the fd_set, clearing bits along the way.
> This leads to intermittent segfaults and thus false positives in these two tests.

What arch is this? On my system both getfdsetsize() and FD_SETSIZE are
set to 1024?

> =====
> diff --git a/testcases/kernel/syscalls/recvfrom/recvfrom01.c b/testcases/kernel/syscalls/recvfrom/recvfrom01.c
> index b5ab4a0..5e68d98 100644
> --- a/testcases/kernel/syscalls/recvfrom/recvfrom01.c
> +++ b/testcases/kernel/syscalls/recvfrom/recvfrom01.c
> @@ -319,7 +319,7 @@ void do_child()
>  			/* send something back */
>  			(void)write(newfd, "hoser\n", 6);
>  		}
> -		for (fd = 0; fd < nfds; ++fd)
> +		for (fd = 0; fd < nfds && fd < FD_SETSIZE; ++fd)
>  			if (fd != sfd && FD_ISSET(fd, &rfds)) {
>  				cc = read(fd, buf, sizeof(buf));
>  				if (cc == 0 || (cc < 0 && errno != EINTR)) {

What about we set nfds to sfd + 1 and for each child we accept we set
nfds to max(nfds, newfd+1)?

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
"Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE
Instantly run your Selenium tests across 300+ browser/OS combos.  Get 
unparalleled scalability from the best Selenium testing platform available.
Simple to use. Nothing to install. Get started now for free."
http://p.sf.net/sfu/SauceLabs
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] Suggested patch to recvfrom / recvmsg tests

[chrubis]

Hi!
> > What arch is this?
> 
> Irrelevant, I think, but x86_64.

Sorry the question was not specific enough I should have asked for
distribution and architecture. But the paragraph below explains why you
system is different.

> > On my system both getfdsetsize() and FD_SETSIZE are set to 1024?
> 
> The test uses getdtablesize(), which pulls the OPEN_MAX kernel parameter.
> In our kernel, we have OPEN_MAX set to more than FD_SETSIZE.
> 
> I first found this problem when this parameter had a typo, adding an order of magnitude to OPEN_MAX.
> Without the added clause in the loop to restrict it, the test would usually core from zero-bit bombardment into related memory. 
> Correcting the parameter typo unfortunately did not alter the test binary's behavior.
> 
> Arguably, this might be a bug with /usr/include/x86_64-linux-gnu/bits/typesizes.h and /usr/include/linux/posix_types.h
> which hard-code __FD_SETSIZE to 1024 instead of checking the OPEN_MAX kernel paramenter.
> (My knee-jerk reaction is to file this alongside "select() is broken".  :-)

Well, select() is decades old interface and fd_set was designed as fixed
size buffer back then, I would say that it's broken by design, because
if you open more filedescriptors than was __FD_SETSIZE by the time of
the program compilation it will break anyway. Happily we have poll() and
epoll() for programs that need to use more than 1024 file descriptors.

> An alternative patch might be to replace the getdtablesize() call by getfdsetsize().
> This would rely on accept() never returning a value greater than FD_SETSIZE.

That will not happen in this case, see below.

> I see no indication in accept, select, or poll manpages whether this is the case;
> I suspect it is so, otherwise select/poll cannot detect activity on some range of returned fd.

Well filedescriptors are allocated from lowest available number, so
everything works fine unless you keep open more than 1022 file
descriptors (first three are allocated for stdin, stdout and stderr).

> This would require inspecting the accept() code, though, and adding a test on accept() to verify that accept() cannot return a fd greater than FD_SETSIZE.
> Maybe accept() in a near-infinite loop to exhaust all fd's ...?
> (Beyond my available time right now.)
> 
> 
> > What about we set nfds to sfd + 1 and for each child we accept we set nfds to max(nfds, newfd+1)?
> 
> When accept() returns the highest-select()-detectable fd, this will clear the high-bit just off the end of the fdset, would it not?
> It'll hit far less often, but still be out-of-bounds, sadly ....

Looking at the code, the newly opened filedescriptor is closed right
away. I've tried to add printf() to the code and indeed it's always se
to 4, so the maximum would end up as 5.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
Is your legacy SCM system holding you back? Join Perforce May 7 to find out:
• 3 signs your SCM is hindering your productivity
• Requirements for releasing software faster
• Expert tips and advice for migrating your SCM now
http://p.sf.net/sfu/perforce
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list