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

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

[Matthew Suozzo via GitGitGadget]

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.

The Makefile did not build the reftable record unit tests (the Meson
build does). Add the suite there so that the new test runs in both.

Assisted-by: Claude Opus 5.5, Claude Fable 5.1
Signed-off-by: Matthew Suozzo <m@suozzo.co>
---
    reftable/record: bound an object record's offset count by its length

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2255%2Fmsuozzo%2Freftable-obj-record-count-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2255/msuozzo/reftable-obj-record-count-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2255

 Makefile                         |  1 +
 reftable/record.c                |  4 ++++
 t/unit-tests/u-reftable-record.c | 35 ++++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+)

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
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;
+
 	REFTABLE_ALLOC_ARRAY(r->offsets, count);
 	if (!r->offsets)
 		return REFTABLE_OUT_OF_MEMORY_ERROR;
diff --git a/t/unit-tests/u-reftable-record.c b/t/unit-tests/u-reftable-record.c
index 9c95083ef4..0165b74bcb 100644
--- a/t/unit-tests/u-reftable-record.c
+++ b/t/unit-tests/u-reftable-record.c
@@ -524,6 +524,41 @@ void test_reftable_record__obj_record_roundtrip(void)
 	reftable_buf_release(&scratch);
 }
 
+void test_reftable_record__obj_record_decode_count_larger_than_record(void)
+{
+	struct reftable_buf key = REFTABLE_BUF_INIT, scratch = REFTABLE_BUF_INIT;
+	struct reftable_record rec = { .type = REFTABLE_BLOCK_TYPE_OBJ };
+	uint8_t buffer[32];
+	struct string_view dest = { .buf = buffer, .len = sizeof(buffer) }, in;
+	int n;
+
+	/*
+	 * A count of offsets that a 32-bit size_t truncates to one,
+	 * followed by two offsets.
+	 */
+	n = put_var_int(&dest, ((uint64_t)1 << 32) + 1);
+	cl_assert_gt_i(n, 0);
+	string_view_consume(&dest, n);
+	n = put_var_int(&dest, 1);
+	cl_assert_gt_i(n, 0);
+	string_view_consume(&dest, n);
+	n = put_var_int(&dest, 1);
+	cl_assert_gt_i(n, 0);
+	string_view_consume(&dest, n);
+
+	in.buf = buffer;
+	in.len = sizeof(buffer) - dest.len;
+
+	cl_assert_equal_i(reftable_buf_addstr(&key, "\x01\x02"), 0);
+	cl_assert_equal_i(reftable_record_decode(&rec, key, 0, in,
+						 REFTABLE_HASH_SIZE_SHA1, &scratch),
+			  REFTABLE_FORMAT_ERROR);
+
+	reftable_record_release(&rec);
+	reftable_buf_release(&key);
+	reftable_buf_release(&scratch);
+}
+
 void test_reftable_record__index_record_comparison(void)
 {
 	struct reftable_record in[3] = {

base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
-- 
gitgitgadget

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

[Patrick Steinhardt]

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.

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

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

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()`.

Thanks!

Patrick

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

[m]

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
>