[RFC 2/2] Make misuse of get_pathname() buffers detectable by valgrind

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Sep 27, 2011, 04:28 UTC
Message-ID: <1317097687-11098-3-git-send-email-mhagger@alum.mit.edu>
In-Reply-To: <1317097687-11098-1-git-send-email-mhagger@alum.mit.edu>

A temporary buffer produced by get_pathname() is recycled after a few subsequent calls of get_pathname(). The use of such a buffer after it has been recycled can result in the wrong file being accessed with very strange effects. Moreover, such a bug can lie dormant until code elsewhere is changed to use a temporary buffer, causing very mysterious, nonlocal failures that are hard to analyze.

Add a second implementation of get_pathname() (activated if the VALGRIND preprocessor macro is defined) that allocates and frees buffers instead of recycling statically-allocated buffers. This does not make the problem less serious, but it turns the errors into access-after-free errors, making it possible to locate the guilty code using valgrind.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

I believe that it is frowned upon to use #ifdefs in git code, but no good alternative is obvious to me for this type of use. Suggestions are welcome.

I would also welcome suggestions for a better name than "VALGRIND" for the preprocessor macro. Are there standard names used elsewhere in git for such purposes?

 path.c |   40 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 38 insertions(+), 2 deletions(-)

diff --git path.c path.c
index 6c4714d..3021207 100644
--- path.c
+++ path.c
@@ -9,6 +9,20 @@
  *   f = open(mkpath("%s/%s.git", base, name), O_RDONLY);
  *
  * which is what it's designed for.
+ *
+ * The temporary buffers returned by these functions will be clobbered
+ * by later calls to these functions.  Therefore it is important not
+ * to expect such buffers to keep their values across calls to other
+ * git functions.  Violations of this rule can cause the original
+ * buffer to be overwritten and lead to very confusing, nonlocal bugs,
+ * including data loss (you think you are writing to your file but are
+ * actually writing to a filename created by some other caller).
+ *
+ * If the VALGRIND preprocessor macro is defined, then buffers are
+ * created via xmalloc and old temporary buffers are recycled using
+ * free().  This changes the symptom of abuse of the buffers from
+ * mysterious, random errors into access-after-free errors that are
+ * detectable by valgrind.
  */
 #include "cache.h"
 #include "strbuf.h"
@@ -17,12 +31,34 @@
 #define PATHNAME_BUFFER_COUNT (1 << 2)
 
 static char bad_path[] = "/bad-path/";
+#ifdef VALGRIND
+static char buggy_path[] = "/git-internal-error/";
+#endif
 
 static char *get_pathname(void)
 {
-	static char pathname_array[PATHNAME_BUFFER_COUNT][PATH_MAX];
 	static int index;
-	return pathname_array[(PATHNAME_BUFFER_COUNT - 1) & ++index];
+#ifdef VALGRIND
+	static char *pathname_array[PATHNAME_BUFFER_COUNT];
+	index = (index + 1) & (PATHNAME_BUFFER_COUNT - 1);
+	if (pathname_array[index]) {
+		/*
+		 * In a correct program, this will have no effect, but
+		 * *if* somebody erroneously uses this buffer after it
+		 * has been freed, it gives more of a chance that the
+		 * error will be detected even if valgrind is not
+		 * running:
+		 */
+		strcpy(pathname_array[index], buggy_path);
+
+		free(pathname_array[index]);
+	}
+	pathname_array[index] = xmalloc(PATH_MAX);
+	return pathname_array[index];
+#else
+	static char pathname_array[PATHNAME_BUFFER_COUNT][PATH_MAX];
+ 	return pathname_array[(PATHNAME_BUFFER_COUNT - 1) & ++index];
+#endif
 }
 
 static char *cleanup_path(char *path)

-- 
1.7.7.rc2