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]

Re: handling overflow in sbrk.


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.  Failures before (and also after, of course), in
case you care:

PRE-PATCH/bld32/TESTS.LOG:make[2]: ***
[/home/cgd/glibc/bld32/iconvdata/bug-iconv6.out] Error 1
PRE-PATCH/bld32/TESTS.LOG:make[2]: ***
[/home/cgd/glibc/bld32/iconvdata/tst-iconv7.out] Error 1
PRE-PATCH/bld32/TESTS.LOG:make[2]: ***
[/home/cgd/glibc/bld32/stdlib/isomac.out] Error 2
PRE-PATCH/bld32/TESTS.LOG:make[2]: ***
[/home/cgd/glibc/bld32/dlfcn/bug-atexit3.out] Error 1
PRE-PATCH/bld32/TESTS.LOG:make[2]:
[/home/cgd/glibc/bld32/posix/annexc.out] Error 1 (ignored)
PRE-PATCH/bld32/TESTS.LOG:make[2]: ***
[/home/cgd/glibc/bld32/misc/tst-sbrk1.out] Error 1
PRE-PATCH/bld32/TESTS.LOG:make[2]: ***
[/home/cgd/glibc/bld32/nptl/tst-cancel24.out] Error 139
PRE-PATCH/bld32/TESTS.LOG:make[2]: ***
[/home/cgd/glibc/bld32/nptl/tst-cleanupx4.out] Error 1
PRE-PATCH/bld32/TESTS.LOG:make[1]: ***
[/home/cgd/glibc/bld32/c++-types-check.out] Error 1
PRE-PATCH/bld64/TESTS.LOG:make[2]: ***
[/home/cgd/glibc/bld64/iconvdata/bug-iconv6.out] Error 1
PRE-PATCH/bld64/TESTS.LOG:make[2]: ***
[/home/cgd/glibc/bld64/iconvdata/tst-iconv7.out] Error 1
PRE-PATCH/bld64/TESTS.LOG:make[2]: ***
[/home/cgd/glibc/bld64/stdlib/isomac.out] Error 2
PRE-PATCH/bld64/TESTS.LOG:make[2]:
[/home/cgd/glibc/bld64/posix/annexc.out] Error 1 (ignored)
PRE-PATCH/bld64/TESTS.LOG:make[2]: ***
[/home/cgd/glibc/bld64/nptl/tst-robust8.out] Error 1
PRE-PATCH/bld64/TESTS.LOG:make[2]: ***
[/home/cgd/glibc/bld64/nptl/tst-stackguard1-static.out] Error 1
PRE-PATCH/bld64/TESTS.LOG:make[2]: ***
[/home/cgd/glibc/bld64/elf/tst-stackguard1-static.out] Error 1

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.
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"

Attachment: 32-on-32.LOG
Description: Binary data

Attachment: 32-on-64.LOG
Description: Binary data


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