{"thread":{"id":"28508","subject":"[RFC 0/2] Debugging tools for get_pathname()","startedAt":"2011-09-27T04:28:05Z","lastAt":"2011-11-16T14:18:53Z","messageCount":5,"participants":["Michael Haggerty","Nguyen Thai Ngoc Duy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"176304","messageId":"1317097687-11098-1-git-send-email-mhagger@alum.mit.edu","threadId":"28508","inReplyTo":null,"subject":"[RFC 0/2] Debugging tools for get_pathname()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-27T04:28:05Z","receivedAt":"2011-09-27T04:28:05Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"I just wasted a bunch of time diagnosing a problem that was caused by\nother code's misuse of a temporary buffer created by get_pathname()\n(patch forthcoming).  The bug was triggered by totally unrelated (and\ninnocent) code changes that I had made.  I found it quite difficult to\nfigure out the source of the problem.\n\nIt is probably obvious to everybody else, but here are the two tricks\nthat finally enabled me to diagnose the problem:\n\n1. I increased the number of temporary buffers available to\n   get_pathname().  This made the test suite failure go away, which\n   was a good indication that the problem was related to the buffers.\n\n2. I changed get_pathname() to allocate and free the buffers instead\n   of using statically-allocated buffers.  This made it trivial to\n   find the offender using valgrind.\n\nThis patch series make it easier to apply these tricks.  I hope it\nhelps somebody else avoid the pain that I just experienced.\n\nMichael Haggerty (2):\n  Make the number of pathname buffers a compile-time constant\n  Make misuse of get_pathname() buffers detectable by valgrind\n\n path.c |   43 +++++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 41 insertions(+), 2 deletions(-)\n\n-- \n1.7.7.rc2\n"},{"id":"176305","messageId":"1317097687-11098-2-git-send-email-mhagger@alum.mit.edu","threadId":"28508","inReplyTo":"1317097687-11098-1-git-send-email-mhagger@alum.mit.edu","subject":"[RFC 1/2] Make the number of pathname buffers a compile-time constant","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-27T04:28:06Z","receivedAt":"2011-09-27T04:28:06Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Changing the number of available temporary buffers for get_pathname()\ncan help diagnose abuse of such buffers.  If a bug goes away when\nPATHNAME_BUFFER_COUNT is increased, then it might be due to a buffer\nbeing used after it has been recycled.  Similarly, if a bug appears\nwhen PATHNAME_BUFFER_COUNT is decreased, then there might be a latent\nproblem that could emerge after unrelated code changes.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n path.c |    7 +++++--\n 1 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git path.c path.c\nindex 6f3f5d5..6c4714d 100644\n--- path.c\n+++ path.c\n@@ -13,13 +13,16 @@\n #include \"cache.h\"\n #include \"strbuf.h\"\n \n+/* This must be a power of 2: */\n+#define PATHNAME_BUFFER_COUNT (1 << 2)\n+\n static char bad_path[] = \"/bad-path/\";\n \n static char *get_pathname(void)\n {\n-\tstatic char pathname_array[4][PATH_MAX];\n+\tstatic char pathname_array[PATHNAME_BUFFER_COUNT][PATH_MAX];\n \tstatic int index;\n-\treturn pathname_array[3 & ++index];\n+\treturn pathname_array[(PATHNAME_BUFFER_COUNT - 1) & ++index];\n }\n \n static char *cleanup_path(char *path)\n-- \n1.7.7.rc2\n"},{"id":"176306","messageId":"1317097687-11098-3-git-send-email-mhagger@alum.mit.edu","threadId":"28508","inReplyTo":"1317097687-11098-1-git-send-email-mhagger@alum.mit.edu","subject":"[RFC 2/2] Make misuse of get_pathname() buffers detectable by valgrind","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-27T04:28:07Z","receivedAt":"2011-09-27T04:28:07Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"A temporary buffer produced by get_pathname() is recycled after a few\nsubsequent calls of get_pathname().  The use of such a buffer after it\nhas been recycled can result in the wrong file being accessed with\nvery strange effects.  Moreover, such a bug can lie dormant until code\nelsewhere is changed to use a temporary buffer, causing very\nmysterious, nonlocal failures that are hard to analyze.\n\nAdd a second implementation of get_pathname() (activated if the\nVALGRIND preprocessor macro is defined) that allocates and frees\nbuffers instead of recycling statically-allocated buffers.  This does\nnot make the problem less serious, but it turns the errors into\naccess-after-free errors, making it possible to locate the guilty code\nusing valgrind.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n\nI believe that it is frowned upon to use #ifdefs in git code, but no\ngood alternative is obvious to me for this type of use.  Suggestions\nare welcome.\n\nI would also welcome suggestions for a better name than \"VALGRIND\" for\nthe preprocessor macro.  Are there standard names used elsewhere in\ngit for such purposes?\n\n path.c |   40 ++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 38 insertions(+), 2 deletions(-)\n\ndiff --git path.c path.c\nindex 6c4714d..3021207 100644\n--- path.c\n+++ path.c\n@@ -9,6 +9,20 @@\n  *   f = open(mkpath(\"%s/%s.git\", base, name), O_RDONLY);\n  *\n  * which is what it's designed for.\n+ *\n+ * The temporary buffers returned by these functions will be clobbered\n+ * by later calls to these functions.  Therefore it is important not\n+ * to expect such buffers to keep their values across calls to other\n+ * git functions.  Violations of this rule can cause the original\n+ * buffer to be overwritten and lead to very confusing, nonlocal bugs,\n+ * including data loss (you think you are writing to your file but are\n+ * actually writing to a filename created by some other caller).\n+ *\n+ * If the VALGRIND preprocessor macro is defined, then buffers are\n+ * created via xmalloc and old temporary buffers are recycled using\n+ * free().  This changes the symptom of abuse of the buffers from\n+ * mysterious, random errors into access-after-free errors that are\n+ * detectable by valgrind.\n  */\n #include \"cache.h\"\n #include \"strbuf.h\"\n@@ -17,12 +31,34 @@\n #define PATHNAME_BUFFER_COUNT (1 << 2)\n \n static char bad_path[] = \"/bad-path/\";\n+#ifdef VALGRIND\n+static char buggy_path[] = \"/git-internal-error/\";\n+#endif\n \n static char *get_pathname(void)\n {\n-\tstatic char pathname_array[PATHNAME_BUFFER_COUNT][PATH_MAX];\n \tstatic int index;\n-\treturn pathname_array[(PATHNAME_BUFFER_COUNT - 1) & ++index];\n+#ifdef VALGRIND\n+\tstatic char *pathname_array[PATHNAME_BUFFER_COUNT];\n+\tindex = (index + 1) & (PATHNAME_BUFFER_COUNT - 1);\n+\tif (pathname_array[index]) {\n+\t\t/*\n+\t\t * In a correct program, this will have no effect, but\n+\t\t * *if* somebody erroneously uses this buffer after it\n+\t\t * has been freed, it gives more of a chance that the\n+\t\t * error will be detected even if valgrind is not\n+\t\t * running:\n+\t\t */\n+\t\tstrcpy(pathname_array[index], buggy_path);\n+\n+\t\tfree(pathname_array[index]);\n+\t}\n+\tpathname_array[index] = xmalloc(PATH_MAX);\n+\treturn pathname_array[index];\n+#else\n+\tstatic char pathname_array[PATHNAME_BUFFER_COUNT][PATH_MAX];\n+ \treturn pathname_array[(PATHNAME_BUFFER_COUNT - 1) & ++index];\n+#endif\n }\n \n static char *cleanup_path(char *path)\n-- \n1.7.7.rc2\n"},{"id":"179590","messageId":"CACsJy8CG0A1kUUyA=1AW+3cNTjrgb_iKjdfs7M8Sa6s0cuDANw@mail.gmail.com","threadId":"28508","inReplyTo":"1317097687-11098-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [RFC 2/2] Make misuse of get_pathname() buffers detectable by valgrind","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-16T13:57:52Z","receivedAt":"2011-11-16T13:57:52Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Sep 27, 2011 at 11:28 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> A temporary buffer produced by get_pathname() is recycled after a few\n> subsequent calls of get_pathname().  The use of such a buffer after it\n> has been recycled can result in the wrong file being accessed with\n> very strange effects.  Moreover, such a bug can lie dormant until code\n> elsewhere is changed to use a temporary buffer, causing very\n> mysterious, nonlocal failures that are hard to analyze.\n>\n> Add a second implementation of get_pathname() (activated if the\n> VALGRIND preprocessor macro is defined) that allocates and frees\n> buffers instead of recycling statically-allocated buffers.  This does\n> not make the problem less serious, but it turns the errors into\n> access-after-free errors, making it possible to locate the guilty code\n> using valgrind.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>\n> I believe that it is frowned upon to use #ifdefs in git code, but no\n> good alternative is obvious to me for this type of use.  Suggestions\n> are welcome.\n\nEnable the code based on an environment variable, e.g.\nGIT_DEBUG_FENCE, then enable it by default in test-lib.sh :-)\n-- \nDuy\n"},{"id":"179591","messageId":"CACsJy8BK3zvxA5mpC+3UvYMe20X6iFWJ7JL+VH6fr-Xstw2usg@mail.gmail.com","threadId":"28508","inReplyTo":"1317097687-11098-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [RFC 2/2] Make misuse of get_pathname() buffers detectable by valgrind","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-16T14:18:53Z","receivedAt":"2011-11-16T14:18:53Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Sep 27, 2011 at 11:28 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> +#ifdef VALGRIND\n> +       static char *pathname_array[PATHNAME_BUFFER_COUNT];\n> +       index = (index + 1) & (PATHNAME_BUFFER_COUNT - 1);\n> +       if (pathname_array[index]) {\n> +               /*\n> +                * In a correct program, this will have no effect, but\n> +                * *if* somebody erroneously uses this buffer after it\n> +                * has been freed, it gives more of a chance that the\n> +                * error will be detected even if valgrind is not\n> +                * running:\n> +                */\n> +               strcpy(pathname_array[index], buggy_path);\n> +\n> +               free(pathname_array[index]);\n> +       }\n> +       pathname_array[index] = xmalloc(PATH_MAX);\n> +       return pathname_array[index];\n> +#else\n\nNot sure if it works (just read man pages, haven't tried anything) I'm\nthinking to use mmap() with MAP_ANONYMOUS instead of xmalloc(), then\nmprotect() instead of free() to remove read access from that area. Any\naccess after that should be caught. Leaking may not be severe for\ngit_path(), hopefully.\n-- \nDuy\n"}]}