Re: [PATCH] prctl.2: PR_SET_PDEATHSIG by orphan process

[Jann Horn <jann-XZ1E9jl8jIdeoWH0uzbU5w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4431 bytes --]

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.


> 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:

$ 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.
Sorry, I probably should have tried to figure out the details of
this before sending a manpages patch.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]