[PATCH] make strace handle SIGTRAP properly

[Dmitry V. Levin]

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");

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

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

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

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

> @@ -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?

> @@ -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?


-- 
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/20110526/9ef29b61/attachment.bin>