Re: [PATCH v2 0/5] Improve ptrace-error detection

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Thursday, March 19 2020, Kevin Buettner wrote:

> Hi Sergio,

Hey Kevin,

Thanks for the review.

> I'm still reviewing this patch set, but noticed the following during testing:
>
> < PASS: gdb.base/attach-twice.exp: attach
> ---
>> XFAIL: gdb.base/attach-twice.exp: attach
>
> and:
>
> < PASS: gdb.base/attach.exp: do_attach_failure_tests: fail to attach again
> ---
>> FAIL: gdb.base/attach.exp: do_attach_failure_tests: fail to attach again
>
> For gdb.base/attach-twice.exp, the relevant sections of the log files look
> like this:
>
> (gdb) spawn /mesquite2/sourceware-git/f31-ptrace-error-detection/bld/gdb/testsuite/outputs/gdb.base/attach-twice/attach-twice
> attach 1113400
> Attaching to program: /mesquite2/sourceware-git/f31-ptrace-error-detection/bld/gdb/testsuite/outputs/gdb.base/attach-twice/attach-twice, process 1113400
> warning: process 1113400 is already traced by process 1113405
> ptrace: Operation not permitted.
> (gdb) PASS: gdb.base/attach-twice.exp: attach
>
> Versus:
>
> (gdb) spawn /mesquite2/sourceware-git/f31-ptrace-error-detection/bld/gdb/testsuite/outputs/gdb.base/attach-twice/attach-twice
> attach 1113182
> Attaching to program: /mesquite2/sourceware-git/f31-ptrace-error-detection/bld/gdb/testsuite/outputs/gdb.base/attach-twice/attach-twice, process 1113182
> warning: ptrace: Operation not permitted
> process 1113182 is already traced by process 1113187

> There might be restrictions preventing ptrace from working.  Please see
> the appendix "Linux kernel ptrace restrictions" in the GDB documentation
> for more details.
> If you are debugging the inferior remotely, the ptrace restriction(s) must
> be disabled in the target system (e.g., where GDBserver is running).
> (gdb) XFAIL: gdb.base/attach-twice.exp: attach

Ah, good catch.  I can reproduce these here (obviously), which makes me
wonder why I missed them.  I think I may have looked at them and thought
they were racy.

> It seems to me that this should still PASS; I think the regex for that
> test simply needs to be updated.  (You could also add another case.)

Yeah.

> I've also looked at the logs for gdb.base/attach.exp.  The output is
> similar to that shown above.  Again, I think that the regex needs to
> be updated.

You know, I thought it was just going to be a matter of expanding the
regexp in order to match the extra text, but then I started thinking if
there was a better way to do this.  I mean, in these two specific cases
(attach.exp and attach-twice.exp) we *know* what is wrong: we're trying
to attach twice to the same process.  So GDB knows this is the problem,
and when we print the whole "There might be restrictions preventing
ptrace from working..." text, it can confuse the user.

Looking at nat/linux-ptrace.c:linux_ptrace_attach_fail_reason, I see
that my patch is currently doing:

  std::string
  linux_ptrace_attach_fail_reason (pid_t pid, int err)
  {
    std::string result = linux_ptrace_attach_fail_reason_1 (pid);
    std::string ptrace_restrict = linux_ptrace_restricted_fail_reason (err);

   if (!ptrace_restrict.empty ())
     result += "\n" + ptrace_restrict;

IOW, it's always appending the result of
"linux_ptrace_restricted_fail_reason" to the string that will be
printed.  Upon a closer inspection of
"linux_ptrace_attach_fail_reason_1"'s comment, we see:

  /* Find all possible reasons we could fail to attach PID and return
     these as a string.  An empty string is returned if we didn't find
     any reason.  Helper for linux_ptrace_attach_fail_reason and
     linux_ptrace_attach_fail_reason_lwp.  */

  static std::string
  linux_ptrace_attach_fail_reason_1 (pid_t pid)

IOW, if the string returned by it is not empty, it means that the
function was able to determine the reason for the ptrace failure.

After seeing this, I decided that the best approach is to call
"linux_ptrace_restricted_fail_reason" only if
"linux_ptrace_attach_fail_reason_1" returns an empty string.

With this, the output generated when the user tries to attach twice to
the same process is kept minimal and concise.

It was still necessary to make a small adjustment in both testcases
because the order of the warnings was reversed: we now first print the
message saying that the process is already attached to GDB, and then
print the ptrace strerror string.

> If it's the case that the old output might still be produced by some
> platforms, care must be taken to ensure that both cases (still) pass.

Right.  I believe that with the change I described above it won't be
necessary to worry about arch-specific cases.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/