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

[Rob Landley]

On 10/8/23 11:12, Colin Cross wrote:
> 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.

I just checked what running an actual init task gave for getsid() and that also
produces 0.

Did commit 94913b5d0c56 address it? (Awkward to test here...)

Rob