This is the mail archive of the libc-help@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

handling overflow in sbrk.


A colleague recently got a bug report about a custom sbrk-based memory
allocator he works on.  (32-bit process running on an x86-64 linux
system.)  The problem was that the allocator tried to do sbrk() giving
an increment which wrapped the break pointer around to the beginning
of the address space.

IMO, it would be good to return an error in this case.  SUSv2
(http://www.opengroup.org/onlinepubs/007908799/xsh/brk.html) says:

> The brk() and sbrk() functions will fail if:
>   [ENOMEM] The requested change would allocate more space than allowed.
>
> The brk() and sbrk() functions may fail if:
> [...]
>   [ENOMEM] The requested change would be impossible as there is insufficient swap space available, or would cause a memory allocation conflict.

To my mind, this is the former (break overflowing pointer size, uh,
more space than allowed 8-).  However, the latter ("memory allocation
conflict") would be a reasonable interpretation as well, though
obviously an error is not *required* in that case.


What happens in this case in glibc is that sbrk hands off the
incremented break value to brk(), which attempts to set it.  The Linux
kernel doesn't seem to communicate an error return from sys_brk in any
way -- it just returns the new break value.  In this case (integer
overflow/wraparound), sys_brk fails and returns the old brk value.
The glibc brk implementation checks for the new break value being too
low:

>  if (newbrk < addr)
>    {
>      __set_errno (ENOMEM);
>      return -1;
>    }

... but since addr wrapped around, that check passes and success is returned.

It's not obvious that there's an easy fix to brk in this case, to make
it return an error.  The kernel doesn't seem to return any sort of
error indication (http://lxr.linux.no/linux/mm/mmap.c#L237).  It might
be possible to get the page size, verify that the break that is set
actually matches the value requested, etc... but that seems fragile
and would prohibit reasonable kernel implementations (e.g., that for
some reason used a larger-than-expected page size, or did any number
of other things).  Any such approach would also require all linux brk
implementations to be changed.

On the other hand, it's really easy to add a check for overflow in
sbrk.  If __curbrk + incr wraps the pointer, declare an error.


See attached test program (sbrktest.c).  Build for (32-bit) x86 and
run on an x86-64 linux system, put text at 0xd0000000 so it's easy to
wrap the break.

Build/run from current glibc sources:

$ gcc -Ttext=0xd0000000 sbrktest.c -L ../inst/lib -static
$ ./a.out
starting
sbrk (0) -> 0xd009e000
sbrk (40000000) -> 0xd009e000
sbrk (0) -> 0xd009e000
0xd009e000, 0xd009e000, 0xd009e000
rv1 == rv2, lose!
$

Build/run from glibc sources with the attached patch
(sbrk_overflow.patch) applied:

$ gcc -Ttext=0xd0000000 sbrktest.c -L ../inst/lib -static
$ ./a.out
starting
sbrk (0) -> 0xd009e000
sbrk (40000000) -> 0xffffffff
 errno = 12 (Cannot allocate memory)
sbrk (0) -> 0xd009e000
0xd009e000, 0xffffffff, 0xd009e000
done
$


Obviously, this doesn't fix all possible issues w/ sbrk (which,
personally, I wouldn't recommend using these days 8-), but it does fix
one case.  IMO if a library/program is still relying on sbrk to do its
memory management, it probably wants to get an error in this case,
rather than the current behavior.


What do y'all think?


thanks,

chris
--
2008-05-16  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).
2008-05-16  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).

Index: misc/sbrk.c
===================================================================
RCS file: /cvs/glibc/libc/misc/sbrk.c,v
retrieving revision 1.1
diff -u -u -p -r1.1 sbrk.c
--- misc/sbrk.c	14 Dec 2005 10:36:01 -0000	1.1
+++ misc/sbrk.c	17 May 2008 06:26:03 -0000
@@ -16,6 +16,7 @@
    Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
    02111-1307 USA.  */
 
+#include <stdint.h>
 #include <unistd.h>
 #include <errno.h>
 
@@ -46,6 +47,15 @@ __sbrk (intptr_t increment)
   if (increment == 0)
     return __curbrk;
 
+  /* If increment is positive (i.e., memory is being requested) and
+     will cause the break to wrap around, the caller has asked for
+     more memory than is supported.  */
+  if (increment > 0
+      && ((uintptr_t) __curbrk + increment) < (uintptr_t) __curbrk) {
+    __set_errno (ENOMEM);
+    return (void *) -1;
+  }
+
   oldbrk = __curbrk;
   if (__brk (oldbrk + increment) < 0)
     return (void *) -1;

Attachment: sbrktest.c
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]