[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [bug-patch] Broken patch commit: don't warn twice about the same inv
From: |
Jim Meyering |
Subject: |
Re: [bug-patch] Broken patch commit: don't warn twice about the same invalid file name |
Date: |
Fri, 06 Apr 2012 15:25:30 +0200 |
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))
Re: [bug-patch] Broken patch commit: don't warn twice about the same invalid file name, Jim Meyering, 2012/04/06
Re: [bug-patch] Broken patch commit: don't warn twice about the same invalid file name, Jean Delvare, 2012/04/10