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] |
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] |