Re: [PATCH] reftable/record: bound an object record's offset count by its length

[m@suozzo.co]

October 19, 2026 at 3:46 AM, "Patrick Steinhardt" <ps@pks.im mailto:ps@pks.im?to=%22Patrick%20Steinhardt%22%20%3Cps%40pks.im%3E > wrote:


> 
> On Wed, Oct 14, 2026 at 02:58:03PM +0000, Matthew Suozzo via GitGitGadget wrote:
> 
> > 
> > From: Matthew Suozzo <m@suozzo.co>
> >  
> >  An object record's count of offsets is a 64-bit varint from the table,
> >  and reftable_obj_record_decode() allocates the offsets array from it
> >  through REFTABLE_ALLOC_ARRAY(), which takes a size_t. Where size_t is
> >  32 bits, a count of 2^32 + 1 allocates room for one offset, and the
> >  loop, which runs to the full count, writes an offset from the table
> >  for each varint left in the block past the end of the array. Where
> >  size_t is 64 bits, a large count only makes a large allocation that
> >  the decode then fails to fill.
> >  
> >  Nothing in Git reaches this decoder from a repository: a block of a
> >  type other than the one wanted is refused before its records are
> >  decoded, and the object index, the only reader of object blocks, is
> >  used only by the unit tests. The decoder is still wrong.
> >  
> >  Each offset takes at least a byte, so refuse a count larger than what
> >  is left of the record.
> > 
> Sorry, but this whole commit message reads very AI-generated to me, as
> it is both overly verbose and hard to parse. I would welcome it if you
> could maybe touch up the message for normal human beings.

yeah it was AI generated and i copy-edited it. it didn't feel too excessive since i wanted to be precise about the issue's mechanism, scope (32-bit only), severity (not really reachable), and describe the fix.

any pointers on how you'd reword? remove the exposition?

> 
> > 
> > diff --git a/Makefile b/Makefile
> >  index a96be506b5..66b7270fc0 100644
> >  --- a/Makefile
> >  +++ b/Makefile
> >  @@ -1551,6 +1551,7 @@ CLAR_TEST_SUITES += u-reftable-block
> >  CLAR_TEST_SUITES += u-reftable-merged
> >  CLAR_TEST_SUITES += u-reftable-pq
> >  CLAR_TEST_SUITES += u-reftable-readwrite
> >  +CLAR_TEST_SUITES += u-reftable-record
> >  CLAR_TEST_SUITES += u-reftable-stack
> >  CLAR_TEST_SUITES += u-reftable-table
> >  CLAR_TEST_SUITES += u-reftable-tree
> > 
> Makes sense, even though this should probably have been in its own
> patch.

happy to move it to a separate patch.

> 
> > 
> > diff --git a/reftable/record.c b/reftable/record.c
> >  index 1fce441930..96eceddca7 100644
> >  --- a/reftable/record.c
> >  +++ b/reftable/record.c
> >  @@ -589,6 +589,10 @@ static int reftable_obj_record_decode(void *rec, struct reftable_buf key,
> >  if (count == 0)
> >  return start.len - in.len;
> >  
> >  + /* each offset takes at least a byte */
> >  + if (count > in.len)
> >  + return REFTABLE_FORMAT_ERROR;
> > 
> This check doesn't really seem to match what you're describing. What you
> describe is an integer overflow, but you only check for a very specific
> edge ecase here. I would have expected for example that...
> 
> > 
> > REFTABLE_ALLOC_ARRAY(r->offsets, count);
> > 
> ... this line already detects the overflow and then refuses the
> operation. Because `REFTABLE_ALLOC_ARRAY()` itself already tries to
> check for overflows.
> 
> That being said, if you take a look at `reftable_alloc_size()` you can
> see that the input is both of type `size_t`. And _that_ is indeed broken
> on platforms where `size_t` is 32 bits, as we'd potentially wrap. But
> that fact is not even mentioned as part of the commit message at all.

isn't that "REFTABLE_ALLOC_ARRAY(), which takes a size_t" in the original message?

> 
> So overall I'm not convinced that this fix makes sense, we should rather
> fix the source of the problem, which is `reftable_alloc_size()`.

that felt like a pretty disruptive change i was hesitant to put up as my first patch but i'd be happy to make the change.

> 
> Thanks!
> 
> Patrick
>