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:
signature.asc
Description: Digital signature
