Fix PTRACE_INTERRUPT to actually work if the tracee is not already stopped. (#3923)

The current code is completely untested because it will always call
syscall_state.emulate_result twice and fatally assert. The ptrace_seize test
does not yield between PTRACE_CONT and PTRACE_INTERRUPT, so rr will not
switch to the emulated ptracee and resume execution, and the PTRACE_INTERRUPT
will always find the emulated ptracee already stopped. Additionally, the test
does not wait on the emulated ptracee again after interrupting it, so the
following assertion about the value of status is bogus.

Fix all of that by adding the appropriate waitpid, tweaking the status
assertion, duplicating the PTRACE_CONT/PTRACE_INTERRUPT sequence but with
an intervening sched_yield() to force the emulated ptracee to resume, and
fixing rr's PTRACE_INTERRUPT emulation to emulate_result only once and to
place an emulated stop on the emulated ptracee.

Author: khuey

src/Scheduler.h

@@ -211,7 +211,7 @@ class Scheduler {
    * the next runnable task after 't' in round-robin order.
    * Sets 'by_waitpid' to true if we determined the task was runnable by
    * calling waitpid on it and observing a state change. This task *must*
-   * be returned by get_next_thread, and is_runnable_task must not be called
+   * be returned by get_next_thread, and is_task_runnable must not be called
    * on it again until it has run.
    * Considers only tasks with priority <= priority_threshold.
    */

src/record_syscall.cc

@@ -3061,12 +3061,19 @@ static Switchable prepare_ptrace(RecordTask* t,
     case PTRACE_INTERRUPT: {
       RecordTask* tracee = verify_ptrace_target(t, syscall_state, pid, false);
       if (tracee) {
+        uint64_t result = 0;
         if (!tracee->is_stopped()) {
           // Running in a blocked syscall. Forward the PTRACE_INTERRUPT.
           // Regular syscall exit handling will take over from here.
+          LOG(debug) << "Interrupting " << tracee->tid;
           errno = 0;
           tracee->fallible_ptrace(PTRACE_INTERRUPT, nullptr, nullptr);
-          syscall_state.emulate_result(-errno);
+          result = -errno;
+          // Technically PTRACE_INTERRUPT stops are distinct from group stops,
+          // but not in any way we currently care about.
+          // NB: Despite the ptrace man page claiming the kernel sends SIGTRAP
+          // in practice it actually sends SIGSTOP.
+          tracee->apply_group_stop(SIGSTOP);
         } else if (tracee->status().is_syscall()) {
           tracee->emulate_ptrace_stop(tracee->status(), SYSCALL_EXIT_STOP);
         } else if (tracee->emulated_stop_pending == NOT_STOPPED) {
@@ -3075,7 +3082,7 @@ static Switchable prepare_ptrace(RecordTask* t,
           tracee->apply_group_stop(SIGSTOP);
         }
         // Otherwise, there's nothing to do.
-        syscall_state.emulate_result(0);
+        syscall_state.emulate_result(result);
       }
       break;
     }

src/test/ptrace_seize.c

@@ -40,9 +40,25 @@ int main(void) {
   test_assert(status == ((PTRACE_EVENT_STOP << 16) | (SIGSTOP << 8) | 0x7f));
 
   test_assert(0 == ptrace(PTRACE_CONT, child, NULL, 0));
+  /* Most likely there is no context switch between these two syscalls, rr does
+   * not actually restart the tracee, and this PTRACE_INTERRUPT sees the task
+   * is already stopped internally, and is emulated as a noop.
+   */
   test_assert(0 == ptrace(PTRACE_INTERRUPT, child, NULL, 0));
-  test_assert(status == ((PTRACE_EVENT_STOP << 16) | (SIGSTOP << 8) | 0x7f) ||
-              status == (((SIGTRAP | 0x80) << 8) | 0x7f));
+  test_assert(child == waitpid(child, &status, 0));
+  test_assert(status == ((PTRACE_EVENT_STOP << 16) | (SIGSTOP << 8) | 0x7f));
+
+  test_assert(WIFSTOPPED(status));
+
+  test_assert(0 == ptrace(PTRACE_CONT, child, NULL, 0));
+  /* Use a sched_yield() to force rr to context switch and restart the tracee,
+   * so when we PTRACE_INTERRUPT below rr is forced to PTRACE_INTERRUPT the
+   * tracee.
+   */
+  sched_yield();
+  test_assert(0 == ptrace(PTRACE_INTERRUPT, child, NULL, 0));
+  test_assert(child == waitpid(child, &status, 0));
+  test_assert(status == ((PTRACE_EVENT_STOP << 16) | (SIGSTOP << 8) | 0x7f));
 
   test_assert(WIFSTOPPED(status));