Page MenuHomeFreeBSD

Add PROC_PDEATHSIG_SET to procctl interface.
ClosedPublic

Authored by munro_ip9.org on Apr 16 2018, 8:28 PM.

Details

Summary

Allow processes to request the delivery of a signal upon death
of their parent process. This provides approximately the same
behaviour as prctl(PR_SET_PDEATHSIG, ...) on Linux.

As discussed on freebsd-hackers:

https://marc.info/?l=freebsd-hackers&m=152368119728365&w=3

Test Plan

ATF tests are included.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

munro_ip9.org created this revision.Apr 16 2018, 8:28 PM
kib added a comment.Apr 16 2018, 8:49 PM

It looks fine overall, please look at several minor comments I posted.

I will wait some more days for more opinions and suggestions before committing.

lib/libc/sys/procctl.2
402 ↗(On Diff #41551)

I think it would be useful to note that zero acts same as the caller pid.

sys/kern/kern_procctl.c
577 ↗(On Diff #41551)

I think it is unnatural to keep the actual code in the locking switch. Might be, put a switch with only PDEATHSIG commands above this switch.

Updated documentation and moved PDEATHSIG handling into its own switch, as requested.

munro_ip9.org marked 2 inline comments as done.Apr 16 2018, 10:10 PM
mjg accepted this revision.Apr 16 2018, 10:34 PM
mjg added a subscriber: mjg.

I only have cosmetic remarks. Definitely looks good enough to be shipped.

lib/libc/sys/procctl.2
547 ↗(On Diff #41557)

A remark about the corner case of clearing the flag on creds-changing exec would be useful, but I don't know how to bolt it in here.

sys/kern/kern_procctl.c
568 ↗(On Diff #41557)

both this and the case below should use the already declared p pointer to shorten the lines.

p = td->td_proc;
......

This revision is now accepted and ready to land.Apr 16 2018, 10:34 PM
seanc added a subscriber: seanc.Apr 16 2018, 10:57 PM

Use 'p' variable instead of repeating td->td_proc, as requested.

This revision now requires review to proceed.Apr 17 2018, 1:24 AM
munro_ip9.org marked 2 inline comments as done.Apr 17 2018, 1:27 AM

Thanks for the reviews and help!

lib/libc/sys/procctl.2
547 ↗(On Diff #41557)

That is covered by the following man page hunk:

+The value is cleared for the children
+of fork() and when executing set-user-ID or set-group-ID binaries.

munro_ip9.org marked 2 inline comments as done.Apr 17 2018, 1:28 AM

Use 'p' instead of td->td_proc in one more place that I'd missed. (Thanks to mjg for prod via IRC.) Sorry for the churn.

emaste added a subscriber: emaste.Apr 17 2018, 2:17 PM
mjg accepted this revision.Apr 17 2018, 2:43 PM
This revision is now accepted and ready to land.Apr 17 2018, 2:43 PM
jilles added a subscriber: jilles.Apr 17 2018, 10:43 PM

The code itself looks good, but something could be improved in the man page and test.

lib/libc/sys/procctl.2
403–404 ↗(On Diff #41562)

for child processes (since this is not just fork() but also vfork(), rfork() and pdfork())

406 ↗(On Diff #41562)

.Vt int

419 ↗(On Diff #41562)

.Vt int

523 ↗(On Diff #41562)

.Fa id and .Fa id_type

tests/sys/kern/pdeathsig.c
191–192 ↗(On Diff #41562)

This is nice and simple, but also slow. Consider mechanisms such as sigsuspend() and sigwait().

199 ↗(On Diff #41562)

Exiting a child process should be done via _exit(0); (here and in some more locations). The code that executes after the test returns may not produce desirable effects when run in a child process.

Used _exit(0) to exit the tests's subprocesses successfully. Use sigwait to wait for signals synchronously. Documentation tweaks.

This revision now requires review to proceed.Apr 18 2018, 12:09 AM
munro_ip9.org marked 6 inline comments as done.Apr 18 2018, 12:10 AM

The "fork_no_inherit" and "exec_inherit" tests' child processes also need to call _exit(0), so here's a new version to do that. Thanks for the review jilles. I agree the tests look much better with those changes.

kib added inline comments.Apr 18 2018, 6:31 AM
sys/kern/kern_thread.c
116 ↗(On Diff #41608)

This one must be changed to 0x318. On amd64 it seems that the new int member fits into some pad, but not on i386.

Fixed assertion about struct layout on i386. Fixed typo in man page (s/pont/point/).

munro_ip9.org marked an inline comment as done.Apr 18 2018, 8:48 AM
kib accepted this revision.Apr 18 2018, 10:29 AM

I will commit this later today, giving some to for jilles to comment further.

This revision is now accepted and ready to land.Apr 18 2018, 10:29 AM
jilles accepted this revision.Apr 18 2018, 8:12 PM

I like it.

This revision was automatically updated to reflect the committed changes.