On Fri, Jan 08, 2016 at 08:18:57PM +0100, Michael Kerrisk (man-pages) wrote:
> Hi Jann,
>
> On 01/08/2016 05:39 PM, Jann Horn wrote:
> > On Fri, Jan 08, 2016 at 05:21:55PM +0100, Michael Kerrisk (man-pages) wrote:
> >> Hi Jann,
> >>
> >> Your mail is a little cryptic. It would be best to start with
> >> a brief summary of your point--something like the text of your
> >> patch at the end of the mail.
> >
> > Ok, will do that next time. I wanted to avoid duplicating the content.
>
> But you did it again :-). See below.
>
> >> On 01/06/2016 07:23 PM, Jann Horn wrote:
> >>> Proof:
> >>> In kernel/sys.c:
> >>>
> >>> case PR_SET_PDEATHSIG:
> >>> if (!valid_signal(arg2)) {
> >>> error = -EINVAL;
> >>> break;
> >>> }
> >>> me->pdeath_signal = arg2;
> >>> break;
> >>
> >> I don't understand how the code above relates to the point you
> >> want to make. (Or maybe you mean: "look, there's no check here
> >> to see that if the parent is already dead"; but it would help
> >> to state that explicitly).
> >
> > Yes, that's what I meant.
> >
> >
> >>> Testcase:
> >>>
> >>> #include <sys/prctl.h>
> >>> #include <err.h>
> >>> #include <unistd.h>
> >>> #include <signal.h>
> >>> #include <stdio.h>
> >>>
> >>> void ponk(int s) {
> >>> puts("ponk!");
> >>> }
> >>>
> >>> int main(void) {
> >>> if (fork() == 0) {
> >>> if (fork() == 0) {
> >>> sleep(1);
> >>> signal(SIGUSR1, ponk);
> >>> prctl(PR_SET_PDEATHSIG, SIGUSR1, 0, 0, 0);
> >>> sleep(1);
> >>> return 0;
> >>> }
> >>> return 0;
> >>> }
> >>>
> >>> sleep(3);
> >>> return 0;
> >>> }
> >>> ---
> >>> man2/prctl.2 | 3 +++
> >>> 1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/man2/prctl.2 b/man2/prctl.2
> >>> index 5cea3bb..3dce8e9 100644
> >>> --- a/man2/prctl.2
> >>> +++ b/man2/prctl.2
> >>> @@ -670,6 +670,9 @@ In other words, the signal will be sent when that thread terminates
> >>> (via, for example,
> >>> .BR pthread_exit (3)),
> >>> rather than after all of the threads in the parent process terminate.
> >>> +
> >>> +If the parent has already died by the time the parent death signal
> >>> +is set, the new parent death signal will not be sent.
> >>
> >> In a way, this seems almost obvious. But perhaps it is better to make the
> >> point explicitly, as you suggest. But, because there may have been a
> >> previous PR_SET_PDEATHSIG, I'd prefer something like this:
> >>
> >> [[
> >> If the caller's parent has already died by the time of this
> >> PR_SET_PDEATHSIG operation, the operation shall have no effect.
> >> ]]
> >>
> >> What do you think?
> >
> > I don't think "no effect" would be strictly correct because weird stuff
> > happens on subreaper death - I'm not sure whether this is intended or a
> > bug though:
>
> Pause. Please begin with a short explanation of what you're about to
> demonstrate with the following code.... As it is, I am (again) not at
> all clear about the point you are trying to make.
>
> > $ cat deathsig2.c
> > #include <sys/prctl.h>
> > #include <err.h>
> > #include <unistd.h>
> > #include <signal.h>
> > #include <stdio.h>
> >
> > void ponk(int s) {
> > puts("ponk!");
> > }
> >
> > int main(void) {
> > if (fork() == 0) {
> > prctl(PR_SET_CHILD_SUBREAPER, 1, 0, 0, 0);
> > puts("enabled subreaper");
> > if (fork() == 0) {
> > if (fork() == 0) {
> > sleep(1);
> > puts("setting deathsig...");
> > signal(SIGUSR1, ponk);
> > prctl(PR_SET_PDEATHSIG, SIGUSR1, 0, 0, 0);
> > sleep(2);
> > return 0;
> > }
> > puts("parent will die now, causing reparent to subreaper");
> > return 0;
> > }
> > sleep(2);
> > puts("subreaper will die now");
> > return 0;
> > }
> > sleep(4);
> > return 0;
> > }
> > $ gcc -o deathsig2 deathsig2.c
> > $ cat deathsig3.c
> > #include <sys/prctl.h>
> > #include <err.h>
> > #include <unistd.h>
> > #include <signal.h>
> > #include <stdio.h>
> >
> > void ponk(int s) {
> > puts("ponk!");
> > }
> >
> > int main(void) {
> > if (fork() == 0) {
> > prctl(PR_SET_CHILD_SUBREAPER, 1, 0, 0, 0);
> > puts("enabled subreaper");
> > if (fork() == 0) {
> > if (fork() == 0) {
> > puts("setting deathsig...");
> > signal(SIGUSR1, ponk);
> > prctl(PR_SET_PDEATHSIG, SIGUSR1, 0, 0, 0);
> > sleep(3);
> > return 0;
> > }
> > sleep(1);
> > puts("parent will die now, causing reparent to subreaper");
> > return 0;
> > }
> > sleep(2);
> > puts("subreaper will die now");
> > return 0;
> > }
> > sleep(4);
> > return 0;
> > }
> > $ gcc -o deathsig3 deathsig3.c
> > $ ./deathsig2
> > enabled subreaper
> > parent will die now, causing reparent to subreaper
> > setting deathsig...
> > subreaper will die now
> > ponk!
> > $ ./deathsig3
> > enabled subreaper
> > setting deathsig...
> > parent will die now, causing reparent to subreaper
> > ponk!
> > subreaper will die now
> > $
> >
> > I didn't manage to find the reason for that in the code.
>
> The reason for *what*? I am none the wiser.... What do you
> see as anomalous in the above? Please explain, so I can
> follow you.
>
> > Sorry, I probably should have tried to figure out the details of
> > this before sending a manpages patch.
>
> FWIW, all of the above looks legitimate and expected to me, but
> again, I'm not sure, because you didn't explain your point, just
> showed some code...
Setting a parent death signal while the parent is still alive causes
a death signal to be sent when the parent dies, but no death signal
is sent on subsequent subreaper parent deaths.
Setting a parent death signal when the parent is already dead, as
far as I can tell, causes a death signal to be sent on the death of
the first subreaper - but not on further subreaper deaths after the
reported one.
This means that the statement "If the caller's parent has already
died by the time of this PR_SET_PDEATHSIG operation, the operation
shall have no effect." would be false because the operation could
still have the effect of sending a signal when the subreaper dies.
deathsig2.c demonstrates this: PR_SET_PDEATHSIG is used after the
parent has died and still has the effect of causing a signal
handler invocation.
Attachment:
signature.asc
Description: Digital signature
