Re: [PATCH 05/11] reftable/block: fix OOB write with bogus inflated log size

[Patrick Steinhardt <ps@pks.im>]

On Fri, Jun 26, 2026 at 09:48:36AM +0200, Christian Couder wrote:
> On Wed, Jun 24, 2026 at 10:24 AM Patrick Steinhardt <ps@pks.im> wrote:
> 
> > diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
> > index f4bded7d26..40274af5c0 100644
> > --- a/t/unit-tests/u-reftable-block.c
> > +++ b/t/unit-tests/u-reftable-block.c
> > @@ -456,3 +456,47 @@ void test_reftable_block__iterator(void)
> >         block_writer_release(&writer);
> >         reftable_buf_release(&data);
> >  }
> > +
> > +void test_reftable_block__corrupt_log_block_size(void)
> > +{
> > +       struct reftable_block_source source = { 0 };
> > +       struct block_writer writer = {
> > +               .last_key = REFTABLE_BUF_INIT,
> > +       };
> > +       struct reftable_record rec = {
> > +               .type = REFTABLE_BLOCK_TYPE_LOG,
> > +               .u.log = {
> > +                       .refname = (char *) "refs/heads/main",
> > +                       .update_index = 1,
> > +                       .value_type = REFTABLE_LOG_UPDATE,
> > +               },
> > +       };
> > +       struct reftable_block block = { 0 };
> > +       struct reftable_buf data;
> > +
> > +       data.len = 1024;
> > +       REFTABLE_CALLOC_ARRAY(data.buf, data.len);
> > +       cl_assert(data.buf != NULL);
> > +
> > +       cl_must_pass(block_writer_init(&writer, REFTABLE_BLOCK_TYPE_LOG,
> > +                                      (uint8_t *) data.buf, data.len,
> > +                                      0, hash_size(REFTABLE_HASH_SHA1)));
> > +       cl_must_pass(block_writer_add(&writer, &rec));
> > +       cl_assert(block_writer_finish(&writer) > 0);
> 
> It looks like some of the block writing code above could be simplified
> using an helper function like:
> 
> int cl_reftable_write_block(struct reftable_buf *buf, uint8_t block_type,
>                            size_t block_size, uint32_t header_off,
>                            struct reftable_record *recs, size_t nrecs)
> {
>        struct block_writer writer = {
>                .last_key = REFTABLE_BUF_INIT,
>        };
>        int block_end;
> 
>        REFTABLE_CALLOC_ARRAY(buf->buf, block_size);
>        cl_assert(buf->buf != NULL);
>        buf->len = block_size;
> 
>        cl_must_pass(block_writer_init(&writer, block_type, (uint8_t *) buf->buf,
>                                       block_size, header_off,
>                                       hash_size(REFTABLE_HASH_SHA1)));
>        for (size_t i = 0; i < nrecs; i++)
>                cl_must_pass(block_writer_add(&writer, &recs[i]));
> 
>        block_end = block_writer_finish(&writer);
>        cl_assert(block_end > 0);
> 
>        block_writer_release(&writer);
> 
>        return block_end;
> }
> 
> This function could be introduced by a preparatory commit in
> t/unit-tests/lib-reftable.{c,h}. It would be kind of similar to the
> existing cl_reftable_write_to_buf() helper in those files.
> 
> It looks like it could already simplify existing tests like:
> 
> - test_reftable_block__log_read_write
> - test_reftable_block__obj_read_write
> - test_reftable_block__ref_read_write
> - test_reftable_block__iterator
> 
> and it could simplify the new tests introduced by other patches in this series:
> 
> - 06/11 reftable/block: fix OOB read with bogus block size
> - 07/11 reftable/block: fix OOB read with bogus restart count
> - 09/11 reftable/block: fix OOB read with bogus restart offset

Good point, will do. Thanks!

Patrick