Re: Fwd: New Defects reported by Coverity Scan for git

[Junio C Hamano <gitster@pobox.com>]

René Scharfe <l.s.r@web.de> 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@web.de>
> ---

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;