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:

quoted

On Fri, Jan 08, 2016 at 05:21:55PM +0100, Michael Kerrisk (man-pages) wrote:

quoted

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.

quoted

quoted

On 01/06/2016 07:23 PM, Jann Horn wrote:

quoted

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.

quoted

quoted

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.

quoted

$ 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.

quoted

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.