Re: suspected bug in timeout command

[Steffen Nurpmeso]

Raffaello D. Di Napoli wrote in
 <3b0697d2-01b6-d36b-10da-32d8cf917...@dinapo.li>:
 |On 3/2/22 03:50, Michael Conrad wrote:
 |> On 3/2/22 02:45, Raffaello D. Di Napoli wrote:
 |>> On 3/1/22 16:57, Denys Vlasenko wrote:
 |>>> On Tue, Mar 1, 2022 at 5:39 PM Denys Vlasenko 
 |>>>  wrote:
 ...
 | Let's go with a solution with fd opened to /proc/PID?
 |>>
 |>> I’d think simplifying the implementation and bringing it closer to 
 |>> coreutils’ would be more in line with BB’s goals, instead of making 
 ...
 |> It might be worth mentioning that busybox can't conform to coreutils 
 |> unless it does remain the parent process, because of this detail: 
 |> (from coreutils' timeout man page)
 |>
 |>> If the command times out, and --preserve-status is not set, then
 |>> exit with status 124.  Otherwise, exit with the status of COMMAND.
 |
 |Well, yes. I already pointed out in an earlier message I see a rewrite 
 |as unavoidable, to really fix the reported issue. By “simplifying the 

I just want to point out that FreeBSD has a small timeout
implementation, with busybox environment (as i know it, argv parse
etc) it could be even smaller.

 |implementation” now I meant redoing it so that it aligns not only with 
 |coreutils, but also with what _every single person who’s looked at it in 
 |this thread_ expected, i.e. the conventional parent/child relationship 
 |rather than a reverse grandchild/parent as it is today.

Maybe then adding PR_SET_CHILD_SUBREAPER prctl(2) (already used in
busybox) would be a suggestion.

  ...

--steffen
|
|Der Kragenbaer,The moon bear,
|der holt sich munter   he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Michael Conrad]

On 3/2/22 02:45, Raffaello D. Di Napoli wrote:

On 3/1/22 16:57, Denys Vlasenko wrote:
On Tue, Mar 1, 2022 at 5:39 PM Denys Vlasenko 
 wrote:

Meanwhile: what "timeout" is doing is it tries to get out
of the way of the PROG to be launched so that timeout's parent
sees PROG (not timeout) as a child. E.g. it can send signals
to it, get waitpid notifications if PROG has been stopped
with a signal, and such.

And PROG also has no spurious "timeout" child.
"timeout" exists as an orphaned granchild.


That doesn’t seem to be a concern for coreutils, according to Rob’s 
inspection. (I haven’t looked, but I’ll assume they still do signal 
forwarding and everything that can be done cheaply.) Isn’t it a goal 
of BB to avoid unnecessary divergence from coreutils?




Let's go with a solution with fd opened to /proc/PID?


I’d think simplifying the implementation and bringing it closer to 
coreutils’ would be more in line with BB’s goals, instead of making it 
larger and more complicated (especially considering how 
counter-intuitive it is already, despite being fairly small).



It might be worth mentioning that busybox can't conform to coreutils 
unless it does remain the parent process, because of this detail: (from 
coreutils' timeout man page)


> If the command times out, and --preserve-status is not set, then
> exit with status 124.  Otherwise, exit with the status of COMMAND.

timeout doesn't appear to be part of POSIX, though.

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Rob Landley]

On 3/1/22 4:15 PM, David Laight wrote:
> From: Denys Vlasenko
>> Sent: 01 March 2022 21:57
>> 
>> On Tue, Mar 1, 2022 at 5:39 PM Denys Vlasenko  
>> wrote:
>> > Meanwhile: what "timeout" is doing is it tries to get out
>> > of the way of the PROG to be launched so that timeout's parent
>> > sees PROG (not timeout) as a child. E.g. it can send signals
>> > to it, get waitpid notifications if PROG has been stopped
>> > with a signal, and such.
>> >
>> > And PROG also has no spurious "timeout" child.
>> > "timeout" exists as an orphaned granchild.
>> >
>> > Let's go with a solution with fd opened to /proc/PID?
>> 
>> This little test:
>> 
>> int fd = open("/proc/self", O_RDONLY);
>> int parent = getpid();
>> int pid = fork();
>> if (pid) { //parent
>> sleep(1);
>> exit(0);
>> }
>> sleep(2);
>> printf("openat:%d\n", openat(fd, ".", O_RDONLY));
>> while (openat(fd, ".", O_RDONLY) == -1)
>> continue;
>> 
>> and a separate "spawn 40k 'sleep 0.03'" loop
>> seems to indicate that openat(fd, ".") on the exited
>> /proc/PID fails, and continues to fail
>> even if another process with this PID exists
>> again (pid was reused).
> 
> Which kernel?
> That may not fail on pre-pidfd kernels.

Nah, nothing to do with that. If you have a file descriptor open to anything
under /proc/$PID then the reference count on the corresponding task struct can't
go to zero so the bit doesn't get cleared in the table. The dirfd still being
open pins it the same way a filehandle to a deleted file pins the inode...

Last I checked (many moons ago) there was a bitmask of active processes that
could be quickly checked for next empty bit and updated with lockless
compare-and-swap assembly magic (hence the O(1) part). That's why max_pid
defaults to 4096*8, it's one page. (I followed the O(1) rewrites when they went
in, and some of the early container work, but that was 10 years ago now...)

I still think it's silly to spend extra effort to make busybox timeout work
differently than anybody else's timeout, but it's Denys' codebase...

Rob
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Rob Landley]

On 3/1/22 1:45 PM, Denys Vlasenko wrote:
> On Tue, Mar 1, 2022 at 6:52 PM Steffen Nurpmeso  wrote:
>> David Laight wrote in
>>  :
>>  |From: Denys Vlasenko
>>  |> Sent: 01 March 2022 16:40
>>  |> On Tue, Feb 15, 2022 at 12:31 PM Rob Landley  wrote:
>>  |>> On 2/14/22 10:09 AM, Roberto A. Foglietta wrote:
>>  ...
>>  |> My memory is hazy on this, but IIRC kernel also actually has some
>>  |> defensive code to not immediately reuse pids which just died.
>>  |
>>  |The Linux krnel only has protection for code inside the kernel.
>>  |Basically there is a ref-counted structure that you need to send the
>>  |signal - not the pid itself.
>>  |I can't quite remember whether the pid itself can be reused even before
>>  |that structure is freed.
>>  |
>>  |NetBSD does guarantee not to reuse a pid for a reasonable number
>>  |of forks after a process exits.
>>
>> ...which might be fruitless with 16-bit pids, define "reasonable".
>> Matt Dillon of DragonFly BSD (crond etc.) made, after implementing
>> some DBSD kernel optimizations (iirc), tests with statically
>> linked programs and... quoting myself
>>
>>   i remember Matthew Dillon's post on DragonFly BSD users@[1], where
>>   he claims 45 execs per second for a statically linked binary,
>>   and about 45000 execs per second for a dynamic one, with DragonFly
>>   5.6 on a threadripper.
> 
> What's relevant is how many fork's you can do per second. Not execs.

Back when Ingo Molnar was cleaning up the thread spawn/exit code in the kernel I
vaguely remember he benchmarked his last optimization pass at 2 million/second
on some big SMP system, but that was quite a while ago (I want to say 2004?) It
was the eventual follow-up to this line of optimization work:

https://lwn.net/Articles/8131/

Of course all those did was call clone() and immediately exit(), and it was
threads not processes so avoided most of the VM setup work and copying the file
descriptor table and so on. (And was before the current namespace stuff.)

Rob
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Steffen Nurpmeso]

David Laight wrote in
 <3c1a02d162c34e908af85ac008a87...@acums.aculab.com>:
 |From: Steffen Nurpmeso
 |> Sent: 01 March 2022 17:49
 |> David Laight wrote in
 |>  :
 |>|From: Denys Vlasenko
 |>|> Sent: 01 March 2022 16:40
 |>|> On Tue, Feb 15, 2022 at 12:31 PM Rob Landley  wrote:
 |>|>> On 2/14/22 10:09 AM, Roberto A. Foglietta wrote:
 |>  ...
 |>|> My memory is hazy on this, but IIRC kernel also actually has some
 |>|> defensive code to not immediately reuse pids which just died.
 |>|
 |>|The Linux krnel only has protection for code inside the kernel.
 |>|Basically there is a ref-counted structure that you need to send the
 |>|signal - not the pid itself.
 |>|I can't quite remember whether the pid itself can be reused even before
 |>|that structure is freed.
 |>|
 |>|NetBSD does guarantee not to reuse a pid for a reasonable number
 |>|of forks after a process exits.
 |> 
 |> ...which might be fruitless with 16-bit pids, define "reasonable".
 |
 |Nothing stops pids going beyond 16 bits.
 |(Apart from some of the very old emulations.)
 |Much like nothing really requires the pid allocator to even start
 |allocating linearly (after giving init pid 1) - but that is
 |rather expected.
 |
 |I can't remember at which point it went above 32767.
 |Was over 20 years ago when I wrote it :-)

Ah, kernel.  Not my league then :-)

(I knew the (Linux) sysctl/proc entry, but it is unchanged here.)

--steffen
|
|Der Kragenbaer,The moon bear,
|der holt sich munter   he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Steffen Nurpmeso]

Denys Vlasenko wrote in
 :
 |On Tue, Mar 1, 2022 at 6:52 PM Steffen Nurpmeso  wrote:
 |> David Laight wrote in
 |>  :
 |>|From: Denys Vlasenko
 |>|> Sent: 01 March 2022 16:40
 |>|> On Tue, Feb 15, 2022 at 12:31 PM Rob Landley  wrote:
 |>|>> On 2/14/22 10:09 AM, Roberto A. Foglietta wrote:
 |>  ...
 |>|> My memory is hazy on this, but IIRC kernel also actually has some
 |>|> defensive code to not immediately reuse pids which just died.
 |>|
 |>|The Linux krnel only has protection for code inside the kernel.
 |>|Basically there is a ref-counted structure that you need to send the
 |>|signal - not the pid itself.
 |>|I can't quite remember whether the pid itself can be reused even before
 |>|that structure is freed.
 |>|
 |>|NetBSD does guarantee not to reuse a pid for a reasonable number
 |>|of forks after a process exits.
 |>
 |> ...which might be fruitless with 16-bit pids, define "reasonable".
 |> Matt Dillon of DragonFly BSD (crond etc.) made, after implementing
 |> some DBSD kernel optimizations (iirc), tests with statically
 |> linked programs and... quoting myself
 |>
 |>   i remember Matthew Dillon's post on DragonFly BSD users@[1], where
 |>   he claims 45 execs per second for a statically linked binary,
 |>   and about 45000 execs per second for a dynamic one, with DragonFly
 |>   5.6 on a threadripper.
 |
 |What's relevant is how many fork's you can do per second. Not execs.

mumble

--steffen
|
|Der Kragenbaer,The moon bear,
|der holt sich munter   he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

RE: suspected bug in timeout command

[David Laight]

From: Denys Vlasenko
> Sent: 01 March 2022 22:40
> 
> On Tue, Mar 1, 2022 at 11:15 PM David Laight  wrote:
> > From: Denys Vlasenko
> > > and a separate "spawn 40k 'sleep 0.03'" loop
> > > seems to indicate that openat(fd, ".") on the exited
> > > /proc/PID fails, and continues to fail
> > > even if another process with this PID exists
> > > again (pid was reused).
> >
> > Which kernel?
> > That may not fail on pre-pidfd kernels.
> > There were some discussions on the linux kernel mailing list
> > that I half followed.
> 
> 4.12.0-0.rc3.git0.2.fc27.x86_64

Reasonably old then.
I could check on a 3.10 RHEL7 kernel.

But any busybox fix is likely to run on a newish kernel.
Not some old distro kernel.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, 
UK
Registration No: 1397386 (Wales)
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Denys Vlasenko]

On Tue, Mar 1, 2022 at 11:15 PM David Laight  wrote:
> From: Denys Vlasenko
> > and a separate "spawn 40k 'sleep 0.03'" loop
> > seems to indicate that openat(fd, ".") on the exited
> > /proc/PID fails, and continues to fail
> > even if another process with this PID exists
> > again (pid was reused).
>
> Which kernel?
> That may not fail on pre-pidfd kernels.
> There were some discussions on the linux kernel mailing list
> that I half followed.

4.12.0-0.rc3.git0.2.fc27.x86_64
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

RE: suspected bug in timeout command

[David Laight]

From: Steffen Nurpmeso
> Sent: 01 March 2022 17:49
...
> 
> David Laight wrote in
>  :
>  |From: Denys Vlasenko
>  |> Sent: 01 March 2022 16:40
>  |> On Tue, Feb 15, 2022 at 12:31 PM Rob Landley  wrote:
>  |>> On 2/14/22 10:09 AM, Roberto A. Foglietta wrote:
>  ...
>  |> My memory is hazy on this, but IIRC kernel also actually has some
>  |> defensive code to not immediately reuse pids which just died.
>  |
>  |The Linux krnel only has protection for code inside the kernel.
>  |Basically there is a ref-counted structure that you need to send the
>  |signal - not the pid itself.
>  |I can't quite remember whether the pid itself can be reused even before
>  |that structure is freed.
>  |
>  |NetBSD does guarantee not to reuse a pid for a reasonable number
>  |of forks after a process exits.
> 
> ...which might be fruitless with 16-bit pids, define "reasonable".

Nothing stops pids going beyond 16 bits.
(Apart from some of the very old emulations.)
Much like nothing really requires the pid allocator to even start
allocating linearly (after giving init pid 1) - but that is
rather expected.

I can't remember at which point it went above 32767.
Was over 20 years ago when I wrote it :-)

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, 
UK
Registration No: 1397386 (Wales)

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

RE: suspected bug in timeout command

[David Laight]

From: Denys Vlasenko
> Sent: 01 March 2022 21:57
> 
> On Tue, Mar 1, 2022 at 5:39 PM Denys Vlasenko  
> wrote:
> > Meanwhile: what "timeout" is doing is it tries to get out
> > of the way of the PROG to be launched so that timeout's parent
> > sees PROG (not timeout) as a child. E.g. it can send signals
> > to it, get waitpid notifications if PROG has been stopped
> > with a signal, and such.
> >
> > And PROG also has no spurious "timeout" child.
> > "timeout" exists as an orphaned granchild.
> >
> > Let's go with a solution with fd opened to /proc/PID?
> 
> This little test:
> 
> int fd = open("/proc/self", O_RDONLY);
> int parent = getpid();
> int pid = fork();
> if (pid) { //parent
> sleep(1);
> exit(0);
> }
> sleep(2);
> printf("openat:%d\n", openat(fd, ".", O_RDONLY));
> while (openat(fd, ".", O_RDONLY) == -1)
> continue;
> 
> and a separate "spawn 40k 'sleep 0.03'" loop
> seems to indicate that openat(fd, ".") on the exited
> /proc/PID fails, and continues to fail
> even if another process with this PID exists
> again (pid was reused).

Which kernel?
That may not fail on pre-pidfd kernels.
There were some discussions on the linux kernel mailing list
that I half followed.

But it is probably a lot better that anything else at least
for the current kernels that people are probably going to use.

(Apart from those of us who have to build release binaries
on old systems in order that customers can run them on the
old distros they like to use)

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, 
UK
Registration No: 1397386 (Wales)

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Denys Vlasenko]

On Tue, Mar 1, 2022 at 5:39 PM Denys Vlasenko  wrote:
> Meanwhile: what "timeout" is doing is it tries to get out
> of the way of the PROG to be launched so that timeout's parent
> sees PROG (not timeout) as a child. E.g. it can send signals
> to it, get waitpid notifications if PROG has been stopped
> with a signal, and such.
>
> And PROG also has no spurious "timeout" child.
> "timeout" exists as an orphaned granchild.
>
> Let's go with a solution with fd opened to /proc/PID?

This little test:

int fd = open("/proc/self", O_RDONLY);
int parent = getpid();
int pid = fork();
if (pid) { //parent
sleep(1);
exit(0);
}
sleep(2);
printf("openat:%d\n", openat(fd, ".", O_RDONLY));
while (openat(fd, ".", O_RDONLY) == -1)
continue;

and a separate "spawn 40k 'sleep 0.03'" loop
seems to indicate that openat(fd, ".") on the exited
/proc/PID fails, and continues to fail
even if another process with this PID exists
again (pid was reused).

This would make it impossible that this
"does process exist?" test hooks on a different
process with the same pid:

while (1) {
sleep(1);
if (--timeout <= 0)
break;
-   if (kill(pid, 0)) {
+   tfd = openat(fd, ".", O_RDONLY);
+   if (tfd == -1) {
/* process is gone */
return EXIT_SUCCESS;
}
+   close(tfd);
}

It would still be possible for the watched process to exit
right before "timeout" decided to SIGTERM it,
another process to fork and reuse this pid, and get
this SIGTERM instead.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Denys Vlasenko]

On Tue, Mar 1, 2022 at 6:52 PM Steffen Nurpmeso  wrote:
> David Laight wrote in
>  :
>  |From: Denys Vlasenko
>  |> Sent: 01 March 2022 16:40
>  |> On Tue, Feb 15, 2022 at 12:31 PM Rob Landley  wrote:
>  |>> On 2/14/22 10:09 AM, Roberto A. Foglietta wrote:
>  ...
>  |> My memory is hazy on this, but IIRC kernel also actually has some
>  |> defensive code to not immediately reuse pids which just died.
>  |
>  |The Linux krnel only has protection for code inside the kernel.
>  |Basically there is a ref-counted structure that you need to send the
>  |signal - not the pid itself.
>  |I can't quite remember whether the pid itself can be reused even before
>  |that structure is freed.
>  |
>  |NetBSD does guarantee not to reuse a pid for a reasonable number
>  |of forks after a process exits.
>
> ...which might be fruitless with 16-bit pids, define "reasonable".
> Matt Dillon of DragonFly BSD (crond etc.) made, after implementing
> some DBSD kernel optimizations (iirc), tests with statically
> linked programs and... quoting myself
>
>   i remember Matthew Dillon's post on DragonFly BSD users@[1], where
>   he claims 45 execs per second for a statically linked binary,
>   and about 45000 execs per second for a dynamic one, with DragonFly
>   5.6 on a threadripper.

What's relevant is how many fork's you can do per second. Not execs.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Rob Landley]

On 3/1/22 10:39 AM, Denys Vlasenko wrote:
> On Tue, Feb 15, 2022 at 12:31 PM Rob Landley  wrote:
>> On 2/14/22 10:09 AM, Roberto A. Foglietta wrote:
>> >  However, if this bug shows-up, probably it means that the system has
>> > a lot of processes running and a lot of processes created and
>> > destroyed compared to the max PID available. Thus, the system might be
>> > incorrectly configured compared with its typical usage which probably
>> > is the main reason because nobody complained before.
>>
>> Nah, a shell script can spin through an awful lot of PIDs pretty fast, and if
>> you're doing a -j 8 build that has a lot of script snippets (let alone 
>> parallel
>> autoconf etc) vs something with say a 10 second timeout?
> 
> I try the below and it seems to be able to spawn "only"
> ~1500 processes/second.

Let's see, off the top of my head...

1) make -j 8

2) Not all wrapper variants ala system() and ccache and friends will exec()
without a fork().

3) the default /proc/sys/kernel/pid_max is only 32768, 10 second timeout would
see a wrap at 1.5k/sec.

4) my laptop has about 500 persistent processes running right now and isn't
really busy: the system can only recycle the free ones.

5) Containers tend to have a small PID budget since each container PID is mapped
to a host PID (https://kubernetes.io/docs/concepts/policy/pid-limiting/)...

6) Some of those short-lived processes may be threaded, does TID count as using
a PID? (I honestly don't know how that maps these days: cat
/proc/sys/kernel/threads-max is 127327 WHILE pid_max is 32768?)

> $ time sh -c 'i=3; while test $((--i)) != 0; do sleep 0 & done 
> 2>/dev/null'
> real0m19.190s
> user0m23.062s
> sys0m6.732s
> 
> My memory is hazy on this, but IIRC kernel also actually has some
> defensive code to not immediately reuse pids which just died.

Other than zombie processes? Isn't it just LRU cycling through them in order? It
won't reuse a PID that's still active (open filehandle and such) but once it's
gone it forgets about it.

It's hard to tell what kernel/pid.c is doing these days with all the namespace
stuff in there, but the idr_alloc_cyclic() call just takes pid_min and pid_max
as arguments...?

Are you thinking of SO_REUSEADDR perhaps?

> It's interesting to find out why pids are reused that fast on the
> affected system.

The list above is not remotely exhaustive, it's just what came to mind. I have
no idea what horrific combination of systemd and ruby might decide to spawn a
process to handle every dbus event or run an RPC service via inetd, but I try
not to assume it won't happen either. (I never know what ELSE the system is
doing...)

> Meanwhile: what "timeout" is doing is it tries to get out
> of the way of the PROG to be launched so that timeout's parent
> sees PROG (not timeout) as a child. E.g. it can send signals
> to it, get waitpid notifications if PROG has been stopped
> with a signal, and such.

Is that how any other timeout works? On devuan:

$ timeout 100 /bin/bash
$ ps $PPID
  PID TTY  STAT   TIME COMMAND
28825 pts/19   S  0:00 timeout 100 /bin/bash

Or for that matter:

$ sudo /bin/bash
# ps $PPID
  PID TTY  STAT   TIME COMMAND
28690 pts/19   S  0:00 sudo /bin/bash

A lot of commands like env/nohup/chroot/linux64 exec their arguments, but that's
because they do a thing up front to modify the process context and are then
done. There's no "env process" lying around monitoring the child because there's
nothing for it to do. When there IS something left to do:

$ strace /bin/bash 2>/dev/null
ps $PPID
  PID TTY  STAT   TIME COMMAND
29428 pts/19   S+ 0:00 strace /bin/bash

It's generally gonna stay the parent because the parent/child process
relationship with zombie sigchild delivery is the traditional unix mechanism to
avoids PID reuse races.

> And PROG also has no spurious "timeout" child.
> "timeout" exists as an orphaned granchild.
> 
> Let's go with a solution with fd opened to /proc/PID?

*shrug* I leave you to it. Not what I did:

https://github.com/landley/toybox/blob/master/toys/other/timeout.c

Let's hold our nose and see what coreutils did...

https://github.com/coreutils/coreutils/blob/master/src/timeout.c

Looks like fork(), execvp(), and waitpid(). Standard parent/child relationship.

Rob
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Steffen Nurpmeso]

Hello.

..despite that particular timeout(1), i have not looked..

David Laight wrote in
 :
 |From: Denys Vlasenko
 |> Sent: 01 March 2022 16:40
 |> On Tue, Feb 15, 2022 at 12:31 PM Rob Landley  wrote:
 |>> On 2/14/22 10:09 AM, Roberto A. Foglietta wrote:
 ...
 |> My memory is hazy on this, but IIRC kernel also actually has some
 |> defensive code to not immediately reuse pids which just died.
 |
 |The Linux krnel only has protection for code inside the kernel.
 |Basically there is a ref-counted structure that you need to send the
 |signal - not the pid itself.
 |I can't quite remember whether the pid itself can be reused even before
 |that structure is freed.
 |
 |NetBSD does guarantee not to reuse a pid for a reasonable number
 |of forks after a process exits.

...which might be fruitless with 16-bit pids, define "reasonable".
Matt Dillon of DragonFly BSD (crond etc.) made, after implementing
some DBSD kernel optimizations (iirc), tests with statically
linked programs and... quoting myself

  i remember Matthew Dillon's post on DragonFly BSD users@[1], where
  he claims 45 execs per second for a statically linked binary,
  and about 45000 execs per second for a dynamic one, with DragonFly
  5.6 on a threadripper.

[1] https://marc.info/?l=dragonfly-users=155846667020624=2

(Hope that is still valid.)
It think it is a problem with shells, i once stumbled over this
with a particular (now completely rewritten) of the test suite of
the mailer i maintain:

  My guess would have been that if i kill(1) a job specification,
  then the sh(1)ell would refuse to use its builtin kill(1) to kill
  a PID that was saved away from ${!}, and the process in question
  has already terminated.  Of course using job specifications is
  impossible here, let alone portably (though that i have not even
  tried, because set -m results in a lot of unwanted noise).

--steffen
|
|Der Kragenbaer,The moon bear,
|der holt sich munter   he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

RE: suspected bug in timeout command

[David Laight]

From: Denys Vlasenko
> Sent: 01 March 2022 16:40
> 
> On Tue, Feb 15, 2022 at 12:31 PM Rob Landley  wrote:
> > On 2/14/22 10:09 AM, Roberto A. Foglietta wrote:
> > >  However, if this bug shows-up, probably it means that the system has
> > > a lot of processes running and a lot of processes created and
> > > destroyed compared to the max PID available. Thus, the system might be
> > > incorrectly configured compared with its typical usage which probably
> > > is the main reason because nobody complained before.
> >
> > Nah, a shell script can spin through an awful lot of PIDs pretty fast, and 
> > if
> > you're doing a -j 8 build that has a lot of script snippets (let alone 
> > parallel
> > autoconf etc) vs something with say a 10 second timeout?
> 
> I try the below and it seems to be able to spawn "only"
> ~1500 processes/second.
> 
> $ time sh -c 'i=3; while test $((--i)) != 0; do sleep 0 & done 
> 2>/dev/null'
> real0m19.190s
> user0m23.062s
> sys0m6.732s
> 
> My memory is hazy on this, but IIRC kernel also actually has some
> defensive code to not immediately reuse pids which just died.

The Linux krnel only has protection for code inside the kernel.
Basically there is a ref-counted structure that you need to send the
signal - not the pid itself.
I can't quite remember whether the pid itself can be reused even before
that structure is freed.

NetBSD does guarantee not to reuse a pid for a reasonable number
of forks after a process exits.

> It's interesting to find out why pids are reused that fast on the
> affected system.

They are allocated by a simple numeric scan for a free pid.
So it depends on where the scan is and the pid being freed.
It can be reused for the very next fork().

> Meanwhile: what "timeout" is doing is it tries to get out
> of the way of the PROG to be launched so that timeout's parent
> sees PROG (not timeout) as a child. E.g. it can send signals
> to it, get waitpid notifications if PROG has been stopped
> with a signal, and such.

I also believe it is only checking the pid every (few?) seconds.

> And PROG also has no spurious "timeout" child.
> "timeout" exists as an orphaned granchild.
> 
> Let's go with a solution with fd opened to /proc/PID?

I think you need to verify some part of the process state.
Especially for pre-pidfd kernels.
Probably the process start time.
If that changes the pid has been reused.

That gets the timing window down to 'we checked it was the
right process', but the pid was reused before we could send
the signal.
It also requires the process to exit on exactly its timeout.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, 
UK
Registration No: 1397386 (Wales)

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Denys Vlasenko]

On Tue, Feb 15, 2022 at 12:31 PM Rob Landley  wrote:
> On 2/14/22 10:09 AM, Roberto A. Foglietta wrote:
> >  However, if this bug shows-up, probably it means that the system has
> > a lot of processes running and a lot of processes created and
> > destroyed compared to the max PID available. Thus, the system might be
> > incorrectly configured compared with its typical usage which probably
> > is the main reason because nobody complained before.
>
> Nah, a shell script can spin through an awful lot of PIDs pretty fast, and if
> you're doing a -j 8 build that has a lot of script snippets (let alone 
> parallel
> autoconf etc) vs something with say a 10 second timeout?

I try the below and it seems to be able to spawn "only"
~1500 processes/second.

$ time sh -c 'i=3; while test $((--i)) != 0; do sleep 0 & done 2>/dev/null'
real0m19.190s
user0m23.062s
sys0m6.732s

My memory is hazy on this, but IIRC kernel also actually has some
defensive code to not immediately reuse pids which just died.

It's interesting to find out why pids are reused that fast on the
affected system.

Meanwhile: what "timeout" is doing is it tries to get out
of the way of the PROG to be launched so that timeout's parent
sees PROG (not timeout) as a child. E.g. it can send signals
to it, get waitpid notifications if PROG has been stopped
with a signal, and such.

And PROG also has no spurious "timeout" child.
"timeout" exists as an orphaned granchild.

Let's go with a solution with fd opened to /proc/PID?
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Rob Landley]

On 2/14/22 10:09 AM, Roberto A. Foglietta wrote:
>  However, if this bug shows-up, probably it means that the system has
> a lot of processes running and a lot of processes created and
> destroyed compared to the max PID available. Thus, the system might be
> incorrectly configured compared with its typical usage which probably
> is the main reason because nobody complained before.

Nah, a shell script can spin through an awful lot of PIDs pretty fast, and if
you're doing a -j 8 build that has a lot of script snippets (let alone parallel
autoconf etc) vs something with say a 10 second timeout?

Entirely plausible, and not a sign of a system being misconfigured. PIDs get
reused, fact of life, unix was already designed to cope back in the 1970s.

Rob
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Roberto A. Foglietta]

Il Sab 12 Feb 2022, 14:13 David Laight  ha scritto:

> From: Michael Conrad
> > Sent: 12 February 2022 12:59
> >
> > On 2/12/22 07:38, Michael Conrad wrote:
> > > Correctly using pidfd *still* requires that you be the parent process,
> > > else the child could get reaped and replaced before the pidfd is
> > > created.  As far as I can tell, the only purpose of pidfd is for
> > > waking on poll() instead of using signals, which is orthagonal to this
> > > problem.
>
> Even if the pidfd can't be passed down, it lets you verify the process
> information and then send the signal.
>
> > > I haven't looked at the source in busybox yet, but it boggles my mind
> > > that it wouldn't just be a simple fork+alarm+waitpid because that is
> > > literally the least code implementation, and race-free.
> > >
> > > -Mike C
> > >
> > Sorry for being lazy.  I looked at the source and this is the reason:
> >
> > /* We want to create a grandchild which will watch
> >   * and kill the grandparent. Other methods:
> >   * making parent watch child disrupts parent<->child link
> >   * (example: "tcpsvd 0.0.0.0 1234 timeout service_prog" -
> >   * it's better if service_prog is a child of tcpsvd!),
> >   * making child watch parent results in programs having
> >   * unexpected children.*/
> >
> > I don't follow this reasoning.  Does "disrupts the parent<->child link"
> > just about sending signals?  If the timeout app relays all signals from
> > itself to the child, what remaining problems would exist?
>
> In that case you can pass 'verification information' through to
> the grandchild.
>
> It could be an open fd of "/proc/self" - which allows the non-racy
> kill on recent kernels.
> But other information would allow the timing window be minimalised on
> older kernels.
>
> ISTR that on older kernels an open fd to "proc/nn" always refers to the
> current process with pid nn. But the actual behaviour is worth checking.
> I think newer kernels will fail any reads after the process has exited.
>

Checking the procfs man page there is some mount options that do not
allows reading other PID files in that filesystem.

I am not 100% sure that a invitered file descriptor could not be read
but negate access.

Mount options The proc filesystem supports the following mount
options: hidepid=n (since Linux 3.3) This option controls who can
access the information in /proc/[pid] directories. The argument, n, is
one of the following values:
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

RE: suspected bug in timeout command

[David Laight]

Yes it does.
But it is unlikely to happen in the short time between some kind of test
that the process is the right one, and sending the signal.
But hoping to see the process ‘gone’ on a 1 second poll is pretty useless.

So if the process’s ‘start time’ is found (before any of the forks) that can
be passed through to the grandchild and checked every second.
Then you are very unlikely to kill the wrong process.

If you also pass through the open directory fd to “/proc/self” and use openat()
to open whichever /proc/xx/yy file is needed then I think with a later kernel
you’ll get an error is the process has died (and the pid reused) and you can
also use it to send the signal – avoiding the pid reuse race.

Just a smop.

David


Im in not wrong linux use a simple allocation method for pid’d were it just set 
the next pid to last pid allocated + 1. Then check if its free and if not keep 
adding until found free one. A process can be created few minutes before it 
exits and meanwhile the pid’s are continuing and wrap around to the point that 
at the moment it release is the exact same moment when the next_pid is the same 
one as the process that just exits.

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, 
UK
Registration No: 1397386 (Wales)
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Roberto A. Foglietta]

‪Il giorno lun 14 feb 2022 alle ore 17:21 ‫סאן עמר‬‎
 ha scritto:‬
>
> Im in not wrong linux use a simple allocation method for pid’d were it just 
> set the next pid to last pid allocated + 1. Then check if its free and if not 
> keep adding until found free one. A process can be created few minutes before 
> it exits and meanwhile the pid’s are continuing and wrap around to the point 
> that at the moment it release is the exact same moment when the next_pid is 
> the same one as the process that just exits.

While the timeout is sleeping for 1 second, the supervised PID process
dies and another one process starts with the same PID, then the
timeout wakes-up and checks for the PID then the race condition
happens if the PID process will last longer than the expiring timeout.
How much is probable the PID race condition? It related to these
factors:

- long timeout is used
- a lot of processes is created
- a lot of processes is running
- small maximum PID is used

 It could happen despite any oddity, by design. That's true. However,
it is not avoidable unless timeout does not establish a child-parent
relationship with the surveilled process as shown by Rob.


 Best regards,
--
Roberto A. Foglietta
+39.349.33.30.697
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[סאן עמר]

Im in not wrong linux use a simple allocation method for pid’d were it just
set the next pid to last pid allocated + 1. Then check if its free and if
not keep adding until found free one. A process can be created few minutes
before it exits and meanwhile the pid’s are continuing and wrap around to
the point that at the moment it release is the exact same moment when the
next_pid is the same one as the process that just exits.

בתאריך יום ב׳, 14 בפבר׳ 2022 ב-18:01 מאת Roberto A. Foglietta <
roberto.foglie...@gmail.com>:

> Il giorno sab 12 feb 2022 alle ore 02:41 Raffaello D. Di Napoli
>  ha scritto:
> >
> >
> > On 2/11/22 16:22, Rob Landley wrote:
> > > On 2/9/22 11:12 AM, Baruch Siach wrote:
> > >> Hi Sun,
> > >>
> > >> On Wed, Feb 09 2022, סאן עמר wrote:
> > >>> Hi, I'm using busybox for a while now (v1.29.2). and I had an issue
> with a sigterm send randomly to a process of mine. I debugged it until I
> found
> > >>> it from the timeout process which was assigned before to another
> process with the same pid. (i'm using a lot of timeouts for a lot of jobs)
> > >>> so i looked at the code, "timeout.c" file where it sleep for 1
> second in each iteration then check the timeout status. I suspect at this
> time the
> > >>> process timeout monitoring is terminated, but another one with the
> same pid is already created. which creates unwanted timeout.
> > >>>
> > >>> There is a comment in there about sleep for "HUGE NUM" will probably
> result in this issue, but I can't see why it won't occur also in the current
> > >>> case.
> > >>>
> > >>> there is no change of this behaviour in the latest master.
> > >>> i would appreciate any help, sun.
> > >> Any reference to PID number is inherently racy.
> > > Not between parent and child.
> >
> > Except in BB’s timeout, the relationship is not parent/child :)
> >
> > Much to my surprise, I’ll say that. When I read the bug report the other
> > day, I thought to myself well, this one ought to be easy to fix. But no,
> > there’s no SIGCHLD to be handled, no relationship between processes to
> > be leveraged.
> >
> > I don’t think this bug can be fixed without a near-complete rewrite, or
> > without doing a lot of procfs digging to really validate the waited-on
> > process, since kill(pid, 0) only validates a pid, not a process.
>
> https://github.com/brgl/busybox/blob/master/miscutils/timeout.c
>
> This is the code under inspection:
>
>  grandchild:
> /* Just sleep(HUGE_NUM); kill(parent) may kill wrong process! */
> while (1) {
> sleep(1);
> if (--timeout <= 0)
> break;
> if (kill(parent, 0)) {
> /* process is gone */
> return EXIT_SUCCESS;
> }
> }
> kill(parent, signo);
> return EXIT_SUCCESS;
>
> After all, it might conduct to a PID-race only if the same pid is
> reused within a second. Which means that 32768-N processes are created
> in less than a second. Where N is the running processes in the system.
>
>  Best regards,
> --
> Roberto A. Foglietta
> +39.349.33.30.697
>
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Roberto A. Foglietta]

Il giorno lun 14 feb 2022 alle ore 17:00 Roberto A. Foglietta
 ha scritto:
>
> Il giorno sab 12 feb 2022 alle ore 02:41 Raffaello D. Di Napoli
>  ha scritto:
> >
> >
> > On 2/11/22 16:22, Rob Landley wrote:
> > > On 2/9/22 11:12 AM, Baruch Siach wrote:
> > >> Hi Sun,
> > >>
> > >> On Wed, Feb 09 2022, סאן עמר wrote:
> > >>> Hi, I'm using busybox for a while now (v1.29.2). and I had an issue 
> > >>> with a sigterm send randomly to a process of mine. I debugged it until 
> > >>> I found
> > >>> it from the timeout process which was assigned before to another 
> > >>> process with the same pid. (i'm using a lot of timeouts for a lot of 
> > >>> jobs)
> > >>> so i looked at the code, "timeout.c" file where it sleep for 1 second 
> > >>> in each iteration then check the timeout status. I suspect at this time 
> > >>> the
> > >>> process timeout monitoring is terminated, but another one with the same 
> > >>> pid is already created. which creates unwanted timeout.
> > >>>
> > >>> There is a comment in there about sleep for "HUGE NUM" will probably 
> > >>> result in this issue, but I can't see why it won't occur also in the 
> > >>> current
> > >>> case.
> > >>>
> > >>> there is no change of this behaviour in the latest master.
> > >>> i would appreciate any help, sun.
> > >> Any reference to PID number is inherently racy.
> > > Not between parent and child.
> >
> > Except in BB’s timeout, the relationship is not parent/child :)
> >
> > Much to my surprise, I’ll say that. When I read the bug report the other
> > day, I thought to myself well, this one ought to be easy to fix. But no,
> > there’s no SIGCHLD to be handled, no relationship between processes to
> > be leveraged.
> >
> > I don’t think this bug can be fixed without a near-complete rewrite, or
> > without doing a lot of procfs digging to really validate the waited-on
> > process, since kill(pid, 0) only validates a pid, not a process.
>
> https://github.com/brgl/busybox/blob/master/miscutils/timeout.c
>
> This is the code under inspection:
>
>  grandchild:
> /* Just sleep(HUGE_NUM); kill(parent) may kill wrong process! */
> while (1) {
> sleep(1);
> if (--timeout <= 0)
> break;
> if (kill(parent, 0)) {
> /* process is gone */
> return EXIT_SUCCESS;
> }
> }
> kill(parent, signo);
> return EXIT_SUCCESS;
>
> After all, it might conduct to a PID-race only if the same pid is
> reused within a second. Which means that 32768-N processes are created
> in less than a second. Where N is the running processes in the system.

 The number of pids could be hugely increased up to 2^22 = 4,194,304
on a 64 bits platform. This does not resolve the issue but it makes it
hugely less probable.

 However, if this bug shows-up, probably it means that the system has
a lot of processes running and a lot of processes created and
destroyed compared to the max PID available. Thus, the system might be
incorrectly configured compared with its typical usage which probably
is the main reason because nobody complained before.

 https://stackoverflow.com/questions/6294133/maximum-pid-in-linux

 Best regards,
-- 
Roberto A. Foglietta
+39.349.33.30.697
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Roberto A. Foglietta]

Il giorno sab 12 feb 2022 alle ore 02:41 Raffaello D. Di Napoli
 ha scritto:
>
>
> On 2/11/22 16:22, Rob Landley wrote:
> > On 2/9/22 11:12 AM, Baruch Siach wrote:
> >> Hi Sun,
> >>
> >> On Wed, Feb 09 2022, סאן עמר wrote:
> >>> Hi, I'm using busybox for a while now (v1.29.2). and I had an issue with 
> >>> a sigterm send randomly to a process of mine. I debugged it until I found
> >>> it from the timeout process which was assigned before to another process 
> >>> with the same pid. (i'm using a lot of timeouts for a lot of jobs)
> >>> so i looked at the code, "timeout.c" file where it sleep for 1 second in 
> >>> each iteration then check the timeout status. I suspect at this time the
> >>> process timeout monitoring is terminated, but another one with the same 
> >>> pid is already created. which creates unwanted timeout.
> >>>
> >>> There is a comment in there about sleep for "HUGE NUM" will probably 
> >>> result in this issue, but I can't see why it won't occur also in the 
> >>> current
> >>> case.
> >>>
> >>> there is no change of this behaviour in the latest master.
> >>> i would appreciate any help, sun.
> >> Any reference to PID number is inherently racy.
> > Not between parent and child.
>
> Except in BB’s timeout, the relationship is not parent/child :)
>
> Much to my surprise, I’ll say that. When I read the bug report the other
> day, I thought to myself well, this one ought to be easy to fix. But no,
> there’s no SIGCHLD to be handled, no relationship between processes to
> be leveraged.
>
> I don’t think this bug can be fixed without a near-complete rewrite, or
> without doing a lot of procfs digging to really validate the waited-on
> process, since kill(pid, 0) only validates a pid, not a process.

https://github.com/brgl/busybox/blob/master/miscutils/timeout.c

This is the code under inspection:

 grandchild:
/* Just sleep(HUGE_NUM); kill(parent) may kill wrong process! */
while (1) {
sleep(1);
if (--timeout <= 0)
break;
if (kill(parent, 0)) {
/* process is gone */
return EXIT_SUCCESS;
}
}
kill(parent, signo);
return EXIT_SUCCESS;

After all, it might conduct to a PID-race only if the same pid is
reused within a second. Which means that 32768-N processes are created
in less than a second. Where N is the running processes in the system.

 Best regards,
-- 
Roberto A. Foglietta
+39.349.33.30.697
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Laurent Bercot]

/* We want to create a grandchild which will watch
 * and kill the grandparent. Other methods:
 * making parent watch child disrupts parent<->child link
 * (example: "tcpsvd 0.0.0.0 1234 timeout service_prog" -
 * it's better if service_prog is a child of tcpsvd!),
 * making child watch parent results in programs having
 * unexpected children.*/

I don't follow this reasoning.  Does "disrupts the parent<->child link" just 
about sending signals?  If the timeout app relays all signals from itself to the child, what 
remaining problems would exist?


 Yeah, that's clearly a misdesign.
 Keeping the same pid for the end of a Bernstein chain whenever possible
is a good idea, so the intention is good - but it's only possible when
the child is *entirely transparent* wrt control flow. It's a good
model, for instance, for ssh-agent, or for a data processor such as a
TLS tunnel (that's how s6-tlsd operates); but here, since the child
actually impacts control (it sends a signal to the parent on timeout)
it's just not applicable - and this thread illustrates exactly why.

 To really fix the bug, timeout should be rewritten, and run as the
parent, and possibly forward signals to the child.

--
 Laurent

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Rob Landley]

On 2/13/22 1:31 AM, סאן עמר wrote:
> Hey, first of all thanks for your answers and sorry for how I phrase my 
> question.

Sorry, looks like I may be the one who needs to apologize. (See below.)

> Rob, I still need a clarification if you don't mind. anything that you mention
> is making sense to me if the timeout implementation here was that the actual
> command is a child process of a process that doing the timeout logic.

One way that wait() can become unreliable is if the child process inherited a
signal mask from its parent (or grandparent) process with SIGCHLD ignored. Let's
see what happens in that case, via this dumb little test program:

$ cat blah.c
#include 
#include 
#include 
#include 
#include 
#include 

int main(int argc, char *argv[])
{
  int pid, status = 0;

  signal(SIGCHLD, SIG_IGN);
  if (!(pid = fork())) {sleep(1); exit(1);}
  else {
pid = wait();
printf("exited %d %d\n", pid, status);
  }

  exit(1);
}

So wait() blocks until the child exits, then returns -1 with status left at 0.
Let's see what busybox */timeout.c is doing:

int status;
...
wait(); /* wait for child to die */
/* Did intermediate [v]fork or exec fail? */
if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
return EXIT_FAILURE;

Which means it's not checking wait's return value, and with a 0 status
_WIFEXITED() returns... um, some grep -r under /usr/include says:

#define __WEXITSTATUS(status)   (((status) & 0xff00) >> 8)
#define __WTERMSIG(status)  ((status) & 0x7f)
#define __WIFEXITED(status) (__WTERMSIG(status) == 0)

So 0 reads as "it exited" and "exit status 0". So busybox timeout gets a false
positive for success. (Except that "status" was uninitalized so it's only
PROBABLY zero, not necessarily required to be. These days signal handlers have
their own stack so it's probably clean if the call to this function is the
first/deepest call made on the stack so far, which wouldn't necessarily be the
case for busybox calling shell builtins...

Anyway, if it doesn't EXIT_FAILURE it falls through to:

/* Ok, exec a program as requested */
argv += optind;
#if !BB_MMU
argv[0] = sv1;
argv[1] = sv2;
#endif
BB_EXECVP_or_die(argv);

Wait, the PARENT process is execing the command line, but is doing so AFTER
waiting for the child? What?

The child is sending a kill signal to the parent process? Ok, that is EXACTLY
the "cannot reliably work" that the other guy was replying to you, that's 
BONKERS.

static NOINLINE int timeout_wait(int timeout, pid_t pid)
{
/* Just sleep(HUGE_NUM); kill(parent) may kill wrong process! */
...
if (kill(pid, 0)) {
...
}

And the caller is:
   /* Here we are grandchild. Sleep, then kill grandparent */
 grandchild:
   if (timeout_wait(timeout, parent) == EXIT_SUCCESS)

I just... no? It literally HAS A COMMENT saying that what it's doing won't work
reliably?

At this point, I think _I_ need some explanation about what's going on here.

> which is the case for other implementations I saw (for example here
> )
> but for the case of busybox implementation, the timeout is a child process of
> the command it runs on (actually a grandchild),

Yeah, sorry. That IS bonkers.

The "this API cannot be used sanely" complaint was wrong, but "busybox is not
using this API sanely" is ALSO wrong. To reliably monitor a process you either
need to be its parent, or set up a similar relationship like containers, or
ptrace it, or you can pull some tricks with a filehandle to its /proc/$PID
contents...

But not this.

> so the case of process that
> represents the command finish and its parent releasing its PID (via wait) can
> occur, while the process that represents the timeout missing it.
> and all this can happen while there is no case of ignoring the SIGCHLD.
> am I wrong?

My bad. I thought the busybox code here was sane. It does not appear to be.
You're right: reversing the parent/child relationship like this CANNOT be 
reliable.

I assumed busybox's implementation worked like the one I wrote at
https://github.com/landley/toybox/blob/master/toys/other/timeout.c where the if
(!pid) does the exec() and the else case handles waiting, and it just uses an
alarm timer to interrupt itself. But apparently not?

(Sorry I didn't check earlier, but I don't usually look at busybox code anymore
because I work on a project under a non-GPL license and don't want to get sued.)

Rob
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[סאן עמר]

Hey, first of all thanks for your answers and sorry for how I phrase my
question.
Rob, I still need a clarification if you don't mind. anything that you
mention is making sense to me if the timeout implementation here was that
the actual command is a child process of a process that doing the timeout
logic.
which is the case for other implementations I saw (for example here

)
but for the case of busybox implementation, the timeout is a child process
of the command it runs on (actually a grandchild), so the case of process
that represents the command finish and its parent releasing its PID (via
wait) can occur, while the process that represents the timeout missing it.
and all this can happen while there is no case of ignoring the SIGCHLD.
am I wrong?


‫בתאריך יום ו׳, 11 בפבר׳ 2022 ב-23:19 מאת ‪Rob Landley‬‏ <‪r...@landley.net
‬‏>:‬

> On 2/9/22 11:12 AM, Baruch Siach wrote:
> > Hi Sun,
> >
> > On Wed, Feb 09 2022, סאן עמר wrote:
> >> Hi, I'm using busybox for a while now (v1.29.2). and I had an issue
> with a sigterm send randomly to a process of mine. I debugged it until I
> found
> >> it from the timeout process which was assigned before to another
> process with the same pid. (i'm using a lot of timeouts for a lot of jobs)
> >> so i looked at the code, "timeout.c" file where it sleep for 1 second
> in each iteration then check the timeout status. I suspect at this time the
> >> process timeout monitoring is terminated, but another one with the same
> pid is already created. which creates unwanted timeout.
> >>
> >> There is a comment in there about sleep for "HUGE NUM" will probably
> result in this issue, but I can't see why it won't occur also in the current
> >> case.
> >>
> >> there is no change of this behaviour in the latest master.
> >> i would appreciate any help, sun.
> >
> > Any reference to PID number is inherently racy.
>
> Not between parent and child. That's why zombies need reaping: a child
> process
> will not exit until the parent accepts its exit code and timing data via
> wait()
> (or itself exits). You can leave a zombie process lying around for
> literally
> years and the PID won't be reused while the zombie still exists.
>
> You can set SIGCHLD to SIG_IGN to allow your child processes to
> asynchronously
> self-reap, which can make it disappear out from under you. And that signal
> mask
> can be inherited by child processes, just like you can leak filehandles and
> environment variables and tty state and cpu mask and nice level and a dozen
> other things into your child process state: the parent process defines the
> child
> state back to PID 1.
>
> If a parent process ignores SIGCHLD and spawns child processes that run
> timeout,
> in that case the child process of the timeout could cycle out from under
> timeout. I'd look for that in this instance.
>
> One workaround would would be timeout.c explicitly doing signal(SIGCHLD,
> SIG_DFL) to correct its inherited signal mask, but the parent process
> doing that
> is pilot error, and that "fix" is about like a program using fcntl(SETFL)
> to
> strip O_NONBLOCK off stdout just in case it was called with broken standard
> filehandles: that kind of defensive programming isn't generally part of
> busybox.
>
> > There is no solution for
> > your problem in the traditional POSIX API.
>
> The preponderance of the evidence is that the posix API people have used
> for ~50
> years and sent into space and such can indeed work reliably for people who
> know
> how to use it.
>
> Next time you want to say "Linux/posix is fundamentally broken, there is no
> solution in existing deployed kernels, this command busybox has had for 14
> years
> can't ever have worked reliably and nobody noticed before", why not try
> phrasing
> it as a question instead of a statement of fact?
>
> Rob
>
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

RE: suspected bug in timeout command

[David Laight]

From: Michael Conrad
> Sent: 12 February 2022 12:59
> 
> On 2/12/22 07:38, Michael Conrad wrote:
> > Correctly using pidfd *still* requires that you be the parent process,
> > else the child could get reaped and replaced before the pidfd is
> > created.  As far as I can tell, the only purpose of pidfd is for
> > waking on poll() instead of using signals, which is orthagonal to this
> > problem.

Even if the pidfd can't be passed down, it lets you verify the process
information and then send the signal.

> > I haven't looked at the source in busybox yet, but it boggles my mind
> > that it wouldn't just be a simple fork+alarm+waitpid because that is
> > literally the least code implementation, and race-free.
> >
> > -Mike C
> >
> Sorry for being lazy.  I looked at the source and this is the reason:
> 
> /* We want to create a grandchild which will watch
>   * and kill the grandparent. Other methods:
>   * making parent watch child disrupts parent<->child link
>   * (example: "tcpsvd 0.0.0.0 1234 timeout service_prog" -
>   * it's better if service_prog is a child of tcpsvd!),
>   * making child watch parent results in programs having
>   * unexpected children.    */
> 
> I don't follow this reasoning.  Does "disrupts the parent<->child link"
> just about sending signals?  If the timeout app relays all signals from
> itself to the child, what remaining problems would exist?

In that case you can pass 'verification information' through to
the grandchild.

It could be an open fd of "/proc/self" - which allows the non-racy
kill on recent kernels.
But other information would allow the timing window be minimalised on
older kernels.

ISTR that on older kernels an open fd to "proc/nn" always refers to the
current process with pid nn. But the actual behaviour is worth checking.
I think newer kernels will fail any reads after the process has exited.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, 
UK
Registration No: 1397386 (Wales)

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Michael Conrad]

On 2/12/22 07:38, Michael Conrad wrote:
Correctly using pidfd *still* requires that you be the parent process, 
else the child could get reaped and replaced before the pidfd is 
created.  As far as I can tell, the only purpose of pidfd is for 
waking on poll() instead of using signals, which is orthagonal to this 
problem.


I haven't looked at the source in busybox yet, but it boggles my mind 
that it wouldn't just be a simple fork+alarm+waitpid because that is 
literally the least code implementation, and race-free.


-Mike C


Sorry for being lazy.  I looked at the source and this is the reason:

/* We want to create a grandchild which will watch
 * and kill the grandparent. Other methods:
 * making parent watch child disrupts parent<->child link
 * (example: "tcpsvd 0.0.0.0 1234 timeout service_prog" -
 * it's better if service_prog is a child of tcpsvd!),
 * making child watch parent results in programs having
 * unexpected children.    */

I don't follow this reasoning.  Does "disrupts the parent<->child link" 
just about sending signals?  If the timeout app relays all signals from 
itself to the child, what remaining problems would exist?


-Mike C

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Michael Conrad]

On 2/12/22 06:08, David Laight wrote:

From: Raffaello D. Di Napoli

Sent: 12 February 2022 01:33


On 2/11/22 16:22, Rob Landley wrote:

On 2/9/22 11:12 AM, Baruch Siach wrote:

Hi Sun,

On Wed, Feb 09 2022, סאן עמר wrote:

Hi, I'm using busybox for a while now (v1.29.2). and I had an issue with a 
sigterm send randomly

to a process of mine. I debugged it until I found

it from the timeout process which was assigned before to another process with 
the same pid. (i'm

using a lot of timeouts for a lot of jobs)

so i looked at the code, "timeout.c" file where it sleep for 1 second in each 
iteration then check

the timeout status. I suspect at this time the

process timeout monitoring is terminated, but another one with the same pid is 
already created.

which creates unwanted timeout.

There is a comment in there about sleep for "HUGE NUM" will probably result in 
this issue, but I

can't see why it won't occur also in the current

case.

there is no change of this behaviour in the latest master.
i would appreciate any help, sun.

Any reference to PID number is inherently racy.

Not between parent and child.

Except in BB’s timeout, the relationship is not parent/child :)

Much to my surprise, I’ll say that. When I read the bug report the other
day, I thought to myself well, this one ought to be easy to fix. But no,
there’s no SIGCHLD to be handled, no relationship between processes to
be leveraged.

I don’t think this bug can be fixed without a near-complete rewrite, or
without doing a lot of procfs digging to really validate the waited-on
process, since kill(pid, 0) only validates a pid, not a process.

And Linux uses a strict 'next free pid' algorithm for new processes
so the is no guard time between a process exiting and its pid being reused.
This problem was 'fixed' inside the kernel by using a small structure
instead of the pid itself - but that didn't help userspace (or even some 
drivers).
By comparison NetBSD uses the high bits of the pid as a 'generation number'
and so guarantees that a pid won't be reused for some time (a few thousand 
forks).

You can use the process start time (I think it is in /proc/pid/stat)
to validate the process just before the kill().
That leaves a very small timing window that it is hard to avoid
without using pidfd.

David

Correctly using pidfd *still* requires that you be the parent process, 
else the child could get reaped and replaced before the pidfd is 
created.  As far as I can tell, the only purpose of pidfd is for waking 
on poll() instead of using signals, which is orthagonal to this problem.


I haven't looked at the source in busybox yet, but it boggles my mind 
that it wouldn't just be a simple fork+alarm+waitpid because that is 
literally the least code implementation, and race-free.


-Mike C

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

RE: suspected bug in timeout command

[David Laight]

From: Raffaello D. Di Napoli
> Sent: 12 February 2022 01:33
> 
> 
> On 2/11/22 16:22, Rob Landley wrote:
> > On 2/9/22 11:12 AM, Baruch Siach wrote:
> >> Hi Sun,
> >>
> >> On Wed, Feb 09 2022, סאן עמר wrote:
> >>> Hi, I'm using busybox for a while now (v1.29.2). and I had an issue with 
> >>> a sigterm send randomly
> to a process of mine. I debugged it until I found
> >>> it from the timeout process which was assigned before to another process 
> >>> with the same pid. (i'm
> using a lot of timeouts for a lot of jobs)
> >>> so i looked at the code, "timeout.c" file where it sleep for 1 second in 
> >>> each iteration then check
> the timeout status. I suspect at this time the
> >>> process timeout monitoring is terminated, but another one with the same 
> >>> pid is already created.
> which creates unwanted timeout.
> >>>
> >>> There is a comment in there about sleep for "HUGE NUM" will probably 
> >>> result in this issue, but I
> can't see why it won't occur also in the current
> >>> case.
> >>>
> >>> there is no change of this behaviour in the latest master.
> >>> i would appreciate any help, sun.
> >> Any reference to PID number is inherently racy.
> > Not between parent and child.
> 
> Except in BB’s timeout, the relationship is not parent/child :)
> 
> Much to my surprise, I’ll say that. When I read the bug report the other
> day, I thought to myself well, this one ought to be easy to fix. But no,
> there’s no SIGCHLD to be handled, no relationship between processes to
> be leveraged.
> 
> I don’t think this bug can be fixed without a near-complete rewrite, or
> without doing a lot of procfs digging to really validate the waited-on
> process, since kill(pid, 0) only validates a pid, not a process.

And Linux uses a strict 'next free pid' algorithm for new processes
so the is no guard time between a process exiting and its pid being reused.
This problem was 'fixed' inside the kernel by using a small structure
instead of the pid itself - but that didn't help userspace (or even some 
drivers).
By comparison NetBSD uses the high bits of the pid as a 'generation number'
and so guarantees that a pid won't be reused for some time (a few thousand 
forks).

You can use the process start time (I think it is in /proc/pid/stat)
to validate the process just before the kill().
That leaves a very small timing window that it is hard to avoid
without using pidfd.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, 
UK
Registration No: 1397386 (Wales)
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Rob Landley]

On 2/9/22 11:12 AM, Baruch Siach wrote:
> Hi Sun,
> 
> On Wed, Feb 09 2022, סאן עמר wrote:
>> Hi, I'm using busybox for a while now (v1.29.2). and I had an issue with a 
>> sigterm send randomly to a process of mine. I debugged it until I found
>> it from the timeout process which was assigned before to another process 
>> with the same pid. (i'm using a lot of timeouts for a lot of jobs)
>> so i looked at the code, "timeout.c" file where it sleep for 1 second in 
>> each iteration then check the timeout status. I suspect at this time the
>> process timeout monitoring is terminated, but another one with the same pid 
>> is already created. which creates unwanted timeout.
>>
>> There is a comment in there about sleep for "HUGE NUM" will probably result 
>> in this issue, but I can't see why it won't occur also in the current
>> case.
>>
>> there is no change of this behaviour in the latest master.
>> i would appreciate any help, sun.
> 
> Any reference to PID number is inherently racy.

Not between parent and child. That's why zombies need reaping: a child process
will not exit until the parent accepts its exit code and timing data via wait()
(or itself exits). You can leave a zombie process lying around for literally
years and the PID won't be reused while the zombie still exists.

You can set SIGCHLD to SIG_IGN to allow your child processes to asynchronously
self-reap, which can make it disappear out from under you. And that signal mask
can be inherited by child processes, just like you can leak filehandles and
environment variables and tty state and cpu mask and nice level and a dozen
other things into your child process state: the parent process defines the child
state back to PID 1.

If a parent process ignores SIGCHLD and spawns child processes that run timeout,
in that case the child process of the timeout could cycle out from under
timeout. I'd look for that in this instance.

One workaround would would be timeout.c explicitly doing signal(SIGCHLD,
SIG_DFL) to correct its inherited signal mask, but the parent process doing that
is pilot error, and that "fix" is about like a program using fcntl(SETFL) to
strip O_NONBLOCK off stdout just in case it was called with broken standard
filehandles: that kind of defensive programming isn't generally part of busybox.

> There is no solution for
> your problem in the traditional POSIX API.

The preponderance of the evidence is that the posix API people have used for ~50
years and sent into space and such can indeed work reliably for people who know
how to use it.

Next time you want to say "Linux/posix is fundamentally broken, there is no
solution in existing deployed kernels, this command busybox has had for 14 years
can't ever have worked reliably and nobody noticed before", why not try phrasing
it as a question instead of a statement of fact?

Rob
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: suspected bug in timeout command

[Michael Conrad]

On 2/9/22 12:12, Baruch Siach wrote:

Hi Sun,

On Wed, Feb 09 2022, סאן עמר wrote:

Hi, I'm using busybox for a while now (v1.29.2). and I had an issue with a 
sigterm send randomly to a process of mine. I debugged it until I found
it from the timeout process which was assigned before to another process with 
the same pid. (i'm using a lot of timeouts for a lot of jobs)
so i looked at the code, "timeout.c" file where it sleep for 1 second in each 
iteration then check the timeout status. I suspect at this time the
process timeout monitoring is terminated, but another one with the same pid is 
already created. which creates unwanted timeout.

There is a comment in there about sleep for "HUGE NUM" will probably result in 
this issue, but I can't see why it won't occur also in the current
case.

there is no change of this behaviour in the latest master.
i would appreciate any help, sun.

Any reference to PID number is inherently racy. There is no solution for
your problem in the traditional POSIX API.


PIDs are not racy if they are sent by the parent process.  Since the 
timeout command starts the other command, and waits to kill it, it 
should be the parent process, and will always reliably know that either 
the child is not reaped and can be sent a signal, or that it is reaped 
and there's no reason to.  Perhaps the implementation of this command 
needs corrected?


-Mike C

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox