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
pgp2aI23HlTZD.pgp
Description: PGP signature
------------------------------------------------------------------------------ vRanger cuts backup time in half-while increasing security. With the market-leading solution for virtual backup and recovery, you get blazing-fast, flexible, and affordable data protection. Download your free trial now. http://p.sf.net/sfu/quest-d2dcopy1
_______________________________________________ Strace-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/strace-devel
