diff --git a/coregrind/vg_include.h b/coregrind/vg_include.h index 36fc10af7..4bfa7643f 100644 --- a/coregrind/vg_include.h +++ b/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 ); diff --git a/coregrind/vg_proxylwp.c b/coregrind/vg_proxylwp.c index d8f683d10..cd76a1235 100644 --- a/coregrind/vg_proxylwp.c +++ b/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. */ } diff --git a/coregrind/vg_scheduler.c b/coregrind/vg_scheduler.c index 4a0ec686c..3a8d7ce1e 100644 --- a/coregrind/vg_scheduler.c +++ b/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); diff --git a/coregrind/vg_signals.c b/coregrind/vg_signals.c index 204ebeabc..c5605646b 100644 --- a/coregrind/vg_signals.c +++ b/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 */ diff --git a/coregrind/vg_syscalls.c b/coregrind/vg_syscalls.c index 10a174c95..d123eec98 100644 --- a/coregrind/vg_syscalls.c +++ b/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); } diff --git a/none/tests/Makefile.am b/none/tests/Makefile.am index a2a3960b1..e75d284b5 100644 --- a/none/tests/Makefile.am +++ b/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 diff --git a/none/tests/syscall-restart1.c b/none/tests/syscall-restart1.c new file mode 100644 index 000000000..29be67b89 --- /dev/null +++ b/none/tests/syscall-restart1.c @@ -0,0 +1,63 @@ +#include +#include +#include +#include +#include +#include + +/* 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; +} diff --git a/none/tests/syscall-restart1.stderr.exp b/none/tests/syscall-restart1.stderr.exp new file mode 100644 index 000000000..b28b04f64 --- /dev/null +++ b/none/tests/syscall-restart1.stderr.exp @@ -0,0 +1,3 @@ + + + diff --git a/none/tests/syscall-restart1.stdout.exp b/none/tests/syscall-restart1.stdout.exp new file mode 100644 index 000000000..e69de29bb diff --git a/none/tests/syscall-restart1.vgtest b/none/tests/syscall-restart1.vgtest new file mode 100644 index 000000000..469d5d373 --- /dev/null +++ b/none/tests/syscall-restart1.vgtest @@ -0,0 +1 @@ +prog: syscall-restart1 diff --git a/none/tests/syscall-restart2.c b/none/tests/syscall-restart2.c new file mode 100644 index 000000000..11defdf92 --- /dev/null +++ b/none/tests/syscall-restart2.c @@ -0,0 +1,62 @@ +#include +#include +#include +#include +#include +#include + +/* 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; +} diff --git a/none/tests/syscall-restart2.stderr.exp b/none/tests/syscall-restart2.stderr.exp new file mode 100644 index 000000000..b28b04f64 --- /dev/null +++ b/none/tests/syscall-restart2.stderr.exp @@ -0,0 +1,3 @@ + + + diff --git a/none/tests/syscall-restart2.stdout.exp b/none/tests/syscall-restart2.stdout.exp new file mode 100644 index 000000000..e69de29bb diff --git a/none/tests/syscall-restart2.vgtest b/none/tests/syscall-restart2.vgtest new file mode 100644 index 000000000..369ff6aa8 --- /dev/null +++ b/none/tests/syscall-restart2.vgtest @@ -0,0 +1 @@ +prog: syscall-restart2