[resend] Re: handling overflow in sbrk.
Chris Demetriou
cgd@google.com
Thu May 29 05:13:00 GMT 2008
[Resent because a coworker pointed out that the actual patch and other
attachments enclosed in the last copy was base64 encoded, and that wasn't
the preferred patch submission format. Sorry!]
On Tue, May 20, 2008 at 05:45, Carlos O'Donell <carlos@systemhalted.org> wrote:
>> 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).
>
> 1. Please add a test case for this bug. Converting your program into a
> test would be a great idea.
See attached patch. Also attached test run outputs from a system
running a 32-bit kernel and a 64-bit kernel, for good measure. (I
wanted to make sure that the fact that the 32-bit kernel takes a chunk
out of the process's address space wouldn't harm the test. This was a
change from my original test program.)
In order to make this test work (a) the test has to be linked
statically (see comments), and (b) the test can't limit memory
allocation.
I figured this wasn't reasonably testable on 64-bit systems (since I
know none that support remotely close to 64 VA bits -- i.e., the
kernel should ENOMEM before you get close enough to *try* to wrap
around), so punted there. It might not be harmful to allow the test
to run normally on 64-bit, but while I'm comfortable w/ trying to
allocate 4GB of virtual memory, I'm less comfortable about filling a
64-bit process's address space. 8-)
> 2. Please run the glibc testsuite to verify that there were no regressions.
Done, no regressions. Same failures before/after, except for the new
tst-sbrk1 which failed before and passed after.
This was on a CentOS 5 x86_64 kernel, uname -msrv -> Linux
2.6.18-53.1.13.el5xen #1 SMP Tue Feb 12 13:33:07 EST 2008 x86_64.
> 3. Do you have an FSF copyright assignment?
Yes, my understanding is that Google's copyright assignment covers
glibc. (I asked around.)
thanks,
chris
---
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.
-------------- next part --------------
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.
Index: misc/Makefile
===================================================================
RCS file: /cvs/glibc/libc/misc/Makefile,v
retrieving revision 1.122
diff -u -u -p -r1.122 Makefile
--- misc/Makefile 12 Oct 2007 21:18:18 -0000 1.122
+++ misc/Makefile 20 May 2008 22:40:23 -0000
@@ -79,11 +79,13 @@ endif
gpl2lgpl := error.c error.h
tests := tst-dirname tst-tsearch tst-fdset tst-efgcvt tst-mntent tst-hsearch \
- tst-error1 tst-pselect tst-insremque tst-mntent2
+ tst-error1 tst-pselect tst-insremque tst-mntent2 tst-sbrk1
ifeq (no,$(cross-compiling))
tests: $(objpfx)tst-error1-mem
endif
+tests-static = tst-sbrk1
+
CFLAGS-tsearch.c = $(uses-callbacks)
CFLAGS-lsearch.c = $(uses-callbacks)
CFLAGS-pselect.c = -fexceptions
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 20 May 2008 22:40:23 -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;
Index: misc/tst-sbrk1.c
===================================================================
RCS file: misc/tst-sbrk1.c
diff -N misc/tst-sbrk1.c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ misc/tst-sbrk1.c 20 May 2008 22:40:23 -0000
@@ -0,0 +1,135 @@
+/* Copyright (C) 2008 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Chris Demetriou <cgd@google.com>, 2008.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, write to the Free
+ Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+ 02111-1307 USA. */
+
+/* This test attempts to verify that sbrk catches and rejects
+ requests that would cause the break pointer to overflow. */
+
+#include <errno.h>
+#include <limits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <string.h>
+#include <unistd.h>
+
+/* The test starts by incrementing the break repeatedly by up to
+ INCREMENT, until it reaches TARGET_ADDRESS. Once there, the test
+ tries to increment the break by OVERFLOW_INCREMENT and attempts to
+ verify that that allocation fails.
+
+ If the test fails before the break reaches TARGET_ADDRESS, the
+ results are indeterminate (but we declare that things are OK.)
+
+ To work properly, this test must be statically linked. Otherwise,
+ shared library mappings may prevent the break from being
+ incremented up to TARGET_ADDRESS.
+
+ Similarly, if the stack (or kernel reserved memory) is at the end
+ of the address space, that should cause brk to fail. However, that
+ failure may not be signaled properly by the brk ipmlementation, so
+ sbrk has to check for overflow itself. This test is primarily
+ intended to test the overflow check in sbrk. */
+
+/* Allow 1GB + 256MB for kernel + process stack. */
+#define TARGET_ADDRESS ((uintptr_t) -((1 << 30) + (1 << 28)))
+/* Should be just a little bit more than necessary to cause overflow,
+ otherwise we may end up hitting text/data again! */
+#define OVERFLOW_INCREMENT ((1 << 30) + (1 << 28) + (1 << 16))
+
+static int
+do_test (void)
+{
+ uintptr_t cur_address, result_address, next_address;
+ intptr_t requested_incr;
+ int saved_errno;
+
+ puts ("testing sbrk for integer overflow.");
+ if (sizeof (void *) != 4)
+ {
+ puts ("this test only supports 32-bit systems, TEST INDETERMINATE.");
+ return 0;
+ }
+
+ printf ("trying to sbrk up to %p\n", (void *) TARGET_ADDRESS);
+
+ cur_address = (uintptr_t) sbrk (0);
+ if (cur_address > TARGET_ADDRESS)
+ {
+ puts ("starting break > target, TEST INDETERMINATE.");
+ return 0;
+ }
+
+ /* May have to call sbrk multiple times, if the difference between
+ the current break and the target isn't a positive intptr_t. */
+ do
+ {
+ requested_incr = TARGET_ADDRESS - cur_address;
+ if (requested_incr < 0)
+ requested_incr = INTPTR_MAX;
+
+ puts ("");
+ printf ("currently at %p, requesting additional %#lx\n",
+ (void *) cur_address, (long) requested_incr);
+ result_address = (uintptr_t) sbrk (requested_incr);
+ if (result_address == -1)
+ {
+ puts ("sbrk failed during setup, TEST INDETERMINATE.");
+ return 0;
+ }
+ next_address = (uintptr_t) sbrk (0);
+ printf ("sbrk returned %p, next address is %p\n",
+ (void *) result_address, (void *) next_address);
+ if (result_address >= next_address)
+ {
+ puts ("sbrk overflow during setup, TEST FAILED.");
+ exit (1);
+ }
+ }
+ while ((cur_address = (uintptr_t) sbrk (0)) < TARGET_ADDRESS);
+
+ /* Break is now at or slightly after TARGET_ADDRESS, add
+ OVERFLOW_INCREMENT to make it overflow. */
+ puts ("");
+ requested_incr = OVERFLOW_INCREMENT;
+ printf ("requesting additional %#lx to cause overflow.\n",
+ (long) requested_incr);
+ if ((result_address = (uintptr_t) sbrk (requested_incr)) != -1)
+ {
+ next_address = (uintptr_t) sbrk (0);
+ printf ("sbrk returned %p, next address is %p\n",
+ (void *) result_address, (void *) next_address);
+ puts ("sbrk at end of address space succeeded, TEST FAILED.");
+ exit (1);
+ }
+
+ saved_errno = errno;
+ printf ("overflow allocation failed, errno = %d (%s)\n", saved_errno,
+ strerror (saved_errno));
+ if (saved_errno != ENOMEM)
+ {
+ puts ("errno != ENOMEM, TEST FAILED");
+ }
+
+ puts ("TEST PASSED.");
+ return 0;
+}
+
+#define TEST_DATA_LIMIT RLIM_INFINITY
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
-------------- next part --------------
# /tmp/tst-sbrk1.PRE
testing sbrk for integer overflow.
trying to sbrk up to 0xb0000000
currently at 0x80ea000, requesting additional 0x7fffffff
sbrk returned 0x80ea000, next address is 0x880e9fff
currently at 0x880e9fff, requesting additional 0x27f16001
sbrk returned 0x880e9fff, next address is 0xb0000000
requesting additional 0x50010000 to cause overflow.
sbrk returned 0xb0000000, next address is 0xb0000000
sbrk at end of address space succeeded, TEST FAILED.
# /tmp/tst-sbrk1.POST
testing sbrk for integer overflow.
trying to sbrk up to 0xb0000000
currently at 0x80ea000, requesting additional 0x7fffffff
sbrk returned 0x80ea000, next address is 0x880e9fff
currently at 0x880e9fff, requesting additional 0x27f16001
sbrk returned 0x880e9fff, next address is 0xb0000000
requesting additional 0x50010000 to cause overflow.
overflow allocation failed, errno = 12 (Cannot allocate memory)
TEST PASSED.
#
-------------- next part --------------
==> PRE-PATCH/bld32/misc/tst-sbrk1.out <==
testing sbrk for integer overflow.
trying to sbrk up to 0xb0000000
currently at 0x80ea000, requesting additional 0x7fffffff
sbrk returned 0x80ea000, next address is 0x880e9fff
currently at 0x880e9fff, requesting additional 0x27f16001
sbrk returned 0x880e9fff, next address is 0xb0000000
requesting additional 0x50010000 to cause overflow.
sbrk returned 0xb0000000, next address is 0xb0000000
sbrk at end of address space succeeded, TEST FAILED.
==> POST-PATCH/bld32/misc/tst-sbrk1.out <==
testing sbrk for integer overflow.
trying to sbrk up to 0xb0000000
currently at 0x80ea000, requesting additional 0x7fffffff
sbrk returned 0x80ea000, next address is 0x880e9fff
currently at 0x880e9fff, requesting additional 0x27f16001
sbrk returned 0x880e9fff, next address is 0xb0000000
requesting additional 0x50010000 to cause overflow.
overflow allocation failed, errno = 12 (Cannot allocate memory)
TEST PASSED.
More information about the Libc-help
mailing list