> On 5 Sep 2020, at 03:22, Philip Guenther <guent...@gmail.com> wrote:
>
> On Fri, Sep 4, 2020 at 2:59 PM Mateusz Guzik <mjgu...@gmail.com> wrote:
>
>> On 9/5/20, Philip Guenther <guent...@gmail.com> wrote:
>>> On Fri, Sep 4, 2020 at 1:06 PM Mateusz Guzik <mjgu...@gmail.com> wrote:
>>>
>>>> On 9/4/20, Vitaliy Makkoveev <m...@openbsd.org> wrote:
>>>>> On Fri, Sep 04, 2020 at 05:24:42PM +0200, Mateusz Guzik wrote:
>>>>>> getppid blindly follows the parent pointer and reads the pid.
>>>>>>
>>>>>> The problem is that ptrace reparents the traced process, so in
>>>>>> particular if you gdb -p $something, the target proc will start
>> seeing
>>>>>> gdb instead of its actual parent.
>>>>>>
>>>>>> There is a lot to say about the entire reparenting business or
>> storing
>>>>>> the original pid in ps_oppid (instead of some form of a reference to
>>>>>> the process).
>>>>>>
>>>>>> However, I think the most feasible fix for now is the same thing
>>>>>> FreeBSD did: *always* store the actual parent pid in ps_oppid. This
>>>>>> means all repareting will keep updating it (most notably when
>>>>>> abandoning children on exit), while ptrace will skip that part.
>>>>>>
>>>>>> Side effect of such a change be that getppid will stop requiring the
>>>>>> kernel lock.
>>>>>>
>>>>>
>>>>> Thanks for report. But we are in beta stage now so such modification
>> is
>>>>> impossible until next iteration.
>>>>>
>>>>> Since original parent identifier is stored as `ps_oppid' while process
>>>>> is traced we just return it to userland for this case. This is the way
>>>>> I
>>>>> propose to fix this bug for now.
>>>>>
>>>>> Comments? OKs?
>>>>>
>>>>> Index: sys/kern/kern_prot.c
>>>>> ===================================================================
>>>>> RCS file: /cvs/src/sys/kern/kern_prot.c,v
>>>>> retrieving revision 1.76
>>>>> diff -u -p -r1.76 kern_prot.c
>>>>> --- sys/kern/kern_prot.c 9 Jul 2019 12:23:25 -0000 1.76
>>>>> +++ sys/kern/kern_prot.c 4 Sep 2020 21:12:15 -0000
>>>>> @@ -84,7 +84,11 @@ int
>>>>> sys_getppid(struct proc *p, void *v, register_t *retval)
>>>>> {
>>>>>
>>>>> - *retval = p->p_p->ps_pptr->ps_pid;
>>>>> + if (p->p_p->ps_flags & PS_TRACED)
>>>>> + *retval = p->p_p->ps_oppid;
>>>>> + else
>>>>> + *retval = p->p_p->ps_pptr->ps_pid;
>>>>> +
>>>>> return (0);
>>>>> }
>>>>
>>>> This is definitely a bare minimum fix, but it does the job.
>>>>
>>>
>>> ptrace() has behaved like this for the life of OpenBSD and an indefinite
>>> number of years previous in the BSD releases. What has happened that a
>>> definitely incomplete fix is needed Right Now?
>>
>> I don't see how this reads as a demand this is fixed Right Now.
>>
>
> I didn't call it a demand, but the point stands: what has changed?
>
>
> I don't see how the fix is incomplete either. It can be done better
>> with more effort, but AFAICS the above results in correct behavior.
>>
>
> There are at least 2 other uses of ps_pptr->ps_pid that should also change,
> unless you like coredumps and ps disagreeing with getppid(), and someone
> needs to think how it affects doas.
>
Thanks for pointing. I missed these two places. However, doas(1) was not
affected because this diff doesn’t modify tty(4) behaviour:
TIOC{GET,SET}VERAUTH still use ps_pptr->ps_pid.
I checked other BSD’s. NetBSD, DragonflyBSD and OSX have the same
behaviour of getppid(2). And this behaviour don’t contradict POSIX.1 [1]. So
I guess Philip is right, there is no reason to follow this way.
1. https://pubs.opengroup.org/onlinepubs/9699919799/functions/getppid.html
