Don't clobber td->td_retval[0] in proc_reap().
ClosedPublic
Actions

Authored by ed on Jul 9 2015, 8:54 AM.

Details

Reviewers

kib
jilles
rwatson

Commits

rS285312: Don't clobber td->td_retval[0] in proc_reap().

Summary

While writing tests for CloudABI, I noticed that close() on process descriptors returns the process ID of the child process. This is interesting, as close() is only allowed to return 0 or -1. It turns out that we clobber td->td_retval[0] in proc_reap(), so that wait*() properly returns the process ID.

Change proc_reap() to leave td->td_retval[0] alone. Set the process ID in kern_wait6() instead, by keeping track of the PID before we (potentially) reap the process.

Test Plan

CloudABI unit tests pass. FreeBSD wait*() still seems to work; the system works fine.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

ed updated this revision to Diff 6807.Jul 9 2015, 8:54 AM

ed retitled this revision from to Don't clobber td->td_retval[0] in proc_reap()..

ed updated this object.

ed edited the test plan for this revision. (Show Details)

ed added reviewers: rwatson, kib, jilles.

ed set the repository for this revision to rS FreeBSD src repository - subversion.

Herald added a subscriber: imp. · View Herald TranscriptJul 9 2015, 8:54 AM

Overall it is fine, just consider my minor note above.

sys/kern/kern_exit.c
842	This is fine.
1193	But I do not like this part of the change. IMO the explicit placement of the assignment for each return branch would make both your goal and code cleaner, rather then finding yet another 'common' place. From what I see, this means that the line commented should move to the block with 'return 0' right below the proc_to_reap() call. Everything else is already handled by the method I see as cleaner.

ed updated this revision to Diff 6812.Jul 9 2015, 11:22 AM

ed updated this object.

ed edited edge metadata.

ed added inline comments.Jul 9 2015, 11:24 AM

sys/kern/kern_exit.c
1193	Yes, I can see what you're hinting at. Makes sense! We do need to keep track of the PID now, as proc_reap() might destroy the process structure. Let's keep track of the PID in a new variable `pid` and set it right before we `return (0);`. Does this look all right?

kib accepted this revision.Jul 9 2015, 11:41 AM

kib edited edge metadata.

This revision is now accepted and ready to land.Jul 9 2015, 11:41 AM

Closed by commit rS285312: Don't clobber td->td_retval[0] in proc_reap(). (authored by ed). · Explain WhyJul 9 2015, 12:04 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

sys/

kern/

kern_exit.c

13 lines

Diff 6812

View Options

Don't clobber td->td_retval[0] in proc_reap().ClosedPublicActions