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@xxxxxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html