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

On Wed, Dec 22, 2021 at 11:51 PM Junio C Hamano [off-list ref] wrote:

quoted

+     if (err)

Is the convention for reader_init() different from all other
functions?  It makes reader wonder why this is not

        if (err < 0)

quoted

+             reftable_block_done(&block);

even though it is not wrong per-se (as long as "zero means success"
is a part of the return value convention).

err > 0 is returned when we reach the end of the iteration, and this
function can generate err==1.

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

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.

quoted

+
+     return err;
 }

This one is new in this round.  All look good, other than that one
check for error return.

quoted

diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
index 5f6bcc2f775..6e88182a83a 100644
--- a/reftable/readwrite_test.c
+++ b/reftable/readwrite_test.c

@@ -254,6 +254,71 @@ static void test_log_write_read(void)
      reader_close(&rd);
 }

+static void test_log_zlib_corruption(void)
+{
+     struct reftable_write_options opts = {
+             .block_size = 256,
+     };
+     struct reftable_iterator it = { 0 };
+     struct reftable_reader rd = { 0 };
+     struct reftable_block_source source = { 0 };
+     struct strbuf buf = STRBUF_INIT;
+     struct reftable_writer *w =
+             reftable_new_writer(&strbuf_add_void, &buf, &opts);
+     const struct reftable_stats *stats = NULL;
+     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. There is a
test_table_read_write_seek_linear_sha256() that ensures that the basic
functionality works for SHA256.

quoted

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

quoted

+     for (i = 0; i < sizeof(message)-1; i++)

Style: SP around "-" on both sides.

done.

(I assume I don't have to resend the whole series for these small
tweaks? I'll wait if anyone else has comments, and send a reroll early
January)
-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado