[PATCH] make strace handle SIGTRAP properly

[Dmitry V. Levin]

Hi,

On Fri, May 20, 2011 at 06:02:14PM +0200, Denys Vlasenko wrote:
>  int debug = 0, followfork = 0;
> -unsigned int ptrace_setoptions = 0;
> +unsigned int ptrace_setoptions_followfork = 0;
> +static unsigned int ptrace_setoptions_for_all = 0;
> +/* Which WSTOPSIG(status) value marks syscall traps? */
> +static unsigned int SYSCALLTRAP = SIGTRAP;

I think giving macros-like names to variables is confusing,
even when these variables are going to be used in place of macros.

> +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 think an error(3)-like interface would be useful in other parts of
strace, too.

> +{
> +	char *msg;
> +	va_list p;
> +
> +	va_start(p, fmt);
> +	msg = NULL;
> +	vasprintf(&msg, fmt, p);
> +	if (msg) {
> +		fprintf(stderr, "%s: %s\n", progname, msg);
> +		free(msg);
> +	}
> +	va_end(p);
> +
> +	cflag = 0; /* or else cleanup may print summary */
> +        cleanup();
> +	fflush(NULL);
> +        _exit(1);

When such function is called in parent process, exit() is more appropriate
than _exit().  Otherwise, cleanup() must not be called.

> +/*
> + * Test whether the kernel support PTRACE_O_TRACESYSGOOD.
> + * First fork a new child, call ptrace(PTRACE_SETOPTIONS) on it,
> + * and then see whether it will stop with (SIGTRAP | 0x80).
> + */
> +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?

> +		kill(pid, SIGSTOP);
> +		_exit(0); /* parent should see entry into this syscall */
> +	}
> +
> +	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.

> +			kill(pid, SIGKILL);
> +			error_msg_and_die("%s: unexpected wait result %d", __func__, tracee_pid);

Well, I'm not quite happy with test_* functions aborting the whole strace
unconditionally.

> +		}
> +		if (WIFEXITED(status))
> +			break;

I'd also check for exit status here.

> +		if (!WIFSTOPPED(status)) {
> +			kill(pid, SIGKILL);
> +			error_msg_and_die("%s: unexpected wait status %x", __func__, status);
> +		}

Same unhappiness about error_msg_and_die.

> +		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.

> +		}
> +		if (WSTOPSIG(status) == (SIGTRAP | 0x80)) {
> +			it_worked = 1;
> +		}
> +		if (ptrace(PTRACE_SYSCALL, pid, 0L, 0L) < 0) {
> +			kill(pid, SIGKILL);
> +			error_msg_and_die("%s: PTRACE_SYSCALL doesn't work");

What about ESRCH?

> +		}
> +	}
> +
> +	if (it_worked) {
> +		SYSCALLTRAP = (SIGTRAP | 0x80);
> +		ptrace_setoptions_for_all = test_options;
> +		if (debug)
> +			fprintf(stderr, "ptrace_setoptions_for_all = %#x\n",
> +				ptrace_setoptions_for_all);
> +		return;
> +	}
> +
> +	fprintf(stderr,
> +		"Test for PTRACE_O_TRACESYSGOOD failed, giving up using this feature.\n");
> +}
>  #endif
>  
>  int
> @@ -977,16 +1075,17 @@ main(int argc, char *argv[])
>  
>  #ifdef LINUX
>  	if (followfork) {
> -		if (test_ptrace_setoptions() < 0) {
> +		if (test_ptrace_setoptions_followfork() < 0) {
>  			fprintf(stderr,
>  				"Test for options supported by PTRACE_SETOPTIONS "
>  				"failed, giving up using this feature.\n");
> -			ptrace_setoptions = 0;
> +			ptrace_setoptions_followfork = 0;
>  		}
>  		if (debug)
> -			fprintf(stderr, "ptrace_setoptions = %#x\n",
> -				ptrace_setoptions);
> +			fprintf(stderr, "ptrace_setoptions_followfork = %#x\n",
> +				ptrace_setoptions_followfork);
>  	}
> +	test_ptrace_setoptions_for_all();
>  #endif
>  
>  	/* Check if they want to redirect the output. */
> @@ -1751,7 +1850,7 @@ int sig;
>  				break;
>  			}
>  			error = ptrace_restart(PTRACE_CONT, tcp,
> -					WSTOPSIG(status) == SIGTRAP ? 0
> +					WSTOPSIG(status) == SYSCALLTRAP ? 0
>  					: WSTOPSIG(status));
>  			if (error < 0)
>  				break;
> @@ -2408,6 +2507,11 @@ handle_ptrace_event(int status, struct t
>  		}
>  		return handle_new_child(tcp, childpid, 0);
>  	}
> +	if (status >> 16 == PTRACE_EVENT_EXEC) {
> +		if (debug)
> +			fprintf(stderr, "PTRACE_EVENT_EXEC on pid %d (ignored)\n", tcp->pid);
> +		return 0;
> +	}
>  	return 1;
>  }
>  #endif
> @@ -2597,7 +2701,7 @@ Process %d attached (waiting for parent)
>  			fprintf(stderr, "pid %u stopped, [%s]\n",
>  				pid, signame(WSTOPSIG(status)));
>  
> -		if (ptrace_setoptions && (status >> 16)) {
> +		if (status >> 16) {

Is this change accurate enough?
How this (status >> 16) should be handled if strace haven't requested it?


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20110525/7b486dc7/attachment.bin>