René Scharfe <l.s.r@xxxxxx> 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 <l.s.r@xxxxxx>
> ---
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 ;-)
> 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;
