[PATCH] Fix overflow in jobs

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix overflow in jobs

Natanael Copa
This fixes an issue with bash hanging if user process rlimit is too
high.

To reproduce:

  ulimit -u 9223372036854775807
  bash -c 'sleep 1 & wait $!'
---
 jobs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/jobs.c b/jobs.c
index fc966036..d203db27 100644
--- a/jobs.c
+++ b/jobs.c
@@ -765,7 +765,7 @@ bgp_resize ()
   else
     nsize = bgpids.nalloc;
 
-  while (nsize < js.c_childmax)
+  while (nsize < (ps_index_t)js.c_childmax)
     nsize *= 2;
 
   if (bgpids.nalloc < js.c_childmax)
--
2.15.0


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix overflow in jobs

Chet Ramey
On 12/7/17 5:36 PM, Natanael Copa wrote:
> This fixes an issue with bash hanging if user process rlimit is too
> high.

Thanks for the report.

--
``The lyf so short, the craft so long to lerne.'' - Chaucer
                 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRU    [hidden email]    http://tiswww.cwru.edu/~chet/

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix overflow in jobs

Natanael Copa
Hi,

On Fri, 8 Dec 2017 08:04:00 -0500
Chet Ramey <[hidden email]> wrote:

> On 12/7/17 5:36 PM, Natanael Copa wrote:
> > This fixes an issue with bash hanging if user process rlimit is too
> > high.  
>
> Thanks for the report.
>

Unfortunately, the proposed patch does not fix the case for 32 bit
architectures.

The bug was introduced with commit a0c0a00f[1].

I think the proper fix may be re-enable the js.c_childmax capping. What
do you think?

diff --git a/jobs.c b/jobs.c
index cef3c79..bf99266 100644
--- a/jobs.c
+++ b/jobs.c
@@ -4166,10 +4166,8 @@ initialize_job_control (force)
   if (js.c_childmax < 0)
     js.c_childmax = DEFAULT_CHILD_MAX;
 
-#if 0
   if (js.c_childmax > MAX_CHILD_MAX)
     js.c_childmax = MAX_CHILD_MAX;
-#endif
 
   return job_control;
 }
@@ -4547,10 +4545,8 @@ mark_dead_jobs_as_notified (force)
   if (js.c_childmax < 0)
     js.c_childmax = DEFAULT_CHILD_MAX;
 
-#if 0
   if (js.c_childmax > MAX_CHILD_MAX)
     js.c_childmax = MAX_CHILD_MAX;
-#endif
 
   /* Don't do anything if the number of dead processes is less than CHILD_MAX
      and we're not forcing a cleanup. */



[1]: http://git.savannah.gnu.org/cgit/bash.git/commit/?id=a0c0a00fc419b7bc08202a79134fcd5bc0427071

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix overflow in jobs

Chet Ramey
On 2/7/18 1:51 PM, Natanael Copa wrote:

> Hi,
>
> On Fri, 8 Dec 2017 08:04:00 -0500
> Chet Ramey <[hidden email]> wrote:
>
>> On 12/7/17 5:36 PM, Natanael Copa wrote:
>>> This fixes an issue with bash hanging if user process rlimit is too
>>> high.  
>>
>> Thanks for the report.
>>
>
> Unfortunately, the proposed patch does not fix the case for 32 bit
> architectures.

That's interesting. It seems to me that the kernel should reject attempts
to set the maximum number of processes larger than 2**(sizeof (pid_t)).

>
> The bug was introduced with commit a0c0a00f[1].
>
> I think the proper fix may be re-enable the js.c_childmax capping. What
> do you think?

That's the wrong place. If your original patch identifies the issue
correctly, we need to bound the size of bgpids.storage to something a
pid_t (aliased to ps_index_t) variable can address.

So instead of using js.c_childmax directly, we compute the maximum
table size using js.c_childmax unless it exceeds TYPE_MAXIMUM(pid_t).

--
``The lyf so short, the craft so long to lerne.'' - Chaucer
                 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRU    [hidden email]    http://tiswww.cwru.edu/~chet/

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix overflow in jobs

Chet Ramey
On 2/7/18 3:30 PM, Chet Ramey wrote:

>> Unfortunately, the proposed patch does not fix the case for 32 bit
>> architectures.
>
> That's interesting. It seems to me that the kernel should reject attempts
> to set the maximum number of processes larger than 2**(sizeof (pid_t)).

I looked at this on a 32-bit debian system and discovered that setting the
limit to anything above 2**32 - 1 will basically give you `unlimited'.
Setting it to anything below that but above 2**31 - 1 will give you a value
that `ulimit' will display, but sysconf() will return as negative, which
the code handles fine. It's when you have values between 2**30 and 2**31-1
where there's a problem in the code that calculates the max needed table
size.

However, when you correct that...


>> I think the proper fix may be re-enable the js.c_childmax capping. What
>> do you think?
>
> That's the wrong place. If your original patch identifies the issue
> correctly, we need to bound the size of bgpids.storage to something a
> pid_t (aliased to ps_index_t) variable can address.
>
> So instead of using js.c_childmax directly, we compute the maximum
> table size using js.c_childmax unless it exceeds TYPE_MAXIMUM(pid_t).

This is true, but doing that will probably result in the kernel killing
you when the code goes to malloc some 32 gig of memory, especially on a
32-bit system. So it seems appropriate to limit the table size to some
reasonable value, but the question is what constitutes `reasonable'. We
could start with some value like 32768 and see what happens.

--
``The lyf so short, the craft so long to lerne.'' - Chaucer
                 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRU    [hidden email]    http://tiswww.cwru.edu/~chet/