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

[Patrick Steinhardt <ps@pks.im>]

On Mon, Oct 19, 2026 at 05:14:08PM +0000, m@suozzo.co wrote:
> 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?

It's hard to say. With AI-generated commit messages you very often have
the feeling that you end up understanding everything and nothing. It
just feels off in a way that is hard to describe, and consequently it's
also hard to say what exactly needs changing.

My typical suggestion is to not use AI to write your commit message at
all and instead write it yourself. Commit messages have always been the
place where you can demonstrate that you understand what you are doing
as part of the patch by properly explaining it in a way that another
human being understands it. And I think that role is becoming even more
important now: in a world where we cannot trust that the contributor has
the slightest understanding of what they are contributing, they need to
go the extra mile to prove it to us by having a well-written explanation
thereof.

I know that this is putting more of a burden on contributors, and I'm
not a fan of that. But I think it's becoming abundantly clear that AI
does not really give open communities much of a choice, at least not if
they want to keep standards high.

> > > 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?

Ugh, no idea how I missed that. Probably because my brain was sucked out
while reading it.

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

Hm, I wouldn't call it disruptive but rather holistic. By fixing that
one function we also implicitly fix all the other callers that may
currently be wrong. And in the end nothing really changes for callers
anyway: I'd change the parameters to be of type `uintmax_t`, but the
return value should probably remain a `size_t`. So we'd continue to work
just fine in all cases where there is no overflow of `size_t`, but we'd
properly detect cases where the parameters themselves already overflow
it.

Patrick