Fix bug 72650. Only restart syscalls on signal if the client asked for it.

git-svn-id: svn://svn.valgrind.org/valgrind/trunk@2197

coregrind/vg_include.h

@@ -1607,7 +1607,7 @@ extern void VG_(proxy_setsigmask)(ThreadId tid);
 extern void VG_(proxy_sigack)   ( ThreadId tid, const vki_ksigset_t *);
 extern void VG_(proxy_abort_syscall) ( ThreadId tid );
 extern void VG_(proxy_waitsig)  ( void );
-extern void VG_(proxy_wait_sys)	(ThreadId tid);
+extern void VG_(proxy_wait_sys)	(ThreadId tid, Bool restart);
 
 extern void VG_(proxy_shutdown) ( void );	/* shut down the syscall workers */
 extern Int  VG_(proxy_resfd)    ( void );	/* FD something can select on to know 
@@ -1630,7 +1630,7 @@ extern void VG_(proxy_handlesig)( const vki_ksiginfo_t *siginfo,
 extern Char *VG_(resolve_filename)(Int fd);
 
 extern Bool VG_(pre_syscall) ( ThreadId tid );
-extern void VG_(post_syscall)( ThreadId tid );
+extern void VG_(post_syscall)( ThreadId tid, Bool restart );
 extern void VG_(restart_syscall) ( ThreadId tid );
 
 extern Bool VG_(is_kerror) ( Int res );

coregrind/vg_proxylwp.c

@@ -180,7 +180,7 @@ struct ProxyLWP {
    jmp_buf		jumpbuf;
 };
 
-static void sys_wait_results(Bool block, ThreadId tid, enum RequestType reqtype);
+static void sys_wait_results(Bool block, ThreadId tid, enum RequestType reqtype, Bool restart);
 
 struct PX_Request {
    enum RequestType	request;
@@ -841,9 +841,9 @@ void VG_(proxy_sendsig)(ThreadId tid, Int sig)
 	 it by the normal mechanism.  In this case, just wait for the
 	 syscall to dinish. */
       if (tst->status == VgTs_WaitSys && tst->syscallno == __NR_rt_sigtimedwait)
-	 sys_wait_results(True, tid, PX_RunSyscall);
+	 sys_wait_results(True, tid, PX_RunSyscall, True);
       else
-	 sys_wait_results(True, tid, PX_Signal);
+	 sys_wait_results(True, tid, PX_Signal, True);
    }
 }
 
@@ -866,7 +866,7 @@ void VG_(proxy_abort_syscall)(ThreadId tid)
    if (lwp != 0)
       VG_(ktkill)(lwp, VKI_SIGVGINT);
 
-   sys_wait_results(True, tid, PX_RunSyscall);
+   sys_wait_results(True, tid, PX_RunSyscall, False);
 
    vg_assert(tst->status == VgTs_Runnable);
 }
@@ -1063,7 +1063,7 @@ void VG_(proxy_delete)(ThreadId tid, Bool force)
       proxy_wait, because if we don't read the results pipe, the proxy
       may be blocked writing to it, causing a deadlock with us as we
       wait for it to exit. */
-   sys_wait_results(True, tid, PX_Exiting);
+   sys_wait_results(True, tid, PX_Exiting, True);
    res = proxy_wait(proxy, True, &status);
 
    if ((!res || status != 0) && VG_(clo_verbosity) > 1)
@@ -1091,7 +1091,7 @@ void VG_(proxy_delete)(ThreadId tid, Bool force)
    reply, otherwise it will block forever).  If tid != 0, then it will
    wait for a reply for that particular tid.
  */
-static void sys_wait_results(Bool block, ThreadId tid, enum RequestType reqtype)
+static void sys_wait_results(Bool block, ThreadId tid, enum RequestType reqtype, Bool restart)
 {
    Bool found_reply = (reqtype == PX_BAD);
    struct PX_Reply res;
@@ -1153,7 +1153,7 @@ static void sys_wait_results(Bool block, ThreadId tid, enum RequestType reqtype)
 	    vg_assert(res.u.syscallno == tst->syscallno);
 	    vg_assert(tst->status == VgTs_WaitSys);
 
-	    VG_(post_syscall)(res.tid);
+	    VG_(post_syscall)(res.tid, restart);
 	    break;
 
 	 case PX_Signal:
@@ -1187,16 +1187,16 @@ static void sys_wait_results(Bool block, ThreadId tid, enum RequestType reqtype)
 /* External version */
 void VG_(proxy_results)(void)
 {
-   sys_wait_results(False, 0, PX_BAD);
+   sys_wait_results(False, 0, PX_BAD, True);
 }
 
-void VG_(proxy_wait_sys)(ThreadId tid)
+void VG_(proxy_wait_sys)(ThreadId tid, Bool restart)
 {
    ThreadState *tst = VG_(get_ThreadState)(tid);
 
    vg_assert(tst->status == VgTs_WaitSys);
 
-   sys_wait_results(True, tid, PX_RunSyscall);
+   sys_wait_results(True, tid, PX_RunSyscall, restart);
 }
 
 /* Tell proxy about it's thread's updated signal mask */
@@ -1226,7 +1226,7 @@ void VG_(proxy_setsigmask)(ThreadId tid)
       with respect to each other (ie, if thread A then thread B
       updates their signal masks, A's update must be done before B's
       is).  */
-   sys_wait_results(True, tid, PX_SetSigmask);
+   sys_wait_results(True, tid, PX_SetSigmask, True);
 }
 
 void VG_(proxy_sigack)(ThreadId tid, const vki_ksigset_t *mask)
@@ -1271,7 +1271,7 @@ void VG_(proxy_waitsig)(void)
    if (VG_(do_signal_routing))
       VG_(route_signals)();
    else
-      sys_wait_results(True, VG_INVALID_THREADID /* any */, PX_Signal);
+      sys_wait_results(True, VG_INVALID_THREADID /* any */, PX_Signal, True);
 }
 
 /* Issue a syscall to the thread's ProxyLWP */
@@ -1364,7 +1364,7 @@ void VG_(proxy_sanity)(void)
 			 tid, px->lwp, ret);
 	    sane = False;
 	 }
-	 sys_wait_results(True, tid, PX_Ping);
+	 sys_wait_results(True, tid, PX_Ping, True);
 	 /* Can't make an assertion here, fortunately; this will
 	    either come back or it won't. */
       }

coregrind/vg_scheduler.c

@@ -699,7 +699,7 @@ void sched_do_syscall ( ThreadId tid )
 
    /* If pre_syscall returns true, then we're done immediately */
    if (VG_(pre_syscall)(tid)) {
-      VG_(post_syscall(tid));
+      VG_(post_syscall(tid, True));
       vg_assert(VG_(threads)[tid].status == VgTs_Runnable);
    } else {
       vg_assert(VG_(threads)[tid].status == VgTs_WaitSys);

coregrind/vg_signals.c

@@ -1447,22 +1447,12 @@ void VG_(deliver_signal) ( ThreadId tid, const vki_ksiginfo_t *info, Bool async
 	 came.  Either way, it is going to give us some syscall
 	 results right now, so wait for them to appear.  This makes
 	 the thread runnable again, so we're in the right state to run
-	 the handler, and resume the syscall when we're done. */
-      VG_(proxy_wait_sys)(tid);
-
+	 the handler.  We ask post_syscall to restart based on the
+	 client's sigaction flags. */
       if (0)
-	 VG_(printf)("signal %d interrupting syscall %d\n",
-		     sigNo, tst->syscallno);
-
-      if (tst->m_eax == -VKI_ERESTARTSYS) {
-	  if (handler->scss_flags & VKI_SA_RESTART) {
-	     VG_(restart_syscall)(tid);
-	  } else
-	     tst->m_eax = -VKI_EINTR;
-      } else {
-	 /* return value is already in eax - either EINTR or the
-	    normal return value */
-      }
+	 VG_(printf)("signal %d interrupted syscall %d; restart=%d\n",
+		     sigNo, tst->syscallno, !!(handler->scss_flags & VKI_SA_RESTART));
+      VG_(proxy_wait_sys)(tid, !!(handler->scss_flags & VKI_SA_RESTART));
    }
 
    /* If the client specifies SIG_IGN, treat it as SIG_DFL */

coregrind/vg_syscalls.c

@@ -5242,12 +5242,13 @@ Bool VG_(pre_syscall) ( ThreadId tid )
 }
 
 
-void VG_(post_syscall) ( ThreadId tid )
+void VG_(post_syscall) ( ThreadId tid, Bool restart )
 {
    ThreadState* tst;
    UInt syscallno;
    const struct sys_info *sys;
    Bool special = False;
+   Bool restarted = False;
    void *pre_res;
 
    VGP_PUSHCC(VgpCoreSysWrap);
@@ -5272,27 +5273,45 @@ void VG_(post_syscall) ( ThreadId tid )
       special = True;
    }
 
-   if (!VG_(is_kerror)(tst->m_eax) && sys->after != NULL)
-      (sys->after)(tst->tid, tst);
-
-   /* Do any post-syscall actions */
-   if (VG_(needs).syscall_wrapper) {
-      VGP_PUSHCC(VgpSkinSysWrap);
-      SK_(post_syscall)(tid, syscallno, pre_res, tst->m_eax, /*isBlocking*/True); // did block
-      VGP_POPCC(VgpSkinSysWrap);
-   }
-
    if (tst->m_eax == -VKI_ERESTARTSYS) {
-      /* Applications never expect to see this, so we should actually
-	 restart the syscall (it means the signal happened before the
-	 syscall made any progress, so we can safely restart it and
-	 pretend the signal happened before the syscall even
-	 started)  */
-      VG_(restart_syscall)(tid);
+      /* Applications never expect to see this, so we should either
+	 restart the syscall or fail it with EINTR, depending on what
+	 our caller wants.  Generally they'll want to restart, but if
+	 client set the signal state to not restart, then we fail with
+	 EINTR.  Either way, ERESTARTSYS means the syscall made no
+	 progress, and so can be failed or restarted without
+	 consequence. */
+      if (0)
+	 VG_(printf)("syscall %d returned ERESTARTSYS; restart=%d\n",
+		     syscallno, restart);
+
+      if (restart) {
+	 restarted = True;
+	 VG_(restart_syscall)(tid);
+      } else
+	 tst->m_eax = -VKI_EINTR;
+   } 
+
+   if (!restarted) {
+      if (!VG_(is_kerror)(tst->m_eax) && sys->after != NULL)
+	 (sys->after)(tst->tid, tst);
+
+      /* Do any post-syscall actions
+
+	 NOTE: this is only called if the syscall completed.  If the
+	 syscall was restarted, then it will call the Tool's
+	 pre_syscall again, without calling post_syscall (ie, more
+	 pre's than post's)
+       */
+      if (VG_(needs).syscall_wrapper) {
+	 VGP_PUSHCC(VgpSkinSysWrap);
+	 SK_(post_syscall)(tid, syscallno, pre_res, tst->m_eax, /*isBlocking*/True); // did block
+	 VGP_POPCC(VgpSkinSysWrap);
+      }
    }
 
    tst->status = VgTs_Runnable;	/* runnable again */
-   tst->syscallno = -1;
+   tst->syscallno = -1;		/* no current syscall */
 
    VGP_POPCC(VgpCoreSysWrap);
 }

none/tests/Makefile.am

@@ -38,6 +38,8 @@ EXTRA_DIST = $(noinst_SCRIPTS) \
 	shortpush.stderr.exp shortpush.vgtest \
 	shorts.stderr.exp shorts.vgtest \
 	smc1.stderr.exp smc1.stdout.exp smc1.vgtest \
+	syscall-restart1.vgtest syscall-restart1.stdout.exp syscall-restart1.stderr.exp \
+	syscall-restart2.vgtest syscall-restart2.stdout.exp syscall-restart2.stderr.exp \
 	yield.stdout.exp yield.vgtest
 
 check_PROGRAMS = \
@@ -46,6 +48,7 @@ check_PROGRAMS = \
 	fucomip munmap_exe map_unmap mremap rcl_assert \
 	rcrl readline1 resolv seg_override sha1_test shortpush shorts smc1 \
 	pth_blockedsig \
+	syscall-restart1 syscall-restart2 \
 	coolo_sigaction gxx304 yield
 
 AM_CFLAGS   = $(WERROR) -Winline -Wall -Wshadow -g -I$(top_srcdir)/include
@@ -77,6 +80,8 @@ smc1_SOURCES 		= smc1.c
 sha1_test_SOURCES 	= sha1_test.c
 shortpush_SOURCES 	= shortpush.c
 shorts_SOURCES 		= shorts.c
+syscall_restart1_SOURCES = syscall-restart1.c
+syscall_restart2_SOURCES = syscall-restart2.c
 yield_SOURCES		= yield.c
 yield_LDADD		= -lpthread
 

none/tests/syscall-restart1.c

@@ -0,0 +1,63 @@
+#include <stdio.h>
+#include <unistd.h>
+#include <signal.h>
+#include <errno.h>
+#include <sys/wait.h>
+#include <string.h>
+
+/* Make sure that a blocking syscall returns EINTR if hit by a signal,
+   and there's no SA_RESTART */
+
+static void handler(int s)
+{
+}
+
+int main()
+{
+	int pid;
+	int fds[2];
+
+	if (pipe(fds) == -1) {
+		perror("FAIL: pipe\n");
+		return 1;
+	}
+
+	pid = fork();
+
+	if (pid == -1) {
+		perror("fork failed");
+		return 1;
+	}
+
+	if (pid == 0) {
+		char ch = '?';
+		int ret;
+		struct sigaction sa;
+		
+		sa.sa_handler = handler;
+		sigfillset(&sa.sa_mask);
+		sa.sa_flags = 0; /* no SA_RESTART */
+
+		sigaction(SIGUSR1, &sa, NULL);
+
+		close(fds[1]);
+		ret = read(fds[0], &ch, 1);
+
+		if (ret != -1 || errno != EINTR)
+			fprintf(stderr, "FAIL: expected EINTR, not %d/%s/%c\n", 
+				ret, strerror(errno), ch);
+	} else {
+		signal(SIGPIPE, SIG_IGN);
+
+		close(fds[0]);
+		sleep(1);
+		kill(pid, SIGUSR1);
+		sleep(1);
+		if (write(fds[1], "x", 1) != -1 || errno != EPIPE)
+			fprintf(stderr, "FAIL: expected write to fail with EPIPE\n");
+
+		waitpid(pid, NULL, 0);
+	}
+
+	return 0;
+}

none/tests/syscall-restart1.stderr.exp

@@ -0,0 +1,3 @@
+
+
+

none/tests/syscall-restart1.vgtest

@@ -0,0 +1 @@
+prog: syscall-restart1

none/tests/syscall-restart2.c

@@ -0,0 +1,62 @@
+#include <stdio.h>
+#include <unistd.h>
+#include <signal.h>
+#include <errno.h>
+#include <sys/wait.h>
+#include <string.h>
+
+/* Make sure that a blocking syscall restarts if hit by a signal,
+   and SA_RESTART is set */
+
+static void handler(int s)
+{
+}
+
+int main()
+{
+	int pid;
+	int fds[2];
+
+	if (pipe(fds) == -1) {
+		perror("FAIL: pipe\n");
+		return 1;
+	}
+
+	pid = fork();
+
+	if (pid == -1) {
+		perror("fork failed");
+		return 1;
+	}
+
+	if (pid == 0) {
+		char ch = '?';
+		int ret;
+		struct sigaction sa;
+		
+		sa.sa_handler = handler;
+		sigfillset(&sa.sa_mask);
+		sa.sa_flags = SA_RESTART;
+
+		sigaction(SIGUSR1, &sa, NULL);
+
+		close(fds[1]);
+		ret = read(fds[0], &ch, 1);
+
+		if (ret != 1 || ch != 'x')
+			fprintf(stderr, "FAIL: expected 1 byte, not %d/%s/%c\n", 
+				ret, strerror(errno), ch);
+	} else {
+		signal(SIGPIPE, SIG_IGN);
+
+		close(fds[0]);
+		sleep(1);
+		kill(pid, SIGUSR1);
+		sleep(1);
+		write(fds[1], "x", 1);
+
+		waitpid(pid, NULL, 0);
+	}
+
+	return 0;
+}

none/tests/syscall-restart2.stderr.exp

@@ -0,0 +1,3 @@
+
+
+

none/tests/syscall-restart2.vgtest

@@ -0,0 +1 @@
+prog: syscall-restart2