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

[Junio C Hamano <gitster@pobox.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.

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

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