Re: [PATCH v5 02/16] reftable: fix resource leak in block.c error path
- From
Han-Wen Nienhuys <hanwen@google.com>
- Date
- Dec 23, 2021, 17:04 UTC
- Message-ID
- <CAFQ2z_OLCzOYXgXCTXyLOwwk7EBkPzwH=KASDmuJbur=q7L1Jg@mail.gmail.com>
- In-Reply-To
- <xmqqtuf0fe3r.fsf@gitster.g>
On Wed, Dec 22, 2021 at 11:51 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> > + if (err) > > Is the convention for reader_init() different from all other > functions? It makes reader wonder why this is not > > if (err < 0) > > + 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.
Show 34 quoted lines
>
> > +
> > + return err;
> > }
>
> This one is new in this round. All look good, other than that one
> check for error return.
>
> > 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.
> > + 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.
> > + 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