On Mon, Mar 28 2022, Derrick Stolee wrote:
> On 3/26/2022 1:24 AM, Junio C Hamano wrote:
>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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]
> 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@xxxxxxxxxxxxxxxxxxx/
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@xxxxxxxxx/
2. https://lore.kernel.org/git/patch-v2-1.1-9951d92176e-20220328T154049Z-avarab@xxxxxxxxx/
