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

From: Derrick Stolee <derrickstolee@github.com>
Date: Mar 28, 2022, 17:55 UTC
Message-ID: <3bb95e8b-4977-ddca-225c-5afe28d8ac20@github.com>
In-Reply-To: <xmqqee2p70c1.fsf@gitster.g>

On 3/26/2022 1:24 AM, Junio C Hamano wrote:

Show 37 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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