[PATCH] make strace handle SIGTRAP properly
Denys Vlasenko
dvlasenk at redhat.com
Thu May 26 08:55:14 UTC 2011
On Thu, 2011-05-26 at 02:21 +0400, Dmitry V. Levin wrote:
> On Wed, May 25, 2011 at 01:53:50PM +0200, Denys Vlasenko wrote:
> > On Wed, 2011-05-25 at 02:23 +0400, Dmitry V. Levin wrote:
> > > > +static void error_msg_and_die(const char *fmt, ...) /* TODO: __attribute__ ((noreturn, format (printf, 1, 2))) */ ;
> > > > +static void error_msg_and_die(const char *fmt, ...)
> > >
> > > I really don't like this interface, it would be less flexible than error(3).
> >
> > I used to use [p]error_msg[_and_die](const char *fmt, ...)
> > functions in several other projects. They proved to be no less versatile
> > than error(), but are significantly more readable. Compare:
> >
> > error(1, errno, "Can't write to '%s'", filename);
> >
> > with:
> >
> > perror_msg_and_die("Can't write to '%s'", filename);
>
> It depends whether you are used to error(3) syntax or not.
> I'd be happy with suggested 4 functions with clear names, but
> you added only one of them and made it static. Could you add others
> and make all of them global, please? One could use more informative
> perror_msg_and_die("fork")
> instead of current
> error_msg_and_die("fork failed");
Sure.
> > > > +static void
> > > > +test_ptrace_setoptions_for_all(void)
> > > > +{
> > > > + const unsigned int test_options = PTRACE_O_TRACESYSGOOD | PTRACE_O_TRACEEXEC;
> > > > + int pid;
> > > > + int it_worked = 0;
> > > > +
> > > > + pid = fork();
> > > > + if (pid < 0)
> > > > + error_msg_and_die("fork failed");
> > > > +
> > > > + if (pid == 0) {
> > > > + pid = getpid();
> > > > + if (ptrace(PTRACE_TRACEME, 0L, 0L, 0L) < 0)
> > > > + /* "parent, something is deeply wrong!" */
> > > > + kill(pid, SIGKILL);
> > >
> > > The approach used in test_ptrace_setoptions() was just to _exit(1) in this
> > > case. Why this test does this part differently?
> >
> > _exit(1) here will exit *in the child*. Parent would see WIFEXITED and
> > decide that it's just merely a test failire, and will continue (without
> > using options it tested for).
>
> I'd like to keep test_ptrace_* code in sync where appropriate. Would it
> be OK to use test_ptrace_setoptions() approach, i.e. to _exit(1) instead
> of SIGKILL in the child and check WEXITSTATUS in the parent?
Yes, no problems
> > > > + while (1) {
> > > > + int status, tracee_pid;
> > > > +
> > > > + errno = 0;
> > > > + tracee_pid = wait(&status);
> > > > + if (tracee_pid <= 0) {
> > > > + if (errno == EINTR)
> > > > + continue;
> > >
> > > In test_ptrace_setoptions() there is a check for errno == ECHILD.
> > > I suppose it won't harm in this case, too.
> >
> > This check should do... what? Something like this?
> >
> > if (errno == ECHILD)
> > error_msg_and_die("Your kernel is terminally broken: it can't even "
> > "properly wait for a single child. Bailing out");
>
> I see, the way how it's coded ECHILD cannot happen.
>
> > If yes, then the below code does that already:
> >
> > > > + kill(pid, SIGKILL);
> > > > + error_msg_and_die("%s: unexpected wait result %d", __func__, tracee_pid);
>
> It should rather be perror_msg_and_die() then: errno, if not clobbered by
> kill(), may contain some useful diagnostics.
Yes.
> > > > + }
> > > > + if (WIFEXITED(status))
> > > > + break;
> > >
> > > I'd also check for exit status here.
> >
> > We need to exit loop regardless of exit code: there is no child anymore,
> > we have no one to wait for.
> >
> > However, just for paranoid reasons we can check that WEXITSTATUS == 0.
> > What do you propose to do if it isn't zero?
>
> I suggest to call _exit(1) instead of SIGKILLing in the child process
> in case of fatal error, and handle WEXITSTATUS != 0 as a fatal error.
Ok.
> > > > + if (WSTOPSIG(status) == SIGSTOP) {
> > > > + /*
> > > > + * We don't check "options aren't accepted" error.
> > > > + * If it happens, we'll never get (SIGTRAP | 0x80),
> > > > + * and thus will decide to not use the option.
> > > > + * IOW: the outcome of the test will be the same.
> > > > + */
> > > > + ptrace(PTRACE_SETOPTIONS, pid, 0L, test_options);
> > >
> > > I'm not sure that this lengthy comment is more obvious than the check
> > > itself.
> >
> > Please clarify: do you propose to perform the check and emit error msg
> > if this ptrace() call failed?
> > (without exiting - here it's a survivable error).
>
> If the error is not due to old kernel that doesn't handle these options,
> I'd like to see diagnostics. It probably means that an error message
> should be printed in case of errno != EINVAL.
Ok.
> > @@ -616,7 +622,8 @@ startup_child(char **argv)
> > if ((pid != 0 && daemonized_tracer) /* parent: to become a traced process */
> > || (pid == 0 && !daemonized_tracer) /* child: to become a traced process */
> > ) {
> > - pid = getpid();
> > + if (pid == 0)
> > + pid = getpid();
>
> This is a change to daemonized_tracer case, that affects only [FREEBSD],
> not related to other changes in this commit?
Sorry, this hunk was not supposed to be in this patch.
> > @@ -710,6 +717,8 @@ startup_child(char **argv)
> > }
> >
> > /* We are the tracer. */
> > + strace_tracer_pid = (pid != 0 ? pid : getpid());
> > +
> > tcp = alloctcb(daemonized_tracer ? getppid() : pid);
> > if (daemonized_tracer) {
> > /* We want subsequent startup_attach() to attach to it. */
>
> Could you explain why strace_tracer_pid should change in startup_child(),
> please?
We need to change strace_tracer_pid because with strace -D, not the
parent pid is the tracer.
(What I wanted to do above is just "strace_tracer_pid = getpid()"
but tried to avoid extra getpid(). I think I should not obfuscate code
for such a tiny win, getpid() is very cheap...)
Actually, with -D tracer is a *grand*child, I forgot that.
Need to assign to strace_tracer_pid once more after second fork...
Please take a look at the patch below. It should have all your
suggestions incorporated.
--
vda
diff -d -urpN strace.0/defs.h strace.1/defs.h
--- strace.0/defs.h 2011-05-24 20:26:27.000000000 +0200
+++ strace.1/defs.h 2011-05-26 09:58:19.801756676 +0200
@@ -339,6 +339,10 @@ extern int mp_ioctl (int f, int c, void
# endif
#endif /* LINUX */
+#if !defined __GNUC__
+# define __attribute__(x) /*nothing*/
+#endif
+
/* Trace Control Block */
struct tcb {
short flags; /* See below for TCB_ values */
@@ -515,6 +519,11 @@ extern struct tcb *tcp_last;
enum bitness_t { BITNESS_CURRENT = 0, BITNESS_32 };
+void error_msg(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
+void perror_msg(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
+void error_msg_and_die(const char *fmt, ...) __attribute__ ((noreturn, format(printf, 1, 2)));
+void perror_msg_and_die(const char *fmt, ...) __attribute__ ((noreturn, format(printf, 1, 2)));
+
extern int set_personality(int personality);
extern const char *xlookup(const struct xlat *, int);
extern struct tcb *alloc_tcb(int, int);
@@ -630,11 +639,7 @@ extern int proc_open(struct tcb *tcp, in
#define printtv_special(tcp, addr) \
printtv_bitness((tcp), (addr), BITNESS_CURRENT, 1)
-extern void tprintf(const char *fmt, ...)
-#ifdef __GNUC__
- __attribute__ ((format (printf, 1, 2)))
-#endif
- ;
+extern void tprintf(const char *fmt, ...) __attribute__ ((format (printf, 1, 2)));
#ifndef HAVE_STRERROR
const char *strerror(int);
diff -d -urpN strace.0/strace.c strace.1/strace.c
--- strace.0/strace.c 2011-05-25 15:32:54.000000000 +0200
+++ strace.1/strace.c 2011-05-26 10:52:54.941995241 +0200
@@ -87,7 +87,7 @@ int debug = 0, followfork = 0;
unsigned int ptrace_setoptions_followfork = 0;
unsigned int ptrace_setoptions_for_all = 0;
/* Which WSTOPSIG(status) value marks syscall traps? */
-static unsigned int SYSCALLTRAP = SIGTRAP;
+static unsigned int syscall_trap_sig = SIGTRAP;
int dtime = 0, xflag = 0, qflag = 0;
cflag_t cflag = CFLAG_NONE;
static int iflag = 0, interactive = 0, pflag_seen = 0, rflag = 0, tflag = 0;
@@ -116,6 +116,7 @@ int tracing_paths = 0;
static int exit_code = 0;
static int strace_child = 0;
+static int strace_tracer_pid = 0;
static char *username = NULL;
uid_t run_uid;
@@ -215,29 +216,62 @@ usage: strace [-CdDffhiqrtttTvVxxy] [-a
exit(exitval);
}
-static void error_msg_and_die(const char *fmt, ...)
-#if defined __GNUC__
- __attribute__ ((noreturn, format(printf, 1, 2)))
-#endif
-;
-static void error_msg_and_die(const char *fmt, ...)
+static void verror_msg(int err_and_exitflag, const char *fmt, va_list p)
{
char *msg;
- va_list p;
- va_start(p, fmt);
msg = NULL;
vasprintf(&msg, fmt, p);
if (msg) {
- fprintf(stderr, "%s: %s\n", progname, msg);
+ int err = err_and_exitflag & INT_MAX;
+ fflush(NULL);
+ if (err)
+ fprintf(stderr, "%s: %s: %s\n", progname, msg, strerror(err));
+ else
+ fprintf(stderr, "%s: %s\n", progname, msg);
free(msg);
}
+
+ if (err_and_exitflag & ((unsigned)INT_MAX+1)) {
+ /* Careful: don't do cleanup not from tracer process */
+ if (strace_tracer_pid == getpid()) {
+ /* TODO? cflag = 0; -- or else cleanup() may print summary */
+ cleanup();
+ }
+ exit(1);
+ }
+}
+
+void error_msg(const char *fmt, ...)
+{
+ va_list p;
+ va_start(p, fmt);
+ verror_msg(0, fmt, p);
va_end(p);
+}
- /* TODO? cflag = 0; -- or else cleanup() may print summary */
- cleanup();
- fflush(NULL);
- _exit(1);
+void error_msg_and_die(const char *fmt, ...)
+{
+ va_list p;
+ va_start(p, fmt);
+ verror_msg(((unsigned)INT_MAX+1), fmt, p);
+ exit(1); /* verror_msg won't return. shut up compiler's warning */
+}
+
+void perror_msg(const char *fmt, ...)
+{
+ va_list p;
+ va_start(p, fmt);
+ verror_msg(errno, fmt, p);
+ va_end(p);
+}
+
+void perror_msg_and_die(const char *fmt, ...)
+{
+ va_list p;
+ va_start(p, fmt);
+ verror_msg(((unsigned)INT_MAX+1) | errno, fmt, p);
+ exit(1); /* verror_msg won't return. shut up compiler's warning */
}
#ifdef SVR4
@@ -429,14 +463,17 @@ startup_attach(void)
}
if (pid) { /* parent */
/*
- * Wait for child to attach to straced process
- * (our parent). Child SIGKILLs us after it attached.
- * Parent's wait() is unblocked by our death,
+ * Wait for grandchild to attach to straced process
+ * (grandparent). Grandchild SIGKILLs us after it attached.
+ * Grandparent's wait() is unblocked by our death,
* it proceeds to exec the straced program.
*/
pause();
_exit(0); /* paranoia */
}
+ /* grandchild */
+ /* We will be the tracer process. Remember our new pid: */
+ strace_tracer_pid = getpid();
}
for (tcbi = 0; tcbi < tcbtabsize; tcbi++) {
@@ -613,8 +650,8 @@ startup_child (char **argv)
cleanup();
exit(1);
}
- if ((pid != 0 && daemonized_tracer) /* parent: to become a traced process */
- || (pid == 0 && !daemonized_tracer) /* child: to become a traced process */
+ if ((pid != 0 && daemonized_tracer) /* -D: parent to become a traced process */
+ || (pid == 0 && !daemonized_tracer) /* not -D: child to become a traced process */
) {
pid = getpid();
#ifdef USE_PROCFS
@@ -710,6 +747,9 @@ startup_child (char **argv)
}
/* We are the tracer. */
+ /* With -D, we are *child* here, IOW: different pid. Fetch it. */
+ strace_tracer_pid = getpid();
+
tcp = alloctcb(daemonized_tracer ? getppid() : pid);
if (daemonized_tracer) {
/* We want subsequent startup_attach() to attach to it. */
@@ -817,13 +857,13 @@ test_ptrace_setoptions_for_all(void)
pid = fork();
if (pid < 0)
- error_msg_and_die("fork failed");
+ perror_msg_and_die("fork");
if (pid == 0) {
pid = getpid();
if (ptrace(PTRACE_TRACEME, 0L, 0L, 0L) < 0)
- /* "parent, something is deeply wrong!" */
- kill(pid, SIGKILL);
+ /* Note: exits with exitcode 1 */
+ perror_msg_and_die("%s: PTRACE_TRACEME doesn't work", __func__);
kill(pid, SIGSTOP);
_exit(0); /* parent should see entry into this syscall */
}
@@ -837,10 +877,14 @@ test_ptrace_setoptions_for_all(void)
if (errno == EINTR)
continue;
kill(pid, SIGKILL);
- error_msg_and_die("%s: unexpected wait result %d", __func__, tracee_pid);
+ perror_msg_and_die("%s: unexpected wait result %d", __func__, tracee_pid);
+ }
+ if (WIFEXITED(status)) {
+ if (WEXITSTATUS(status) == 0)
+ break;
+ /* PTRACE_TRACEME failed in child. This is fatal. */
+ exit(1);
}
- if (WIFEXITED(status))
- break;
if (!WIFSTOPPED(status)) {
kill(pid, SIGKILL);
error_msg_and_die("%s: unexpected wait status %x", __func__, status);
@@ -852,19 +896,21 @@ test_ptrace_setoptions_for_all(void)
* and thus will decide to not use the option.
* IOW: the outcome of the test will be correct.
*/
- ptrace(PTRACE_SETOPTIONS, pid, 0L, test_options);
+ if (ptrace(PTRACE_SETOPTIONS, pid, 0L, test_options) < 0)
+ if (errno != EINVAL)
+ perror_msg("PTRACE_SETOPTIONS");
}
if (WSTOPSIG(status) == (SIGTRAP | 0x80)) {
it_worked = 1;
}
if (ptrace(PTRACE_SYSCALL, pid, 0L, 0L) < 0) {
kill(pid, SIGKILL);
- error_msg_and_die("PTRACE_SYSCALL doesn't work");
+ perror_msg_and_die("PTRACE_SYSCALL doesn't work");
}
}
if (it_worked) {
- SYSCALLTRAP = (SIGTRAP | 0x80);
+ syscall_trap_sig = (SIGTRAP | 0x80);
ptrace_setoptions_for_all = test_options;
if (debug)
fprintf(stderr, "ptrace_setoptions_for_all = %#x\n",
@@ -889,6 +935,8 @@ main(int argc, char *argv[])
progname = argv[0] ? argv[0] : "strace";
+ strace_tracer_pid = getpid();
+
/* Allocate the initial tcbtab. */
tcbtabsize = argc; /* Surely enough for all -p args. */
if ((tcbtab = calloc(tcbtabsize, sizeof tcbtab[0])) == NULL) {
@@ -1000,7 +1048,7 @@ main(int argc, char *argv[])
progname, optarg);
break;
}
- if (pid == getpid()) {
+ if (pid == strace_tracer_pid) {
fprintf(stderr, "%s: I'm sorry, I can't let you do that, Dave.\n", progname);
break;
}
@@ -1863,7 +1911,7 @@ int sig;
break;
}
error = ptrace_restart(PTRACE_CONT, tcp,
- WSTOPSIG(status) == SYSCALLTRAP ? 0
+ WSTOPSIG(status) == syscall_trap_sig ? 0
: WSTOPSIG(status));
if (error < 0)
break;
@@ -2525,6 +2573,8 @@ handle_ptrace_event(int status, struct t
fprintf(stderr, "PTRACE_EVENT_EXEC on pid %d (ignored)\n", tcp->pid);
return 0;
}
+ /* Some PTRACE_EVENT_foo we didn't ask for?! */
+ fprintf(stderr, "Unexpected status %x on pid %d\n", status, tcp->pid);
return 1;
}
#endif
@@ -2756,7 +2806,7 @@ Process %d attached (waiting for parent)
if (ptrace(PTRACE_SETOPTIONS, tcp->pid, NULL, options) < 0) {
if (errno != ESRCH) {
/* Should never happen, really */
- error_msg_and_die("PTRACE_SETOPTIONS");
+ perror_msg_and_die("PTRACE_SETOPTIONS");
}
}
}
@@ -2764,7 +2814,7 @@ Process %d attached (waiting for parent)
goto tracing;
}
- if (WSTOPSIG(status) != SYSCALLTRAP) {
+ if (WSTOPSIG(status) != syscall_trap_sig) {
if (WSTOPSIG(status) == SIGSTOP &&
(tcp->flags & TCB_SIGTRAPPED)) {
/*
More information about the Strace-devel
mailing list