Re: [PATCH] PR gdb/16188: Verify PTRACE_TRACEME succeeded

Pedro Alves
2017-02-20T12:19:41Z

Hi Sergio,

This LGTM, save for the errno handling in Darwin bits:

On 02/18/2017 05:09 AM, Sergio Durigan Junior wrote:
> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index 8c5e8a0..e02e51d 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -254,7 +254,6 @@ darwin_ptrace (const char *name,
> {
> int ret;
>
> - errno = 0;
> ret = ptrace (request, pid, arg3, arg4);
> if (ret == -1 && errno == 0)
> ret = 0;

Removing "errno = 0" here is incorrect. There are ptrace calls where a -1
return is not an error, thus that check for "errno==0" after the
ptrace call. Since system calls are not required to clear errno on
success, that errno=0 is required.

This is Darwin, but the Linux man pages, in "man ptrace" say:

On error, all requests return -1, and errno is set appropriately. Since the
value returned by a successful PTRACE_PEEK* request may be -1, the caller
must clear errno before the call, and then check it afterward to determine whether
or not an error occurred.

And actually, the comment just above darwin_ptrace talks
about clearning errno. So it's really incorrect.

> @@ -1728,23 +1727,30 @@ darwin_ptrace_me (void)
> int res;
> char c;
>
> + errno = 0;

OTOH, I don't see the need to clear it here. Below,
errno will only be used when a syscall fails, and in
failure case, the syscall must set errno.

> /* Close write end point. */
> - close (ptrace_fds[1]);
> + if (close (ptrace_fds[1]) < 0)
> + trace_start_error_with_name ("close");
>
> /* Wait until gdb is ready. */
> res = read (ptrace_fds[0], &c, 1);
> if (res != 0)
> - error (_("unable to read from pipe, read returned: %d"), res);
> - close (ptrace_fds[0]);
> + trace_start_error (_("unable to read from pipe, read returned: %d"), res);
> +
> + if (close (ptrace_fds[0]) < 0)
> + trace_start_error_with_name ("close");
>
> /* Get rid of privileges. */
> - setegid (getgid ());
> + if (setegid (getgid ()) < 0)
> + trace_start_error_with_name ("setegid");
>
> /* Set TRACEME. */
> - PTRACE (PT_TRACE_ME, 0, 0, 0);
> + if (PTRACE (PT_TRACE_ME, 0, 0, 0) < 0)
> + trace_start_error_with_name ("PTRACE");
>
> /* Redirect signals to exception port. */
> - PTRACE (PT_SIGEXC, 0, 0, 0);
> + if (PTRACE (PT_SIGEXC, 0, 0, 0) < 0)
> + trace_start_error_with_name ("PTRACE");
> }

Thanks,
Pedro Alves