clean: fix theoretical path corruption

newren · gitster · commit 902b90cf42bc · 2019-09-17T12:20:36.000-07:00
cmd_clean() had the following code structure: struct strbuf abs_path = STRBUF_INIT; for_each_string_list_item(item, &del_list) { strbuf_addstr(&abs_path, prefix); strbuf_addstr(&abs_path, item->string); PROCESS(&abs_path); strbuf_reset(&abs_path); } where I've elided a bunch of unnecessary details and PROCESS(&abs_path) represents a big chunk of code rather than an actual function call. One piece of PROCESS was: if (lstat(abs_path.buf, &st)) continue; which would cause the strbuf_reset() to be missed -- meaning that the next path to be handled would have two paths concatenated. This path used to use die_errno() instead of continue prior to commit 396049e ("git-clean: refactor git-clean into two phases", 2013-06-25), but my understanding of how correct_untracked_entries() works is that it will prevent both dir/ and dir/file from being in the list to clean so this should be dead code and the die_errno() should be safe. But I hesitate to remove it since I am not certain. However, we can fix both this bug and possible similar future bugs by simply moving the strbuf_reset(&abs_path) to the beginning of the loop. It'll result in N calls to strbuf_reset() instead of N-1, but that's a small price to pay to avoid sneaky bugs like this. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
diff --git a/builtin/clean.c b/builtin/clean.c
@@ -1018,6 +1018,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	for_each_string_list_item(item, &del_list) {
 		struct stat st;
 
+		strbuf_reset(&abs_path);
 		if (prefix)
 			strbuf_addstr(&abs_path, prefix);
 
@@ -1051,7 +1052,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 				printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
 			}
 		}
-		strbuf_reset(&abs_path);
 	}
 
 	strbuf_release(&abs_path);

Original file line number	Diff line number	Diff line change
`@@ -1018,6 +1018,7 @@ int cmd_clean(int argc, const char *argv, const char prefix)`
`1018`	`1018`	`for_each_string_list_item(item, &del_list) {`
`1019`	`1019`	`struct stat st;`
`1020`	`1020`
	`1021`	`+ strbuf_reset(&abs_path);`
`1021`	`1022`	`if (prefix)`
`1022`	`1023`	`strbuf_addstr(&abs_path, prefix);`
`1023`	`1024`
`@@ -1051,7 +1052,6 @@ int cmd_clean(int argc, const char *argv, const char prefix)`
`1051`	`1052`	`printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);`
`1052`	`1053`	`}`
`1053`	`1054`	`}`
`1054`		`- strbuf_reset(&abs_path);`
`1055`	`1055`	`}`
`1056`	`1056`
`1057`	`1057`	`strbuf_release(&abs_path);`