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

[Pedro Alves <palves@redhat.com>]

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