Re: [resend] Re: handling overflow in sbrk.

["Chris Demetriou" <cgd at google dot com>]

On Thu, May 29, 2008 at 08:04, Carlos O'Donell <carlos@codesourcery.com> wrote:
>
> What does the test do when you run make -k check on an native 64-bit system?

+ if (sizeof (void *) != 4)
+    {
+      puts ("this test only supports 32-bit systems, TEST INDETERMINATE.");
+      return 0;
+    }


>> 2008-05-20  Chris Demetriou  <cgd@google.com>
>>
>>        * misc/sbrk.c (__sbrk): If incrementing __curbrk by the requested
>>        amount would cause it to overflow, return an error (ENOMEM).
>>        * misc/tst-sbrk1.c: New test.
>>        * misc/Makefile (tests): Add new test.
>>
>
> I saw you posted this again to libc-alpha, I hope you put on asbestos underwear.

Err, did I?  I tried to do that shortly after I sent it to libc-help
and got no response, but didn't try to resend after that.

Anyway, I volunteered to send this (rather than having the colleague
who noticed it send it) just for this reason.  8-)


> The patch makes the common case in sbrk slower, and the kernel (2.6.25) already has support checking this overflow?
>
> ~~~ mm/mmap.c (do_brk) ~~~
>       if ((addr + len) > TASK_SIZE || (addr + len) < addr)
>                return -EINVAL;
> ~~~
>
> Can you identify which kernels are broken?

2.6.25 is still broken AFAICT.  I don't know whether or not the intent
is ultimately to fix this in the kernel.

do_brk is nice, but its return value is not used to ever propagate an
actual error back to the program.  The actual brk syscall is
implemented (in general) by sys_brk, which includes the code at:

http://lxr.linux.no/linux+v2.6.25.4/mm/mmap.c#L279

 278        /* Ok, looks good - let it rip. */
 279        if (do_brk(oldbrk, newbrk-oldbrk) != oldbrk)
 280                goto out;

The return value is not used, other than to check to see whether it's
the same as oldbrk.  If it is, success, brk is updated and the new brk
value is returned from sys_brk.  If it is not, failure, brk is not
updated, and the (new == old) brk value is returned.

In fact, though, since in the wraparound case it appears that the brk
is being lowered, I believe the code:

http://lxr.linux.no/linux+v2.6.25.4/mm/mmap.c#L268

 267        /* Always allow shrinking brk. */
 268        if (brk <= mm->brk) {
 269                if (!do_munmap(mm, newbrk, oldbrk-newbrk))
 270                        goto set_brk;
 271                goto out;
 272        }

is being run... but the net effect is the same on failure.


IMO yes, the kernel _should_ return an error on bad sys_brk, but given
that it does not (maybe never has) and AFAICT the interface to sys_brk
is different than brk and is not actually firmly documented... I'm a
bit concerned about trying to make that change.

re: slow path: well, you're just about to do a syscall which changes
your page tables... *and* it's a correctness issue.  Yes probably
better to be handled in the kernel with a normal error return ... but
it's not.


chris


chris