Fixed brk(); it was almost completely broken.

Added a regression test for it.


git-svn-id: svn://svn.valgrind.org/valgrind/trunk@1810

coregrind/vg_syscalls.c

@@ -1327,33 +1327,42 @@ void VG_(perform_assumed_nonblocking_syscall) ( ThreadId tid )
          break;
 
       case __NR_brk: /* syscall 45 */
-         /* Haven't a clue if this is really right. */
-         /* int brk(void *end_data_segment); */
+         /* libc   says: int   brk(void *end_data_segment);
+            kernel says: void* brk(void* end_data_segment);  (more or less)
+
+            libc returns 0 on success, and -1 (and sets errno) on failure.
+            Nb: if you ask to shrink the dataseg end below what it
+            currently is, that always succeeds, even if the dataseg end
+            doesn't actually change (eg. brk(0)).  Unless it seg faults.
+
+            Kernel returns the new dataseg end.  If the brk() failed, this
+            will be unchanged from the old one.  That's why calling (kernel)
+            brk(0) gives the current dataseg end (libc brk() just returns
+            zero in that case).
+
+            Both will seg fault if you shrink it back into a text segment.
+         */
          MAYBE_PRINTF("brk ( %p ) --> ",arg1);
          KERNEL_DO_SYSCALL(tid,res);
          MAYBE_PRINTF("0x%x\n", res);
 
-         if (!VG_(is_kerror)(res)) {
-            if (arg1 == 0) {
-               /* Just asking where the current end is. (???) */
-               curr_dataseg_end = res;
-            } else
-            if (arg1 < curr_dataseg_end) {
-               /* shrinking the data segment. */
-               VG_TRACK( die_mem_brk, (Addr)arg1, 
+         if (res == arg1) {
+            /* brk() succeeded */
+            if (res < curr_dataseg_end) {
+               /* successfully shrunk the data segment. */
+               VG_TRACK( die_mem_brk, (Addr)arg1,
                                       curr_dataseg_end-arg1 );
-               curr_dataseg_end = arg1;
             } else
-            if (arg1 > curr_dataseg_end && res != 0) {
-               /* asked for more memory, and got it */
-               /* 
-               VG_(printf)("BRK: new area %x .. %x\n", 
-                           VG_(curr_dataseg_end, arg1-1 );
-               */
-               VG_TRACK( new_mem_brk, (Addr)curr_dataseg_end, 
-                                         arg1-curr_dataseg_end );
-               curr_dataseg_end = arg1;         
+            if (res > curr_dataseg_end && res != 0) {
+               /* successfully grew the data segment */
+               VG_TRACK( new_mem_brk, curr_dataseg_end,
+                                      arg1-curr_dataseg_end );
             }
+            curr_dataseg_end = res;
+
+         } else {
+            /* brk() failed */
+            vg_assert(curr_dataseg_end == res);
          }
          break;
 

memcheck/tests/Makefile.am

@@ -14,6 +14,7 @@ EXTRA_DIST = $(noinst_SCRIPTS) \
 	badfree.stderr.exp badfree.vgtest \
 	badjump.stderr.exp badjump.vgtest \
 	badloop.stderr.exp badloop.vgtest \
+	brk.stderr.exp brk.vgtest \
 	buflen_check.stderr.exp buflen_check.vgtest \
 	clientperm.stderr.exp \
 	clientperm.stdout.exp clientperm.vgtest \
@@ -59,8 +60,8 @@ EXTRA_DIST = $(noinst_SCRIPTS) \
 	threadederrno.stderr.exp threadederrno.stdout.exp threadederrno.vgtest
 
 check_PROGRAMS = \
-	badaddrvalue badfree badjump badloop buflen_check clientperm \
-	custom_alloc \
+	badaddrvalue badfree badjump badloop brk buflen_check 
+	clientperm custom_alloc \
 	doublefree error_counts errs1 exitprog fprw fwrite inits inline \
 	malloc1 malloc2 malloc3 manuel1 manuel2 manuel3 \
 	memalign_test memcmptest mmaptest nanoleak null_socket \
@@ -78,6 +79,7 @@ badaddrvalue_SOURCES 	= badaddrvalue.c
 badfree_SOURCES 	= badfree.c
 badjump_SOURCES 	= badjump.c
 badloop_SOURCES 	= badloop.c
+brk_SOURCES 		= brk.c
 buflen_check_SOURCES	= buflen_check.c
 clientperm_SOURCES 	= clientperm.c
 custom_alloc_SOURCES 	= custom_alloc.c

memcheck/tests/brk.c

@@ -0,0 +1,40 @@
+#include <assert.h>
+#include <stdio.h>
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+// kernel brk() and libc brk() act quite differently...
+
+int main(void)
+{
+   int i;
+   void* orig_ds = sbrk(0);
+   void* ds = orig_ds;
+   void* vals[10];
+   void* res;
+
+   vals[0] = (void*)0;
+   vals[1] = (void*)1;
+   vals[2] = ds - 0x1;          // small shrink
+   vals[3] = ds;
+   vals[4] = ds + 0x1000;       // small growth
+   vals[5] = ds + 0x40000000;   // too-big growth
+   vals[6] = ds + 0x500;        // shrink a little, but still above start size
+   vals[7] = ds - 0x1;          // shrink below start size
+//   vals[8] = ds - 0x1000;       // shrink a lot below start size (into text)
+//   vals[9] = (void*)0xffffffff;
+   vals[8] = (void*)0xffffffff;
+
+   for (i = 0; (void*)0xffffffff != vals[i]; i++) {
+      res = (void*)syscall(__NR_brk, vals[i]);
+   }
+
+   assert( 0 == brk(orig_ds) );  // libc brk()
+
+   for (i = 0; (void*)0xffffffff != vals[i]; i++) {
+      res = (void*)brk(vals[i]);
+   }
+
+   return 0;
+}

memcheck/tests/brk.stderr.exp

@@ -0,0 +1,7 @@
+
+
+ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
+malloc/free: in use at exit: 0 bytes in 0 blocks.
+malloc/free: 0 allocs, 0 frees, 0 bytes allocated.
+For a detailed leak analysis,  rerun with: --leak-check=yes
+For counts of detected errors, rerun with: -v

memcheck/tests/brk.vgtest

@@ -0,0 +1 @@
+prog: brk