Re: [RFC PATCH 05/15] refs/packed-backend.c: add a BUG() if iter is NULL

From: René Scharfe <l.s.r@web.de>
Date: Jun 3, 2022, 23:14 UTC
Message-ID: <5b3e0d7c-e507-2edc-0cbc-5dd54883a8b3@web.de>
In-Reply-To: <RFC-patch-05.15-46e0c307941-20220603T183608Z-avarab@gmail.com>

Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:

Show 17 quoted lines

> Adjust code added in 2775d8724d7 (packed_ref_store: implement
> reference transactions, 2017-09-08) to BUG() out in a case there GCC
> v12's -fanalyzer flagged that the "iter->oid" seen in the context was
> reachable where iter was NULL.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  refs/packed-backend.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 97b68377673..65991bbcaf5 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -1226,6 +1226,8 @@ static int write_with_updates(struct packed_ref_store *refs,
>  			struct object_id peeled;
>  			int peel_error = ref_iterator_peel(iter, &peeled);

Some peel backends dereference iter, e.g. packed_ref_iterator_peel() from the same file.

>
> +			if (!iter)
> +				BUG("must have iter if cmp < 0");

So if iter can be NULL at this point, then it must be checked before the ref_iterator_peel() call above, no?

*Can* it be NULL, though? The code in question is a bit complicated and it's late around here, but I cannot see how cmp < 0 and !iter can both be true at this place in the code. An optimizing compiler will probably optimize out the added check and string. Mine does (clang 13); you can check if yours does the same using:

  $ make refs/packed-backend.o && grep "must have" refs/packed-backend.o

>  			if (write_packed_entry(out, iter->refname,
>  					       iter->oid,
>  					       peel_error ? NULL : &peeled))