This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] PR gdb/16188: Verify PTRACE_TRACEME succeeded
- From: Pedro Alves <palves at redhat dot com>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>, GDB Patches <gdb-patches at sourceware dot org>
- Cc: dje at google dot com
- Date: Mon, 20 Feb 2017 12:19:35 +0000
- Subject: Re: [PATCH] PR gdb/16188: Verify PTRACE_TRACEME succeeded
- Authentication-results: sourceware.org; auth=none
- References: <20170216201931.5843-1-sergiodj@redhat.com> <20170218050900.31399-1-sergiodj@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