Re: [PATCH v5 02/16] reftable: fix resource leak in block.c error path

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 24, 2021, 04:16 UTC
Message-ID: <xmqq4k6y63j7.fsf@gitster.g>
In-Reply-To: <CAFQ2z_OLCzOYXgXCTXyLOwwk7EBkPzwH=KASDmuJbur=q7L1Jg@mail.gmail.com>

Han-Wen Nienhuys <hanwen@google.com> writes:

> Normally, block_reader_init() transfers the block to the block_reader.
> If err > 0, we skip that, so we'd be leaking the block.

Thanks.  I was about to say that

    <reftable/block.h> wants to say more than "initializes a block
    reader."; perhaps "returns the number of blocks" or something
    needs to be added.

but I do not think the lack of the documentation was the source of my confusion.

> At the same time, this means the "if (err)" is superfluous. In the
> success case, the block was transferred to the block_reader, so the
> reftable_block_done() call is a nop.

I agree that having the "if (err)" there is highly misleading. "if (err < 0 || 0 < err)" would be the more faithful expression of what you explained, and if it were written that way, I wouldn't have been as confused as I was.

In any case, if _done() becomes a safe and quick no-op when 0 block was transferred, losing the conditional would be the way to make the resulting code the most readable, I think.

Show 9 quoted lines

>> > +     uint8_t hash1[GIT_SHA1_RAWSZ] = { 1 };
>> > +     uint8_t hash2[GIT_SHA1_RAWSZ] = { 2 };
>>
>> Will this code be exercised when compiling with SHA256 support?  If
>> not, this is perfectly fine, but otherwise, this needs to be MAX,
>> not SHA1, no?
>
> The code is parameterized in terms of hash_size, so we don't have to
> test both flavors exhaustively.

If it is safe for this function to assume that it will be called only with hash_size of SHA1, then what you wrote is perfectly fine, and I agree that there is no particular reason why this test must be repeated for all available hashes.

I just don't know if we have a mechanism to prevent clueless code churners from saying "let's test both flavors exhaustively". For example, I see nothing in this function, other than the size of these two hashes, that says that this test function will be compiled with reftable library that expects that the hash function is SHA1. How does this function protect itself from being run in a CI job that uses GIT_TEST_DEFAULT_HASH=sha256, for example? It was why I asked the question. It wasn't that I necessarily thought we need to test all combinations (but there is no need not to, if it takes more engineering time to exclude it when testing Git as a whole in sha256 mode).

Show 8 quoted lines

>> > +     char message[100] = { 0 };
>>
>> You're filling this to the sizeof(message)-1, so we can afford to
>> leave it uninitialized.
>
> At the same time, we can afford to initialize it :-)
>
> I'd rather not think about this, and always initialize everything.

I do not care too deeply in this test-only function, but as a general principle, primarily to help less experienced developers who may be reading from the sidelines to avoid copying such an attitude, I have to make a note here.

If you know you will have to fill an array with, or assign to a variable, meaningful value(s), leaving the array or variable uninitialized at the declaration time is a much better thing to do than initializing them with less meaningful value(s). It will help compilers and tools to detect a lack of proper assignment and use of uninitialized variable (iow, you may know you will have to fill, but in the code to do so, your implementation may be botched). Once you initialize at the declaration with "less meaningful" value (like zero initialization), the tools won't be able to tell when the code uses that variable "uninitialized" (because the assignment was skipped by a bug), since it appears to always be initialied to them.

Helping the tools help us is what those of us, who would rather not think about it, should be doing.