[PATCH] make strace handle SIGTRAP properly

Denys Vlasenko dvlasenk at redhat.com
Tue Jun 21 12:50:50 UTC 2011


On Tue, 2011-06-21 at 01:25 +0400, Dmitry V. Levin wrote:
> On Mon, Jun 20, 2011 at 09:24:15PM +0200, Denys Vlasenko wrote:
> [...]
> >                         int options = ptrace_setoptions_for_all;
> >                         if (followfork)    // && (tcp->parent == NULL))  <=== HERE
> >                                 options |= ptrace_setoptions_followfork;
> >                         if (options) {
> >                                 if (debug)
> >                                         fprintf(stderr, "setting opts %x on pid %d\n", options, tcp->pid);
> >                                 if (ptrace(PTRACE_SETOPTIONS, tcp->pid, NULL, options) < 0) {
> >                                         if (errno != ESRCH) {
> >                                                 /* Should never happen, really */
> >                                                 error_msg_and_die("PTRACE_SETOPTIONS");
> >                                         }
> >                                 }
> >                         }
> > 
> > The check (tcp->parent == NULL) in old code was meant to check
> > "if we are not a child created by auto-attach" - in this case,
> > options need to be set on the child; otherwise they are inherited
> > and do not need to be set.
> > 
> > I misunderstood the check and if tcp->parent is not NULL, I was
> > setting only ptrace_setoptions_for_all bits!
> > 
> > Simple solution is depicted above - just comment out "&& (tcp->parent == NULL))".
> > 
> > I will test the "more optimal" solution which does the check, but sets correct opts.
> > 
> > Meanwhile, can you confirm that commenting-out works for you too?
> 
> Yes, removing this "tcp->parent == NULL" check fixes the issue, thanks.

I committed the fix. Please take a look:
-- 
vda


commit f44cce48bbbd573cc5ae801f69f857433160b03a
Author: Denys Vlasenko <dvlasenk at redhat.com>
Date:   Tue Jun 21 14:34:10 2011 +0200

    Fix regression introduced by "Properly handle real SIGTRAPs" change
    
    Commit 3454e4b463e6c22c7ea8c5461ef5a077f4650a54
    introduced a bug: sometimes, TRACECLONE/TRACE[V]FORK opts were not set.
    The check (tcp->parent == NULL) in old code was meant to check
    "if we are not a child created by auto-attach" - in this case,
    options need to be set on the child; otherwise they are inherited
    and do not need to be set.
    I misunderstood the check and if tcp->parent is not NULL, I was
    setting only ptrace_setoptions_for_all bits.
    This change fixes the problem. Since the fixed logic makes it
    unnecessary to keep two sets of options in separate variables,
    I merge them back into one variable, ptrace_setoptions.
    
    * defs.h: Merge ptrace_setoptions_followfork and ptrace_setoptions_for_all
      into one variable, ptrace_setoptions.
    * strace.c: Likewise.
      (test_ptrace_setoptions_followfork): Use ptrace_setoptions variable.
      (test_ptrace_setoptions_for_all): Likewise.
      (main): Likewise.
    * process.c (internal_fork): Likewise.
      (internal_exec): Likewise.
    * strace.c (trace): Fix the bug where different options were set
      depending on "tcp->parent == NULL" condition. Add a comment
      which makes it more clear why this condition is checked.
    
    Signed-off-by: Denys Vlasenko <dvlasenk at redhat.com>

diff --git a/defs.h b/defs.h
index b2ca923..75b002d 100644
--- a/defs.h
+++ b/defs.h
@@ -508,8 +508,7 @@ typedef enum {
 extern struct tcb **tcbtab;
 extern int *qual_flags;
 extern int debug, followfork;
-extern unsigned int ptrace_setoptions_followfork;
-extern unsigned int ptrace_setoptions_for_all;
+extern unsigned int ptrace_setoptions;
 extern int dtime, xflag, qflag;
 extern cflag_t cflag;
 extern int acolumn;
diff --git a/process.c b/process.c
index fc5fa90..0584f54 100644
--- a/process.c
+++ b/process.c
@@ -904,7 +904,7 @@ Process %u resumed (parent %d ready)\n",
 int
 internal_fork(struct tcb *tcp)
 {
-	if ((ptrace_setoptions_followfork
+	if ((ptrace_setoptions
 	    & (PTRACE_O_TRACECLONE | PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK))
 	   == (PTRACE_O_TRACECLONE | PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK))
 		return 0;
@@ -1751,7 +1751,7 @@ internal_exec(struct tcb *tcp)
 		tcp->flags &= ~TCB_WAITEXECVE;
 	else {
 		/* Maybe we have post-execve SIGTRAP suppressed? */
-		if (!(ptrace_setoptions_for_all & PTRACE_O_TRACEEXEC))
+		if (!(ptrace_setoptions & PTRACE_O_TRACEEXEC))
 			tcp->flags |= TCB_WAITEXECVE; /* no */
 	}
 #endif /* LINUX && TCB_WAITEXECVE */
diff --git a/strace.c b/strace.c
index 3298795..93e6d29 100644
--- a/strace.c
+++ b/strace.c
@@ -84,8 +84,7 @@ extern char *optarg;
 
 
 int debug = 0, followfork = 0;
-unsigned int ptrace_setoptions_followfork = 0;
-unsigned int ptrace_setoptions_for_all = 0;
+unsigned int ptrace_setoptions = 0;
 /* Which WSTOPSIG(status) value marks syscall traps? */
 static unsigned int syscall_trap_sig = SIGTRAP;
 int dtime = 0, xflag = 0, qflag = 0;
@@ -826,7 +825,7 @@ test_ptrace_setoptions_followfork(void)
 		}
 	}
 	if (expected_grandchild && expected_grandchild == found_grandchild)
-		ptrace_setoptions_followfork |= test_options;
+		ptrace_setoptions |= test_options;
 	return 0;
 }
 
@@ -907,10 +906,10 @@ test_ptrace_setoptions_for_all(void)
 
 	if (it_worked) {
 		syscall_trap_sig = (SIGTRAP | 0x80);
-		ptrace_setoptions_for_all = test_options;
+		ptrace_setoptions |= test_options;
 		if (debug)
-			fprintf(stderr, "ptrace_setoptions_for_all = %#x\n",
-				ptrace_setoptions_for_all);
+			fprintf(stderr, "ptrace_setoptions = %#x\n",
+				ptrace_setoptions);
 		return;
 	}
 
@@ -1136,11 +1135,11 @@ main(int argc, char *argv[])
 			fprintf(stderr,
 				"Test for options supported by PTRACE_SETOPTIONS "
 				"failed, giving up using this feature.\n");
-			ptrace_setoptions_followfork = 0;
+			ptrace_setoptions = 0;
 		}
 		if (debug)
-			fprintf(stderr, "ptrace_setoptions_followfork = %#x\n",
-				ptrace_setoptions_followfork);
+			fprintf(stderr, "ptrace_setoptions = %#x\n",
+				ptrace_setoptions);
 	}
 	test_ptrace_setoptions_for_all();
 #endif
@@ -2679,16 +2678,16 @@ Process %d attached (waiting for parent)\n",
 				}
 			}
 #ifdef LINUX
-			int options = ptrace_setoptions_for_all;
-			if (followfork && (tcp->parent == NULL))
-				options |= ptrace_setoptions_followfork;
-			if (options) {
-				if (debug)
-					fprintf(stderr, "setting opts %x on pid %d\n", options, tcp->pid);
-				if (ptrace(PTRACE_SETOPTIONS, tcp->pid, NULL, options) < 0) {
-					if (errno != ESRCH) {
-						/* Should never happen, really */
-						perror_msg_and_die("PTRACE_SETOPTIONS");
+			/* If options were not set for this tracee yet */
+			if (tcp->parent == NULL) {
+				if (ptrace_setoptions) {
+					if (debug)
+						fprintf(stderr, "setting opts %x on pid %d\n", ptrace_setoptions, tcp->pid);
+					if (ptrace(PTRACE_SETOPTIONS, tcp->pid, NULL, ptrace_setoptions) < 0) {
+						if (errno != ESRCH) {
+							/* Should never happen, really */
+							perror_msg_and_die("PTRACE_SETOPTIONS");
+						}
 					}
 				}
 			}





More information about the Strace-devel mailing list