Changeset View
Standalone View
sys/kern/sys_process.c
Show First 20 Lines • Show All 1,119 Lines • ▼ Show 20 Lines | sendsig: | ||||
} | } | ||||
p->p_xsig = data; | p->p_xsig = data; | ||||
p->p_xthread = NULL; | p->p_xthread = NULL; | ||||
if ((p->p_flag & (P_STOPPED_SIG | P_STOPPED_TRACE)) != 0) { | if ((p->p_flag & (P_STOPPED_SIG | P_STOPPED_TRACE)) != 0) { | ||||
/* deliver or queue signal */ | /* deliver or queue signal */ | ||||
td2->td_dbgflags &= ~TDB_XSIG; | td2->td_dbgflags &= ~TDB_XSIG; | ||||
td2->td_xsig = data; | td2->td_xsig = data; | ||||
/* | |||||
* P_WKILLED is insurance that a PT_KILL always works | |||||
* immediately, even if another thread is unsuspended | |||||
* first and attempts to handle a different signal or if | |||||
* the POSIX.1b style signal queue cannot accommodate | |||||
* any new signals. | |||||
*/ | |||||
if (data == SIGKILL) | |||||
p->p_flag |= P_WKILLED; | |||||
jhb: Perhaps this should be in the PT_KILL case before the 'goto sendsig'? I think we might end up… | |||||
Not Done Inline ActionsI was aiming to avoid differences between, e.g., PT_CONTINUE with SIGKILL and PT_KILL (the latter being described in the manpage as behaving "as if PT_CONTINUE had been used with SIGKILL given as the signal"). But now that you ask the question, I'm thinking there may be requests where this isn't appropriate. Re: clearing ptrace event mask: tdsendsignal will do this once the thread in ptracestop() wakes up and runs it, but perhaps it could race with another thread that stops incorrectly before the mask is cleared. In fact, the more I think about the mask clearing in tdsendsignal, the less certain I am that it is correct at all. It will clear the ptrace mask if SIGKILL is sent (from anywhere), but in the case the signal was sent from some source other than ptrace, the debugger may later catch and discard the SIGKILL. I'm not sure how the mask would be restored in that case. badger: I was aiming to avoid differences between, e.g., PT_CONTINUE with SIGKILL and PT_KILL (the… | |||||
Not Done Inline ActionsI think perhaps something like this is actually desirable: diff diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c index ddde0dc..f03a36b 100644 --- a/sys/kern/kern_sig.c +++ b/sys/kern/kern_sig.c @@ -2527,7 +2527,7 @@ ptracestop(struct thread *td, int sig, ksiginfo_t *si) td->td_xsig = sig; - if (si != NULL && P_KILLED(p)) { + if (P_KILLED(p)) { /* * Ensure that, if we've been PT_KILLed, the exit status * reflects that. Another thread may also be in ptracestop(), @@ -2535,6 +2535,7 @@ ptracestop(struct thread *td, int sig, ksiginfo_t *si) * unsuspended first. */ td->td_xsig = SIGKILL; + p->p_ptevents = 0; } else if (si == NULL || (si->ksi_flags & KSI_PTRACE) == 0) { td->td_dbgflags |= TDB_XSIG; CTR4(KTR_PTRACE, "ptracestop: tid %d (pid %d) flags %#x sig %d", That is, whichever thread hits the next ptracestop(), for whatever reason, will ensure a SIGKILL is queued up and then clear the stop mask so it can hurry off to handle the SIGKILL. The reason I think this is better than clearing p_ptevents in sys_process.c is that it should guarantee that the process exits with SIGKILL, which I think is useful for tests, if nothing else (I don't think gdb cares). If p_ptevents is cleared here in sys_process.c, there is still a race between the thread that accepted the SIGKILL and exit(). Another thread may exit() normally before the SIGKILL is handled. The above diff should prevent it. badger: I think perhaps something like this is actually desirable:
```diff
diff --git… | |||||
Not Done Inline ActionsI looked again and can't see any issue with setting the P_WKILLED flag here instead of only for PT_KILL. If you've spotted something, however, please point me to it. I added a test for the race I described above which passes. I think it still may be possible for a thread to exit() after a PT_KILL, but this is a narrow corner case which I think is safe to address separately. I'd also prefer to address the clearing of p_ptevents in tdsendsignal separately, mainly because I want to go get a clue about procfs events too. badger: I looked again and can't see any issue with setting the P_WKILLED flag here instead of only for… | |||||
if (req == PT_DETACH) { | if (req == PT_DETACH) { | ||||
FOREACH_THREAD_IN_PROC(p, td3) | FOREACH_THREAD_IN_PROC(p, td3) | ||||
td3->td_dbgflags &= ~TDB_SUSPEND; | td3->td_dbgflags &= ~TDB_SUSPEND; | ||||
} | } | ||||
/* | /* | ||||
* unsuspend all threads, to not let a thread run, | * unsuspend all threads, to not let a thread run, | ||||
* you should use PT_SUSPEND to suspend it before | * you should use PT_SUSPEND to suspend it before | ||||
* continuing process. | * continuing process. | ||||
▲ Show 20 Lines • Show All 315 Lines • Show Last 20 Lines |
Perhaps this should be in the PT_KILL case before the 'goto sendsig'? I think we might end up needing other logic here such as clearing the ptrace event mask, though I'm fine with fixing that as a separate followup if so.