[RFC 2/2] Make misuse of get_pathname() buffers detectable by valgrind
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
Michael Haggerty