[PATCH] draft: better string overflow checking (was: memory corruption in SAMBA_3_0)

[Martin Pool]

I was thinking about Andrew's fstring-overflow patch from a few weeks
ago: for developer builds, it touches the last byte of a string buffer
to check that it's as long as it should be.  

This should be reasonably helpful in catching string overflows on the
heap, but not so good on the stack, because the program can probably
write arbitrarily far past stack variables without trapping, even
under Valgrind.  Writing a \0 in there will damage *something* and
probably make the program crash, but it won't be very obvious.  I
think this might have been what Jerry saw the other day.

I think this patch is better: it thoroughly clobbers the contents of
string buffers to make any fstring/pstring/dynamic confusion obvious.

Here is an example that is caught in developer builds with this patch,
but is hard to catch otherwise:

#include "includes.h"

 int main(void)
{
	fstring dest;

	pstrcpy(dest, "hello");

	return 0;
}

This fails with an obvious message under gdb:

#0  0xf1f1f1f1 in ?? ()
Cannot access memory at address 0xf1f1f1f1

Please don't apply this yet because I want to see if it catches any
bugs, but I'd love to hear comments.


--- util_str.c.~1.83.~	2003-03-05 15:24:08.000000000 +1100
+++ util_str.c	2003-03-05 15:57:16.000000000 +1100
@@ -430,6 +430,27 @@ BOOL str_is_all(const char *s,char c)
 	return True;
 }
 
+
+/**
+ * In developer builds, clobber a region of memory.
+ *
+ * If we think a string buffer is longer than it really is, this ought
+ * to make the failure obvious, by segfaulting (if in the heap) or by
+ * killing the return address (on the stack), or by trapping under a
+ * memory debugger.
+ *
+ * This is meant to catch possible string overflows, even if the
+ * actual string copied is not big enough to cause an overflow.
+ **/
+void clobber_region(char *dest, size_t len)
+{
+#ifdef DEVELOPER
+	/* F1 is odd and 0xf1f1f1f1 shouldn't be a valid pointer */
+	memset(dest, 0xF1, len);
+#endif
+}
+
+
 /**
  Safe string copy into a known length string. maxlength does not
  include the terminating zero.
@@ -444,13 +465,7 @@ char *safe_strcpy(char *dest,const char 
 		return NULL;
 	}
 
-#ifdef DEVELOPER
-	/* We intentionally write out at the extremity of the destination
-	 * string.  If the destination is too short (e.g. pstrcpy into mallocd
-	 * or fstring) then this should cause an error under a memory
-	 * checker. */
-	dest[maxlength] = '\0';
-#endif
+	clobber_region(dest, maxlength+1);
 
 	if (!src) {
 		*dest = 0;
@@ -499,6 +514,8 @@ char *safe_strcat(char *dest, const char
 		dest[maxlength] = 0;
 		return NULL;
 	}
+
+	clobber_region(dest + dest_len, maxlength - src_len);
 	
 	memcpy(&dest[dest_len], src, src_len);
 	dest[dest_len + src_len] = 0;
@@ -516,6 +533,8 @@ char *alpha_strcpy(char *dest, const cha
 {
 	size_t len, i;
 
+	clobber_region(dest, maxlength);
+
 	if (!dest) {
 		DEBUG(0,("ERROR: NULL dest in alpha_strcpy\n"));
 		return NULL;
@@ -554,8 +573,12 @@ char *alpha_strcpy(char *dest, const cha
 char *StrnCpy(char *dest,const char *src,size_t n)
 {
 	char *d = dest;
+
+	clobber_region(dest, n+1);
+	
 	if (!dest)
 		return(NULL);
+	
 	if (!src) {
 		*dest = 0;
 		return(dest);
@@ -576,6 +599,8 @@ char *strncpyn(char *dest, const char *s
 	char *p;
 	size_t str_len;
 
+	clobber_region(dest, n+1);
+
 	p = strchr_m(src, c);
 	if (p == NULL) {
 		DEBUG(5, ("strncpyn: separator character (%c) not found\n", c));


-- 
Martin