R devel - Feb 2008 - patch: two minor debugging-related pointer protection stack issues (PR#10832)

Full_Name: John Brzustowski
Version: R-devel trunk and R-2.4.0
OS: linux
Submission from: (NULL) (76.10.152.79)


Here are two minor potential issues in pointer protection stack (PPS) code which
could arise in debugging R with valgrind.  Only the second could possibly cause
a problem in normal use of R, and only under laughably implausible
circumstances.

(1) valgrind is given a wrong address when it is told to mark the reserved
"red
zone" portion of the PPS as "no access".   This might generate
spurious access
errors while debugging under valgrind.

(2) unprotect_ptr always does one more copy than necessary when collapsing the
slot of the pointer it is unprotecting.  This is but a trivial performance
penalty unless the PPS is full, in which case it causes a read of the first word
in the PPS red zone. If issue (1) were corrected, that would trigger a spurious
valgrind error.

If all of the following were to occur:

  a) the PPS overflowed 
  b) the cleanup code filled the PPS red zone
  c) unprotect_ptr was called from the cleanup code while the PPS red zone was
full
and 
  d) the memory malloc'd for the PPS occupied the very end of a readable
segment
     with fewer than sizeof(SEXP) trailing guard bytes

then a segfault would result.  (You are laughing at this point, right?)

In any case, here is a patch against R-devel trunk:

Index: trunk/src/main/memory.c
==================================================================---
trunk/src/main/memory.c	(revision 44589)
+++ trunk/src/main/memory.c	(working copy)
@@ -1557,7 +1557,7 @@
 	R_Suicide("couldn't allocate memory for pointer stack");
     R_PPStackTop = 0;
 #if VALGRIND_LEVEL > 1
-    VALGRIND_MAKE_NOACCESS(R_PPStackTop+R_PPStackSize,PP_REDZONE_SIZE);
+    VALGRIND_MAKE_NOACCESS(R_PPStack+R_PPStackSize,PP_REDZONE_SIZE);
 #endif
     vsfac = sizeof(VECREC);
     R_VSize = (((R_VSize + 1)/ vsfac));
@@ -2329,11 +2329,11 @@
     } while ( R_PPStack[--i] != s );
 
     /* OK, got it, and  i  is indexing its location */
-    /* Now drop stack above it */
+    /* Now drop stack above it, if any */
 
-    do {
-    	R_PPStack[i] = R_PPStack[i + 1];
-    } while ( i++ < R_PPStackTop );
+    while (++i < R_PPStackTop) {
+    	R_PPStack[i - 1] = R_PPStack[i];
+    }
 
     R_PPStackTop--;
 }