Re: Fwd: New Defects reported by Coverity Scan for git
From: Junio C Hamano <hidden>
Date: 2017-07-20 21:52:48
René Scharfe [off-list ref] writes:
We could remove that NULL check -- it's effectively just a shortcut. But how would that improve safety? Well, if the array is unallocated (NULL) and _num is greater than zero we'd get a segfault without it, and thus would notice it. That check currently papers over such a hypothetical bug. Makes sense? -- >8 -- Subject: [PATCH] pack-objects: remove unnecessary NULL check If done_pbase_paths is NULL then done_pbase_paths_num must be zero and done_pbase_path_pos() returns -1 without accessing the array, so the check is not necessary. If the invariant was violated then the check would make sure we keep on going and allocate the necessary amount of memory in the next ALLOC_GROW call. That sounds nice, but all array entries except for one would contain garbage data. If the invariant was violated without the check we'd get a segfault in done_pbase_path_pos(), i.e. an observable crash, alerting us of the presence of a bug. Currently there is no such bug: Only the functions check_pbase_path() and cleanup_preferred_base() change pointer and counter, and both make sure to keep them in sync. Get rid of the check anyway to allow us to see if later changes introduce such a defect, and to simplify the code. Detected by Coverity Scan. Signed-off-by: Rene Scharfe <redacted> ---
It's always amusing to see that a removal of conditional codepath would result in better chance of finding possible invariant breakers, as we often think that belt-and-suspenders safety would require more conditionals and asserts ;-)
quoted hunk
builtin/pack-objects.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index e730b415bf..c753e9237a 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c@@ -1289,7 +1289,7 @@ static int done_pbase_path_pos(unsigned hash) static int check_pbase_path(unsigned hash) { - int pos = (!done_pbase_paths) ? -1 : done_pbase_path_pos(hash); + int pos = done_pbase_path_pos(hash); if (0 <= pos) return 1; pos = -pos - 1;