Jean, Am 6. April 2012 14:09 schrieb Jean Delvare <[email protected]>: > 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 <[email protected]> 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. --- src/common.h | 1 + src/pch.c | 52 +++++++++++++++++++++++++++++++--------------------- 2 files changed, 32 insertions(+), 21 deletions(-) diff --git a/src/common.h b/src/common.h index d91f696..bc93e59 100644 --- a/src/common.h +++ b/src/common.h @@ -62,6 +62,7 @@ #define strEQ(s1,s2) (!strcmp(s1, s2)) #define strnEQ(s1,s2,l) (!strncmp(s1, s2, l)) +#define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0])) /* typedefs */ diff --git a/src/pch.c b/src/pch.c index 6b0605e..a07f291 100644 --- a/src/pch.c +++ b/src/pch.c @@ -42,6 +42,7 @@ static int p_says_nonexistent[2]; /* [0] for old file, [1] for new: 2 for nonexistent */ static int p_rfc934_nesting; /* RFC 934 nesting level */ static char *p_name[3]; /* filenames in patch headers */ +static char const *invalid_names[2]; bool p_copy[2]; /* Does this patch create a copy? */ bool p_rename[2]; /* Does this patch rename a file? */ static char *p_timestr[2]; /* timestamps as strings */ @@ -379,34 +380,41 @@ skip_hex_digits (char const *str) static bool name_is_valid (char const *name) { - static char const *bad[2]; char const *n; + int i; + bool is_valid = true; - if (bad[0] && ! strcmp (bad[0], name)) - return false; - if (bad[1] && ! strcmp (bad[1], name)) - return false; + for (i = 0; i < ARRAY_SIZE (invalid_names); i++) + { + if (! invalid_names[i]) + break; + if (! strcmp (invalid_names[i], name)) + return false; + } if (IS_ABSOLUTE_FILE_NAME (name)) + is_valid = false; + else + for (n = name; *n; ) + { + if (*n == '.' && *++n == '.' && ( ! *++n || ISSLASH (*n))) + { + is_valid = false; + break; + } + while (*n && ! ISSLASH (*n)) + n++; + while (ISSLASH (*n)) + n++; + } + + if (! is_valid) { say ("Ignoring potentially dangerous file name %s\n", quotearg (name)); - bad[!! bad[0]] = 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; - return false; - } - while (*n && ! ISSLASH (*n)) - n++; - while (ISSLASH (*n)) - n++; + if (i < ARRAY_SIZE (invalid_names)) + invalid_names[i] = name; } - return true; + return is_valid; } /* Determine what kind of diff is in the remaining part of the patch file. */ @@ -434,6 +442,8 @@ intuit_diff_type (bool need_header, mode_t *p_file_type) free (p_name[i]); p_name[i] = 0; } + for (i = 0; i < ARRAY_SIZE (invalid_names); i++) + invalid_names[i] = NULL; for (i = OLD; i <= NEW; i++) if (p_timestr[i]) { -- 1.7.7.6
