From c189c4f2c42083b329605fb7b0583b29b73da086 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Thu, 8 Aug 2013 20:31:44 +0200 Subject: [PATCH 1/3] diff: remove ternary operator evaluating always to true The line being changed is deep inside the function builtin_diff. The variable name_b, which is used to evaluate the ternary expression must evaluate to true at that position, hence the replacement with just name_b. The name_b variable only occurs a few times in that lengthy function: As a parameter to the function itself: static void builtin_diff(const char *name_a, const char *name_b, ... The next occurrences are at: /* Never use a non-valid filename anywhere if at all possible */ name_a = DIFF_FILE_VALID(one) ? name_a : name_b; name_b = DIFF_FILE_VALID(two) ? name_b : name_a; a_one = quote_two(a_prefix, name_a + (*name_a == '/')); b_two = quote_two(b_prefix, name_b + (*name_b == '/')); In the last line of this block 'name_b' is dereferenced and compared to '/'. This would crash if name_b was NULL. Hence in the following code we can assume name_b being non-null. The next occurrence is just as a function argument, which doesn't change the memory, which name_b points to, so the assumption name_b being not null still holds: emit_rewrite_diff(name_a, name_b, one, two, textconv_one, textconv_two, o); The next occurrence would be the line of this patch. As name_b still must be not null, we can remove the ternary operator. Inside the emit_rewrite_diff function there is a also a line ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a); which was also simplified as there is also a dereference before the ternary operator. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- diff.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/diff.c b/diff.c index 266112ca61..80f8439441 100644 --- a/diff.c +++ b/diff.c @@ -669,7 +669,7 @@ static void emit_rewrite_diff(const char *name_a, memset(&ecbdata, 0, sizeof(ecbdata)); ecbdata.color_diff = want_color(o->use_color); ecbdata.found_changesp = &o->found_changes; - ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a); + ecbdata.ws_rule = whitespace_rule(name_b); ecbdata.opt = o; if (ecbdata.ws_rule & WS_BLANK_AT_EOF) { mmfile_t mf1, mf2; @@ -2372,7 +2372,7 @@ static void builtin_diff(const char *name_a, ecbdata.label_path = lbl; ecbdata.color_diff = want_color(o->use_color); ecbdata.found_changesp = &o->found_changes; - ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a); + ecbdata.ws_rule = whitespace_rule(name_b); if (ecbdata.ws_rule & WS_BLANK_AT_EOF) check_blank_at_eof(&mf1, &mf2, &ecbdata); ecbdata.opt = o; From 3b0c18af5c545c93bf33b53d467b887e9733c0c6 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Fri, 9 Aug 2013 00:11:53 +0200 Subject: [PATCH 2/3] diff: fix a possible null pointer dereference The condition in the ternary operator was wrong, hence the wrong char pointer could be used as the parameter for show_submodule_summary. one->path may be null, but we definitely need a non null path given to the function. Signed-off-by: Stefan Beller Acked-By: Johannes Schindelin Signed-off-by: Junio C Hamano --- diff.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/diff.c b/diff.c index 80f8439441..061694b5ea 100644 --- a/diff.c +++ b/diff.c @@ -2252,7 +2252,7 @@ static void builtin_diff(const char *name_a, (!two->mode || S_ISGITLINK(two->mode))) { const char *del = diff_get_color_opt(o, DIFF_FILE_OLD); const char *add = diff_get_color_opt(o, DIFF_FILE_NEW); - show_submodule_summary(o->file, one ? one->path : two->path, + show_submodule_summary(o->file, one->path ? one->path : two->path, line_prefix, one->sha1, two->sha1, two->dirty_submodule, meta, del, add, reset); From f8aae0b51743d5e5f35292caee41e5215b0e5bad Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Thu, 8 Aug 2013 19:52:41 +0200 Subject: [PATCH 3/3] rm: remove unneeded null pointer check As of 7612a1efdb (2006-06-09 git-rm: honor -n flag.) the variable 'pathspec' seems to be assumed to be never NULL after calling get_pathspec There was a NULL pointer check after the seen = NULL assignment, which was removed by that commit. So if pathspec would be NULL now, we'd segfault in the line accessing the pathspec: for (i = 0; pathspec[i] ; i++) A few lines later, 'pathspec' still cannot be NULL, but that check was overlooked, hence removing it now. As the null pointer check was removed, it makes no sense to assign NULL to seen and 3 lines later another value as there are no conditions in between. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/rm.c | 40 ++++++++++++++++++---------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/builtin/rm.c b/builtin/rm.c index 0df0b4d942..d00eaf86d5 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -277,8 +277,8 @@ static struct option builtin_rm_options[] = { int cmd_rm(int argc, const char **argv, const char *prefix) { - int i, newfd; - const char **pathspec; + int i, newfd, seen_any; + const char **pathspec, *match; char *seen; git_config(git_default_config, NULL); @@ -314,7 +314,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix) pathspec = get_pathspec(prefix, argv); refresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL); - seen = NULL; for (i = 0; pathspec[i] ; i++) /* nothing */; seen = xcalloc(i, 1); @@ -328,27 +327,24 @@ int cmd_rm(int argc, const char **argv, const char *prefix) list.entry[list.nr++].is_submodule = S_ISGITLINK(ce->ce_mode); } - if (pathspec) { - const char *match; - int seen_any = 0; - for (i = 0; (match = pathspec[i]) != NULL ; i++) { - if (!seen[i]) { - if (!ignore_unmatch) { - die(_("pathspec '%s' did not match any files"), - match); - } - } - else { - seen_any = 1; - } - if (!recursive && seen[i] == MATCHED_RECURSIVELY) - die(_("not removing '%s' recursively without -r"), - *match ? match : "."); - } - if (! seen_any) - exit(0); + seen_any = 0; + for (i = 0; (match = pathspec[i]) != NULL ; i++) { + if (!seen[i]) { + if (!ignore_unmatch) { + die(_("pathspec '%s' did not match any files"), + match); + } + } + else { + seen_any = 1; + } + if (!recursive && seen[i] == MATCHED_RECURSIVELY) + die(_("not removing '%s' recursively without -r"), + *match ? match : "."); } + if (!seen_any) + exit(0); /* * If not forced, the file, the index and the HEAD (if exists)