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

["Michael Kerrisk (man-pages)" <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hi Jann,

Ping!

Cheers,

Michael


On 01/08/2016 08:18 PM, 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...
> 
> Thanks,
> 
> Michael
> 
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html