Re: [bug-patch] Broken patch commit: don't warn twice about the same invalid file name

[Jim Meyering]

Andreas Grünbacher wrote:
> Am 6. April 2012 14:09 schrieb Jean Delvare <address@hidden>:
>> This commit [...] is broken.
>> bad[] holds pointers to the name strings, and these get freed
>> when switching to the next hunk in the patch file (if I read the code
>> properly...) So you end up accessing freed memory. Valgrind complains
>> about that and the code will eventually fail, as was reported by an
>> openSUSE user:
>>
>> https://bugzilla.novell.com/show_bug.cgi?id=755136
>>
>> I don't think the code even actually does what it is supposed to, as it
>> can only store 2 bad names (the slots are never freed), while a given
>> patch file could contain a lot more.
>
> Thanks. We are actually only interested in each patch here, not in all the
> patches that could be in the same input file. How about the attached change
> for a fix?
>
> Andreas
>
> From 593d7997706e523069e0f1c1a3330e9201aab442 Mon Sep 17 00:00:00 2001
> From: Andreas Gruenbacher <address@hidden>
> Date: Fri, 6 Apr 2012 14:43:33 +0200
> Subject: [PATCH] Fix use-after-free bug in name_is_valid()
>
> * src/common.h (ARRAY_SIZE): New macro.
> * src/pch.c (invalid_names): New global variable for remembering bad names.
> (intuit_diff_type): Reset invalid_names for each new patch; the names from the
> previous patch have already been freed.
> (name_is_valid): Use invalid_names.  Make the code "safer" and avoid
> duplication.

For the record, here's an alternative that's smaller and more local
(i.e., doesn't add a new global), at the expense of allocating a copy
of each invalid name on the heap.

I've verified that it avoids the problem and does not introduce a leak.

Andreas, if you prefer this one, let me know and I'll add comments
and prepare a complete patch.


diff --git a/src/pch.c b/src/pch.c
index 6b0605e..10968d0 100644
--- a/src/pch.c
+++ b/src/pch.c
@@ -23,6 +23,7 @@
 #include <dirname.h>
 #include <inp.h>
 #include <quotearg.h>
+#include <xalloc.h>
 #include <util.h>
 #undef XTERN
 #define XTERN
@@ -376,10 +377,19 @@ skip_hex_digits (char const *str)
   return s == str ? NULL : s;
 }

+static void
+add_bad (char *bad[], char const *name)
+{
+  char **p = &bad[!! bad[0]];
+  say ("Ignoring potentially dangerous file name %s\n", quotearg (name));
+  free (*p);
+  *p = xstrdup (name);
+}
+
 static bool
 name_is_valid (char const *name)
 {
-  static char const *bad[2];
+  static char *bad[2];
   char const *n;

   if (bad[0] && ! strcmp (bad[0], name))
@@ -389,16 +399,14 @@ name_is_valid (char const *name)

   if (IS_ABSOLUTE_FILE_NAME (name))
     {
-      say ("Ignoring potentially dangerous file name %s\n", quotearg (name));
-      bad[!! bad[0]] = name;
+      add_bad (bad, name);
       return false;
     }
   for (n = name; *n; )
     {
       if (*n == '.' && *++n == '.' && ( ! *++n || ISSLASH (*n)))
         {
-         say ("Ignoring potentially dangerous file name %s\n", quotearg 
(name));
-         bad[!! bad[0]] = name;
+         add_bad (bad, name);
          return false;
        }
       while (*n && ! ISSLASH (*n))