On 05/02/15 07:39 PM, Bryce Harrington wrote: > This fixes a regression in the testsuite since c3653f7f, where four of > the timeout tests fail with "Timeouts suppressed" messages. > > The timeouts are being suppressed because the testsuite is erroneously > detecting that a debugger is attached. This detection mechanism > (adopted from libinput) uses ptrace to test if there is a debugger > parent process that can be attached. Unfortunately, this is an > unreliable test: Kernel security policies exist to restrict the scope of > ptrace to prevent processes from snooping on one another.[1] This > security policy is set as the default on Ubuntu, and potentially other > Linux distributions.[2] > > The Yama documentation suggests, "For software that has defined > application-specific relationships between a debugging process and its > inferior (crash handlers, etc), prctl(PR_SET_PTRACER, pid, ...) can be > used. An inferior can declare which other process (and its descendents) > are allowed to call PTRACE_ATTACH against it." This prctl call has no > effect if Yama LSM is not loaded. > > The child needs to be synchronized to the client to prevent a race > condition where the child might try to operate before the parent has > finished its prctl call. This synchronization is done via pipes. > > This patch can be tested by running sanity-test with > /proc/sys/kernel/yama/ptrace_scope set to 0 or 1; the test must pass for > either value. > > 1: > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=2d514487faf188938a4ee4fb3464eeecfbdcf8eb > 2: > https://wiki.ubuntu.com/SecurityTeam/Roadmap/KernelHardening#ptrace_Protection > > Signed-off-by: Bryce Harrington <[email protected]> > Reviewed-by: Marek Chalupa <[email protected]> > > v4: Allow parent to communicate error state to child to prevent leaving > child in zombie state if parent hits an error. > > v5: Check errno instead of rc for error. Don't waitpid on ppid. > (I guess these two coding errors canceled out for me or something.)
FWIW, the waitpid thing was never a bug, I errantly called it one in an off list conversation. It'll work fine the way it is now, or the way it was. Either way you choose: Reviewed-by: Derek Foreman <[email protected]> > Signed-off-by: Bryce Harrington <[email protected]> > --- > tests/test-runner.c | 62 > +++++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 51 insertions(+), 11 deletions(-) > > diff --git a/tests/test-runner.c b/tests/test-runner.c > index 48e9a22..f031a42 100644 > --- a/tests/test-runner.c > +++ b/tests/test-runner.c > @@ -34,6 +34,10 @@ > #include <errno.h> > #include <limits.h> > #include <sys/ptrace.h> > +#include <sys/prctl.h> > +#ifndef PR_SET_PTRACER > +# define PR_SET_PTRACER 0x59616d61 > +#endif > > #include "test-runner.h" > > @@ -249,29 +253,65 @@ stderr_reset_color(void) > > /* this function is taken from libinput/test/litest.c > * (rev 028513a0a723e97941c39) > + * > + * Returns: 1 if a debugger is confirmed present; 0 if no debugger is > + * present or if it can't be determined. > */ > static int > is_debugger_attached(void) > { > int status; > int rc; > - pid_t pid = fork(); > + pid_t pid; > + int pipefd[2]; > > - if (pid == -1) > + if (pipe(pipefd) == -1) { > + perror("pipe"); > return 0; > + } > > - if (pid == 0) { > + pid = fork(); > + if (pid == -1) { > + perror("fork"); > + close(pipefd[0]); > + close(pipefd[1]); > + return 0; > + } else if (pid == 0) { > + char buf; > pid_t ppid = getppid(); > - if (ptrace(PTRACE_ATTACH, ppid, NULL, NULL) == 0) { > - waitpid(ppid, NULL, 0); > - ptrace(PTRACE_CONT, NULL, NULL); > - ptrace(PTRACE_DETACH, ppid, NULL, NULL); > - rc = 0; > + > + /* Wait until parent is ready */ > + close(pipefd[1]); /* Close unused write end */ > + read(pipefd[0], &buf, 1); > + close(pipefd[0]); > + if (buf == '-') > + _exit(1); > + if (ptrace(PTRACE_ATTACH, ppid, NULL, NULL) != 0) > + _exit(1); > + if (!waitpid(-1, NULL, 0)) > + _exit(1); > + ptrace(PTRACE_CONT, NULL, NULL); > + ptrace(PTRACE_DETACH, ppid, NULL, NULL); > + _exit(0); > + } else { > + close(pipefd[0]); > + > + /* Enable child to ptrace the parent process */ > + rc = prctl(PR_SET_PTRACER, pid); > + if (rc != 0 && errno != EINVAL) { > + /* An error prevents us from telling if a debugger is > attached. > + * Instead of propagating the error, assume no debugger > present. > + * But note the error to the log as a clue for > troubleshooting. > + * Then flag the error state to the client by sending > '-'. > + */ > + perror("prctl"); > + write(pipefd[1], "-", 1); > } else { > - rc = 1; > + /* Signal to client that parent is ready by passing '+' > */ > + write(pipefd[1], "+", 1); > } > - _exit(rc); > - } else { > + close(pipefd[1]); > + > waitpid(pid, &status, 0); > rc = WEXITSTATUS(status); > } > _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
