PR gdb/16188: Verify PTRACE_TRACEME succeeded

  This patch fixes PR gdb/16188, which is about the fact that
fork_inferior doesn't verify the return value of the "traceme_fun"
callback.  On most targets, this callback is actually a wrapper to a
ptrace call that does a PTRACE_TRACEME on the forked GDB process that
will eventually become the inferior.

Thanks to Pedro, this second version of the patch is simpler and more
more logical.  Basically, two helper functions are added:
trace_start_error and trace_start_error_with_name.  The former can be
used when there is a customized error message to be printed to the
user.  The latter works like perror_with_name, so you just need to
pass the function that error'd.

Both helper functions mentioned above do basically the same thing:
print the error message to stderr and call _exit, properly terminating
the forked inferior.

Most of the patch takes care of guarding the necessary system calls
against errors on the "traceme_fun" callbacks.  It is not right to
call error on these situations, so I've replaced these calls with the
proper helper function call.

Regression-tested on BuildBot.

Thanks,

gdb/ChangeLog:
2017-02-17  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	PR gdb/16188
	* darwin-nat.c (darwin_ptrace_me): Check if calls to system
	calls succeeded.
	* fork-child.c (trace_start_error): New function.
	(trace_start_error_with_name): Likewise.
	* gnu-nat.c (gnu_ptrace_me): Check if call to PTRACE succeeded.
	* inf-ptrace.c (inf_ptrace_me): Likewise.
	* inferior.h (trace_start_error): New prototype.
	(trace_start_error_with_name): Likewise.
---
 gdb/darwin-nat.c | 20 +++++++++++++-------
 gdb/fork-child.c | 25 +++++++++++++++++++++++++
 gdb/gnu-nat.c    |  2 +-
 gdb/inf-ptrace.c |  3 ++-
 gdb/inferior.h   | 14 ++++++++++++++
 5 files changed, 55 insertions(+), 9 deletions(-)
  

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
  

On Monday, February 20 2017, Pedro Alves wrote:

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

Oh, I'm really sorry, this was actually a mistake on the patch.  I meant
to delete the 'errno = 0;' on darwin_ptrace_me, not on darwin_ptrace.
Of course, I understand that errno must be cleared before the ptrace
call and I had read the exact same paragraph on the manpage.  Anyway,
sorry for wasting your time on this.

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

Yeah.

I fixed the mistake and pushed the patch.  Thanks.

  0db8980cc0ee05727c11f8b7c6674137a4d5de4e