LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Re: [PATCH] select: fix sys_select to not leak ERESTARTNOHAND to userspace
@ 2007-08-17 15:41 John Blackwood
  2007-08-17 20:55 ` Neil Horman
  0 siblings, 1 reply; 10+ messages in thread
From: John Blackwood @ 2007-08-17 15:41 UTC (permalink / raw)
  To: Neil Horman
  Cc: linux-rt-users, linux-kernel, Sven-Thorsten Dietrich,
	Gregory Haskins, Tom Horsley

Hi Neil,

We've been having problems with this select patch change.

Specifically -- previously, when a ptrace attach was done to a task
blocked in a select() call and that task had a timeout value,
the task would restart the select() call with an updated timeout value.

With this patch in place, the task now instead returns EINTR.

A test that shows this issue is provided below.

We also confirmed that attaching to a program sitting
in select() with gdb makes the select get an EINTR, so this
behavior also shows up in gdb.

Thank you for your considerations in this matter.

------------------- -------------------
#include <stdio.h>
#include <sys/time.h>
#include <sys/types.h>
#include <unistd.h>
#include <errno.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#include <time.h>
#include <signal.h>
#include <sys/wait.h>
#include <sys/ptrace.h>

int
main(int argc, char ** argv) {
    pid_t kid;
    if ((kid = fork()) == 0) {
       int ms_wait = 2000;
       int rval;
       struct timeval timeout;

       timeout.tv_sec = ms_wait / 1000;
       timeout.tv_usec = (ms_wait % 1000) * 1000;
       rval = select(0, NULL, NULL, NULL, &timeout);
       if (rval == -1) {
          int errcode = errno;
          printf("Hey! Why did my select error off with errno %d (%s)?\n",
                 errcode, strerror(errcode));
          fflush(stdout);
       } else {
          printf("select call completed, return value: %d\n", rval);
       }
       exit(0);
    } else if (kid == (pid_t)-1) {
       perror("fork");
    } else {
       int ms_wait = 500;
       int rval;
       struct timeval timeout;

       /* Wait a bit to make sure kid has a chance to get into its
        * select call
        */
       timeout.tv_sec = ms_wait / 1000;
       timeout.tv_usec = (ms_wait % 1000) * 1000;
       rval = select(0, NULL, NULL, NULL, &timeout);

       /* Now attach to the kid, then continue him.
        */
       if (ptrace(PTRACE_ATTACH, kid, (void *)0, (void *)0) != 0) {
          perror("ptrace");
       }
       if (waitpid(kid, &rval, 0) != kid) {
          perror("waitpid");
       }
       if (ptrace(PTRACE_CONT, kid, (void *)0, (void *)0) != 0) {
          perror("ptrace");
       }
       if (waitpid(kid, &rval, 0) != kid) {
          perror("waitpid");
       }
    }
    return 0;
}

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH] select: fix sys_select to not leak ERESTARTNOHAND to userspace
@ 2007-01-16 20:13 Neil Horman
  2007-01-22 13:59 ` Paolo Ornati
  2007-01-24  5:59 ` David Miller
  0 siblings, 2 replies; 10+ messages in thread
From: Neil Horman @ 2007-01-16 20:13 UTC (permalink / raw)
  To: linux-kernel; +Cc: akpm, torvalds, nhorman

As it is currently written, sys_select checks its return code to convert
ERESTARTNOHAND to EINTR.  However, the check is within an if (tvp) clause, and
so if select is called from userspace with a NULL timeval, then it is possible
for the ERESTARTNOHAND errno to leak into userspace, which is incorrect.  This
patch moves that check outside of the conditional, and prevents the errno leak.

Thanks & Regards
Neil

Signed-Off-By: Neil Horman <nhorman@tuxdriver.com>


 select.c |   18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)


diff --git a/fs/select.c b/fs/select.c
index fe0893a..afcd691 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -415,20 +415,12 @@ asmlinkage long sys_select(int n, fd_set __user *inp, fd_set __user *outp,
 		rtv.tv_sec = timeout;
 		if (timeval_compare(&rtv, &tv) >= 0)
 			rtv = tv;
-		if (copy_to_user(tvp, &rtv, sizeof(rtv))) {
-sticky:
-			/*
-			 * If an application puts its timeval in read-only
-			 * memory, we don't want the Linux-specific update to
-			 * the timeval to cause a fault after the select has
-			 * completed successfully. However, because we're not
-			 * updating the timeval, we can't restart the system
-			 * call.
-			 */
-			if (ret == -ERESTARTNOHAND)
-				ret = -EINTR;
-		}
+		if (copy_to_user(tvp, &rtv, sizeof(rtv)))
+			return -EFAULT;
 	}
+sticky:
+	if (ret == -ERESTARTNOHAND)
+		ret = -EINTR;
 
 	return ret;
 }

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2007-08-17 20:56 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-08-17 15:41 [PATCH] select: fix sys_select to not leak ERESTARTNOHAND to userspace John Blackwood
2007-08-17 20:55 ` Neil Horman
  -- strict thread matches above, loose matches on Subject: below --
2007-01-16 20:13 Neil Horman
2007-01-22 13:59 ` Paolo Ornati
2007-01-22 14:52   ` Neil Horman
2007-01-22 16:03     ` Linus Torvalds
2007-01-22 16:24       ` Neil Horman
2007-01-23  0:00         ` bert hubert
2007-01-24  5:59 ` David Miller
2007-01-24 13:21   ` Neil Horman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).