Only clear a pending thread event if one is pending.

[jhb]

Summary

If only P_STOPPED_SIG is set but not P_STOPPED_TRACE, then p_xthread
might be NULL. While here, try to adjust some comments to more
clearly document some of the cases.

There are some XXX questions I've added that I'd like some feedback on.
I can either adjust code and/or comments based on feedback. I haven't
run Peter's stress test against this but simple tests in kyua pass.

Test Plan

• tests/sys/kern/ptrace_test

[pho]

I ran all of the ptrace(2) scenarios several times, without seeing any problems.

[kib]

sys/kern/sys_process.c

1132 ↗

(On Diff #34462)

Should this comment become an assertion ?

1159 ↗

(On Diff #34462)

I prefer to move the previous P_STOPPED_TRACE block above the beginning of the P_STOPPED_SIG|P_STOPPED_TRACE block and then move the p_xsig = data into it.
At least it reduces the indent.

1179 ↗

(On Diff #34462)

Commit this comment enhancement separately, perhaps ?

1195 ↗

(On Diff #34462)

I am not sure that I understand this comment fully. Do you mean that the assert there should be

MPASS(td2 != NULL || req == PT_ATTACH);

1205 ↗

(On Diff #34462)

What condition do you mean to be always true ? The data != 0 ? What if userspace does ptrace(PT_CONTINUE, data = 0) ?

[jhb]

sys/kern/sys_process.c

871 ↗

(On Diff #34462)

I think this can change to require P_STOPPED_TRACE. In kern_sig.c we never set P_STOPPED_SIG without P_STOPPED_TRACE on a traced process.

1132 ↗

(On Diff #34462)

I don't think it is required for correctness. At one point I had looked at "inlining" the sendsig handling for PT_ATTACH and PT_KILL to avoid the goto and see if the resulting code was simpler, but it wasn't. I do want to document a bit which parts are different though for the different cases.

1195 ↗

(On Diff #34462)

No, I would need some new variable to hold the "old" value of p_xthread before it was cleared. However, all of the requests that get to this point require P_STOPPED_TRACE other than PT_ATTACH.

1205 ↗

(On Diff #34462)

Any request other than PT_ATTACH fails if P_STOPPED_TRACE isn't set (see my note up above on line 871), so we can only be in this case for PT_ATTACH I believe.

[jhb]

• Move P_STOPPED_TRACE section up.
• Require P_STOPPED_TRACE for most actions.
• Clarify some comments.

[jhb]

• Consolidate kern_psignal cases for PT_ATTACH.

[jhb]

I will probably try to "uninline" the PT_ATTACH case as a followup after this. After the last bits of refactoring I think it will probably be worth it.

[kib]

sys/kern/sys_process.c

878 ↗

(On Diff #35202)

Is this something local ?

[jhb]

sys/kern/sys_process.c

878 ↗

(On Diff #35202)

Nope, added in rS153697