[Toybox] [PATCH] Fix pgrep -s 0 when running in session ID 0

[Colin Cross]

On Sat, Oct 7, 2023 at 11:47 PM Rob Landley <rob at landley.net> wrote:
>
> On 10/7/23 21:47, Colin Cross wrote:
> > On Fri, Oct 6, 2023 at 9:29 PM Rob Landley <rob at landley.net> wrote:
> >>
> >> On 10/6/23 17:10, Colin Cross via Toybox wrote:
> >> > I came across an issue when running the pgrep -s 0 test in Android's
> >> > CI infrastructure that uses a PID namespace, causing the test to run
> >> > session ID 0:
> >> >
> >> > $ sudo unshare -fp ./toybox pgrep -s 0
> >> > pgrep: bad -s '0'
> >> >
> >> > The attached patch fixes the argument parsing to support getsid returning 0.
> >>
> >> Ah, I read "man 2 getsid" to indicate that sid 0 was special, but it's passing
> >> in _pid_ 0 that's special. Which is why, right before your patch, this part
> >> renders what you did moot:
> >>
> >>     // For pkill, -s 0 represents pkill's session id.
> >>     if (pl==&TT.ss && ll[pl->len]==0) ll[pl->len] = getsid(0);
> >>
> >> Ah, no wait, that _is_ in the pkill man page:
> >>
> >>        -s, --session sid,...
> >>               Only match processes whose process session ID is  listed.   Ses‐
> >>               sion ID 0 is translated into pgrep's or pkill's own session ID.
> >>
> >> ...so you want pgrep to accept -s 0, and it'll it will coincidentally work for
> >> pkill if your session ID _is_ 0 then getsid() will return 0 anyway? And pkill
> >> should work differently from pgrep?
> >
> > No, pkill and pgrep both already work with -s 0 as long as the current
> > process is not in session ID 0.
>
> By coincidence.
>
> > When passed -s 0, they replace the
> > parsed value (0) with the result of getsid(0).  It then falls through
> > to the check that the parsed value (now the getsid value) is greater
> > than 0, but that's incorrect because 0 is a valid return value from
> > getsid.
>
> I think it's actually a kernel bug that the session ID can be 0. The setsid()
> call takes no arguments because it sets the new session ID to equal to the PID
> of the calling process, and no running process can have a PID of 0 (that's the
> idle task).
>
> If you call getsid(1) it returns 1, because the init task is in its own session.
> But the kernel is missing some container initialization, and is thus leaving the
> session ID uninitialized (zeroed) which means the session belongs to the idle
> task. (Either that or it's the PID namespace stuff returning 0 somewhere it
> should return 1.)
>
> We can fix userspace to work with this kernel bug, but it IS a kernel bug.
>
> > There's no way to limit pgrep or pkill to the processes in session 0
> > unless it is itself running in session 0, but that's a general problem
> > with pgrep and pkill, and not specific to toybox.
>
> It's not a problem with either one of those, it's a problem with the kernel's
> container support. Unless the linux-kernel clique is going to "that's a
> volkswagen feature" it...

It turns out this isn't a kernel bug, it's just an unfortunate
ramification of PID namespaces.  It's hinted at in pid_namespaces(7):
       Some processes in a PID namespace may have parents that are
outside of the namespace.  For
       example, the parent of the initial process in the namespace
(i.e.,  the  init(1)  process
       with  PID  1)  is  necessarily  in  another namespace.
Likewise, the direct children of a
       process that uses setns(2) to cause its children to join a PID
namespace are in a  differ‐
       ent PID namespace from the caller of setns(2).  Calls to
getppid(2) for such processes re‐
       turn 0.

In this case, the session id of the process is a pid outside the
namespace.  getsid(0) can't return that pid, so it returns 0 instead.
You can see this with:
$ sudo unshare -fp cat /proc/self/status | grep NS
NStgid: 3435791 1
NSpid: 3435791 1
NSpgid: 3435788 0
NSsid: 3380005 0

The first column is the pids in the root namespace, the second is the
pids in the new namespace.  The process group and session id are both
0 inside the namespace.

A container that calls setsid gets more reasonable values:
$ sudo unshare -fp setsid cat /proc/self/status | grep NS
NStgid: 3529299 1
NSpid: 3529299 1
NSpgid: 3529299 1
NSsid: 3529299 1