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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 28, 2022, 18:55 UTC
Message-ID: <220328.86tubh3nmx.gmgdl@evledraar.gmail.com>
In-Reply-To: <3bb95e8b-4977-ddca-225c-5afe28d8ac20@github.com>

On Mon, Mar 28 2022, Derrick Stolee wrote:

Show 39 quoted lines

> On 3/26/2022 1:24 AM, Junio C Hamano wrote:
>> Æ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.
>

[re-arranging a bit]

Show 19 quoted lines

> 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.

Mine too, IOW I really don't like the version in my own v3 here, but wanted to just use an initialization to { 0 } as in the v2.

But I read Junio's reply to https://lore.kernel.org/git/220324.868rszmga6.gmgdl@evledraar.gmail.com/ as objecting to that general assumption, as we do e.g. in some of this code:

    git grep 'struct.*=.*\{ 0 \}'

So the v3 rewrite of those was seeking a way around that which didn't require implementing a full init-from-macro of REV_INFO_INIT.

Junio, would you be OK/prefer to basically have the v2 verison, with just a dummy macro like this in revision.h?:

    #define REV_INFO_INIT { 0 }

Which we'd then do something more useful with down the line?

> Æ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.

Just to be clear, I prefer not doing initialization in cases where the compiler is guaranteed to help you by skipping them, i.e. init to NULL within a function where say 2 branchesh both init the value, but if you were to change that the init to NULL would hide the bug.

(And I'm sure it's not something I came up with, but got pointed out to me repeatedly in review (by Jeff King or Junio?), until I got it...)

But skipping zero-ing out a struct like "struct rev_info" is not such a case where the compiler can really help, and we often use that struct at a far distance from where it was init'd. So I'd really like to see us not do that as a habit, even if say valgrind can sometimes catch it (but only if we have 100% test coverage!).

You're probably thinking more of the case of [1]. I consider that a pretty clear case of running with scissors, I just figured that one was narrow enough to be OK, but it's good to have [2] instead.

1. https://lore.kernel.org/git/patch-1.1-193534b0f07-20220325T121715Z-avarab@gmail.com/
2. https://lore.kernel.org/git/patch-v2-1.1-9951d92176e-20220328T154049Z-avarab@gmail.com/