On Thu, Jul 14, 2016 at 11:16:05AM -0700, Mark Johnston wrote:
> Please see the program here:
> https://people.freebsd.org/~markj/ptrace_stop.c
> 
> It cheats a bit: it uses SIGSTOP to stop the child before sending a
> SIGHUP to it. However, this is just for convenience; note that PT_ATTACH
> will result in a call to thread_unsuspend() on the child, so PT_ATTACH's
> SIGSTOP will be delivered to a running process. When ptrace attaches,
> the child stops and WSTOPSIG(status) == SIGHUP. When ptrace detaches,
> the child is left stopped.
No, it is not for convenience, it relies on another bug to get the effect,
see below.

As I understand you intent, you prefer to get SIGSTOP from the first
waitpid(2) call after successful PT_ATTACH, am I right ?  At least for
single-threaded case, this can be achieved with a flag indicating that
we a doing first cursig(9) action after the attach, and preferring
SIGSTOP over any other queued signal.  The new flag P2_PTRACE_FSTP
does just that.  For mt case, I believe that some enchancements to
my proc_next_xthread() would fix that.

But when debugging the code, I found that it still does not work reliably
for your test.  The reason is that issignal() consumes a queued stop signal
after the thread_suspend_switch().  It allows the attach to occur, but then
sigqueue_delete() calls ('take the signal!') eat the signal for attach. It
seems that we should consume stops before going to stop state.  An open
question is how much this hurts when another (non-debugging) SIGSTOP is
queued while in stopped state.

Please try this.

diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index 2a5e6de..36ed15f 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
@@ -2525,22 +2525,21 @@ ptracestop(struct thread *td, int sig)
                        PROC_SUNLOCK(p);
                        return (sig);
                }
-               /*
-                * Just make wait() to work, the last stopped thread
-                * will win.
-                */
-               p->p_xsig = sig;
-               p->p_xthread = td;
-               p->p_flag |= (P_STOPPED_SIG|P_STOPPED_TRACE);
-               sig_suspend_threads(td, p, 0);
-               if ((td->td_dbgflags & TDB_STOPATFORK) != 0) {
-                       td->td_dbgflags &= ~TDB_STOPATFORK;
-                       cv_broadcast(&p->p_dbgwait);
+               if (p->p_xthread == NULL)
+                       p->p_xthread = td;
+               if (p->p_xthread == td) {
+                       p->p_xsig = sig;
+                       p->p_flag |= P_STOPPED_SIG | P_STOPPED_TRACE;
+                       sig_suspend_threads(td, p, 0);
+                       if ((td->td_dbgflags & TDB_STOPATFORK) != 0) {
+                               td->td_dbgflags &= ~TDB_STOPATFORK;
+                               cv_broadcast(&p->p_dbgwait);
+                       }
                }
 stopme:
                thread_suspend_switch(td, p);
                if (p->p_xthread == td)
-                       p->p_xthread = NULL;
+                       proc_next_xthread(p);
                if (!(p->p_flag & P_TRACED))
                        break;
                if (td->td_dbgflags & TDB_SUSPEND) {
@@ -2725,7 +2724,20 @@ issignal(struct thread *td)
                        SIG_STOPSIGMASK(sigpending);
                if (SIGISEMPTY(sigpending))     /* no signal to send */
                        return (0);
-               sig = sig_ffs(&sigpending);
+               if ((p->p_flag & (P_TRACED | P_PPTRACE)) == P_TRACED &&
+                   (p->p_flag2 & P2_PTRACE_FSTP) != 0 &&
+                   SIGISMEMBER(sigpending, SIGSTOP)) {
+                       /*
+                        * If debugger just attached, always consume
+                        * SIGSTOP from ptrace(PT_ATTACH) first, to
+                        * execute the debugger attach ritual in
+                        * order.
+                        */
+                       sig = SIGSTOP;
+                       p->p_flag2 &= ~P2_PTRACE_FSTP;
+               } else {
+                       sig = sig_ffs(&sigpending);
+               }
 
                if (p->p_stops & S_SIG) {
                        mtx_unlock(&ps->ps_mtx);
@@ -2742,7 +2754,7 @@ issignal(struct thread *td)
                        sigqueue_delete(&p->p_sigqueue, sig);
                        continue;
                }
-               if (p->p_flag & P_TRACED && (p->p_flag & P_PPTRACE) == 0) {
+               if ((p->p_flag & (P_TRACED | P_PPTRACE)) == P_TRACED) {
                        /*
                         * If traced, always stop.
                         * Remove old signal from queue before the stop.
@@ -2845,6 +2857,8 @@ issignal(struct thread *td)
                                mtx_unlock(&ps->ps_mtx);
                                WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK,
                                    &p->p_mtx.lock_object, "Catching SIGSTOP");
+                               sigqueue_delete(&td->td_sigqueue, sig);
+                               sigqueue_delete(&p->p_sigqueue, sig);
                                p->p_flag |= P_STOPPED_SIG;
                                p->p_xsig = sig;
                                PROC_SLOCK(p);
@@ -2852,7 +2866,7 @@ issignal(struct thread *td)
                                thread_suspend_switch(td, p);
                                PROC_SUNLOCK(p);
                                mtx_lock(&ps->ps_mtx);
-                               break;
+                               goto next;
                        } else if (prop & SA_IGNORE) {
                                /*
                                 * Except for SIGCONT, shouldn't get here.
@@ -2883,6 +2897,7 @@ issignal(struct thread *td)
                }
                sigqueue_delete(&td->td_sigqueue, sig); /* take the signal! */
                sigqueue_delete(&p->p_sigqueue, sig);
+next:;
        }
        /* NOTREACHED */
 }
diff --git a/sys/kern/sys_process.c b/sys/kern/sys_process.c
index a6037e3..af6b231 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
@@ -885,6 +885,7 @@ kern_ptrace(struct thread *td, int req, pid_t pid, void 
*addr, int data)
        case PT_TRACE_ME:
                /* set my trace flag and "owner" so it can read/write me */
                p->p_flag |= P_TRACED;
+               p->p_flag2 |= P2_PTRACE_FSTP;
                if (p->p_flag & P_PPWAIT)
                        p->p_flag |= P_PPTRACE;
                p->p_oppid = p->p_pptr->p_pid;
@@ -903,6 +904,7 @@ kern_ptrace(struct thread *td, int req, pid_t pid, void 
*addr, int data)
                 * on a "detach".
                 */
                p->p_flag |= P_TRACED;
+               p->p_flag2 |= P2_PTRACE_FSTP;
                p->p_oppid = p->p_pptr->p_pid;
                if (p->p_pptr != td->td_proc) {
                        proc_reparent(p, td->td_proc);
@@ -1057,7 +1059,7 @@ kern_ptrace(struct thread *td, int req, pid_t pid, void 
*addr, int data)
                        proctree_locked = 0;
                }
                p->p_xsig = data;
-               p->p_xthread = NULL;
+               proc_next_xthread(p);
                if ((p->p_flag & (P_STOPPED_SIG | P_STOPPED_TRACE)) != 0) {
                        /* deliver or queue signal */
                        td2->td_dbgflags &= ~TDB_XSIG;
@@ -1065,7 +1067,8 @@ kern_ptrace(struct thread *td, int req, pid_t pid, void 
*addr, int data)
 
                        if (req == PT_DETACH) {
                                FOREACH_THREAD_IN_PROC(p, td3)
-                                       td3->td_dbgflags &= ~TDB_SUSPEND; 
+                                       td3->td_dbgflags &= ~(TDB_SUSPEND |
+                                           TDB_XSIG);
                        }
                        /*
                         * unsuspend all threads, to not let a thread run,
@@ -1376,9 +1379,24 @@ stopevent(struct proc *p, unsigned int event, unsigned 
int val)
        do {
                if (event != S_EXIT)
                        p->p_xsig = val;
-               p->p_xthread = NULL;
+               proc_next_xthread(p);
                p->p_stype = event;     /* Which event caused the stop? */
                wakeup(&p->p_stype);    /* Wake up any PIOCWAIT'ing procs */
                msleep(&p->p_step, &p->p_mtx, PWAIT, "stopevent", 0);
        } while (p->p_step);
 }
+
+void
+proc_next_xthread(struct proc *p)
+{
+       struct thread *td;
+
+       PROC_LOCK_ASSERT(p, MA_OWNED);
+       FOREACH_THREAD_IN_PROC(p, td) {
+               if (td == p->p_xthread)
+                       continue;
+               if ((td->td_dbgflags & TDB_XSIG) != 0)
+                       break;
+       }
+       p->p_xthread = td;
+}
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index f533db6..7ab7410 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -712,6 +712,7 @@ struct proc {
 #define        P2_NOTRACE_EXEC 0x00000004      /* Keep P2_NOPTRACE on exec(2). 
*/
 #define        P2_AST_SU       0x00000008      /* Handles SU ast for kthreads. 
*/
 #define        P2_LWP_EVENTS   0x00000010      /* Report LWP events via 
ptrace(2). */
+#define        P2_PTRACE_FSTP  0x00000020      /* First issignal() after 
PT_ATTACH. */
 
 /* Flags protected by proctree_lock, kept in p_treeflags. */
 #define        P_TREE_ORPHANED         0x00000001      /* Reparented, on 
orphan list */
@@ -999,6 +1000,7 @@ int        proc_getenvv(struct thread *td, struct proc *p, 
struct sbuf *sb);
 void   procinit(void);
 void   proc_linkup0(struct proc *p, struct thread *td);
 void   proc_linkup(struct proc *p, struct thread *td);
+void   proc_next_xthread(struct proc *p);
 struct proc *proc_realparent(struct proc *child);
 void   proc_reap(struct thread *td, struct proc *p, int *status, int options);
 void   proc_reparent(struct proc *child, struct proc *newparent);
_______________________________________________
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

Reply via email to