Subject: Re: [PATCH v3 08/27] revisions API users: add "goto cleanup" for "rev_info" early exit

From: Derrick Stolee <[email-masked]>
Date: Mon, 28 Mar 2022 13:55:07 -0400

On 3/26/2022 1:24 AM, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <[email-masked]> writes:
> 
>> Because I don't see how it makes any sense to have a REV_INFO_INIT if it
>> doesn't actually give you an init'd "struct rev_info" that's ready for
>> use. I.e. if you still need to call repo_init_revisions() it's just a
>> misleading interface.
> 
> You can say
> 
> 	struct something *foo = NULL;
> 
> 	if (some condition)
> 		foo = alloc_and_init_foo();
> 
> 	...
> 
> 	free_and_destruct(foo);
> 
> and it is correct that "initialize with NULL" alone would not let
> you use the thing as full fledged 'foo', but you can still safely
> pass it to free_and_destruct() (or if "something" does not hold
> external resources, it could just be free()).  A flow like this:
> 
> 	struct rev_info revs = REV_INFO_INIT;
> 
> 	if (!some condition)
> 		goto leave;
> 	init(&revs);
> 	... use revs ...
> leave:
>         release(&revs);
> 
> would exactly be the same thing.
> 
> In other words, you say "I do not see how it makes any sense" but to
> me it looks more like what does not make sense is your argument
> against what was suggested.

Ævar has stated in multiple threads that he prefers to not
initialize data so that static analysis tools can detect a
use-before-initialization of specific members.

However, now that we are intending to free rev_info structs,
we need them to be initialized with NULL pointers because
otherwise the release_revisions() method won't know which
portions were legitimately initialized and which ones were
not.

Maybe this NULL assignment happens as part of
repo_init_revisions(), but that also assumes that there is no
code path that would jump to a "leave" or "cleanup" tag before
running that initialization method (which is the broken case
that Junio mentions above).

Maybe there are tools that would identify that Junio's example
would be bad, but it is also likely that a compiler doesn't
catch that issue and tests don't cover that error condition.

It's my preference to initialize things to all-zeroes whenever
there is any chance of complexity, which is why this topic has
come to my attention on multiple threads.

Thanks,
-Stolee