Re: incorrect result from getppid for ptraced processes
On 9/5/20, Philip Guenther wrote:
> On Fri, Sep 4, 2020 at 2:59 PM Mateusz Guzik wrote:
>
>> On 9/5/20, Philip Guenther wrote:
>> > On Fri, Sep 4, 2020 at 1:06 PM Mateusz Guzik wrote:
>> >
>> >> On 9/4/20, Vitaliy Makkoveev 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 - 1.76
>> >> > +++ sys/kern/kern_prot.c 4 Sep 2020 21:12:15 -
>> >> > @@ -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?
>
Nothing has changed. I don't use the system apart from sometimes
testing stuff in a VM. There is 0 pressure on my end to fix this.
>
> 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.
>
I see, I did not grep for these.
--
Mateusz Guzik
Re: incorrect result from getppid for ptraced processes
> On 5 Sep 2020, at 03:22, Philip Guenther wrote:
>
> On Fri, Sep 4, 2020 at 2:59 PM Mateusz Guzik wrote:
>
>> On 9/5/20, Philip Guenther wrote:
>>> On Fri, Sep 4, 2020 at 1:06 PM Mateusz Guzik wrote:
>>>
On 9/4/20, Vitaliy Makkoveev 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 - 1.76
> +++ sys/kern/kern_prot.c 4 Sep 2020 21:12:15 -
> @@ -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
Re: incorrect result from getppid for ptraced processes
On Fri, Sep 4, 2020 at 2:59 PM Mateusz Guzik wrote:
> On 9/5/20, Philip Guenther wrote:
> > On Fri, Sep 4, 2020 at 1:06 PM Mateusz Guzik wrote:
> >
> >> On 9/4/20, Vitaliy Makkoveev 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 - 1.76
> >> > +++ sys/kern/kern_prot.c 4 Sep 2020 21:12:15 -
> >> > @@ -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.
Philip Guenther
Re: incorrect result from getppid for ptraced processes
On 9/5/20, Philip Guenther wrote:
> On Fri, Sep 4, 2020 at 1:06 PM Mateusz Guzik wrote:
>
>> On 9/4/20, Vitaliy Makkoveev 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 - 1.76
>> > +++ sys/kern/kern_prot.c 4 Sep 2020 21:12:15 -
>> > @@ -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 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.
--
Mateusz Guzik
Re: incorrect result from getppid for ptraced processes
On Fri, Sep 4, 2020 at 1:06 PM Mateusz Guzik wrote:
> On 9/4/20, Vitaliy Makkoveev 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 - 1.76
> > +++ sys/kern/kern_prot.c 4 Sep 2020 21:12:15 -
> > @@ -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?
Philip Guenther
Re: incorrect result from getppid for ptraced processes
On 9/4/20, Vitaliy Makkoveev 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 - 1.76
> +++ sys/kern/kern_prot.c 4 Sep 2020 21:12:15 -
> @@ -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.
--
Mateusz Guzik
Re: incorrect result from getppid for ptraced processes
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.c9 Jul 2019 12:23:25 - 1.76
+++ sys/kern/kern_prot.c4 Sep 2020 21:12:15 -
@@ -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);
}
incorrect result from getppid for ptraced processes
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. -- Mateusz Guzik
