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

From: Matthew Suozzo <[email protected]>

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.

---
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(+)

index a96be506b5..66b7270fc0 100644
@@ -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

index 1fce441930..96eceddca7 100644
@@ -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;

index 9c95083ef4..0165b74bcb 100644
@@ -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

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?

happy to move it to a separate patch.

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

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.

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.

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

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

Putting more burden on the user is fine, and the AI output, when used correctly, can be not so bad.

Having said that, the real problem is that nobody seems to guide generative AI machinery well enough to make intelligible output. In order for a user to guide the machinery well enough, they must know what they want to discuss, and the output would be different from what a clueless and hands-off "here is a bug report from somewhere I saw, I do not understand the system well enough, solve it and describe the solution" prompt would result in.

Writing the first draft manually and having a machine check and correct typos and grammar is not a bad practice, as you remain in control of the overall structure and contents of the final result.

Going in the other direction? If you are not familiar with the area and are easily fooled by intelligent-sounding nonsense uttered by generative AI, correcting such nonsense into a usable shape would be difficult, and would make those on the receiving end who have to read it miserable.

By the way, I cannot help but wonder what happened to the AI policy discussion that the contributors' summit promised we would have soon. Who is organizing it?

[snip]

I think Emily wanted to kick it off, if I'm not mistaken.

Patrick

Hi Matthew,

I tried Patrick's change on a 32 bit build and it works for REFTABLE_ALLOC_ARRAY. However REFTABLE_CALLOC_ARRAY goes straight to reftable_calloc() with a size_t and gets truncated. I think no one passes a 64 bit count to it so nothing breaks but maybe worth a mention in the commit message?

Also the new test passes because of the count > in.len check. If that check goes away in v2 I think it might need a different test since on 64 bit nothing truncates. Maybe we can test reftable_alloc_size() directly?

Aniket

Changes since v1:

* Fix the truncation in reftable_alloc_size() and reftable_calloc() instead of checking the count in reftable_obj_record_decode() (Patrick, Aniket).
* Test reftable_alloc_size() directly instead of the decoder (Aniket).
* Move the Makefile change into its own patch (Patrick).
* Rewrite the commit messages (Patrick).

Matthew Suozzo (2):
Makefile: build the reftable record unit tests reftable/basics: widen allocation size arguments to uintmax_t

 Makefile                         |  1 +
 reftable/basics.c                |  9 +++++----
 reftable/basics.h                |  5 +++--
 t/unit-tests/u-reftable-basics.c | 18 ++++++++++++++++++
 4 files changed, 27 insertions(+), 6 deletions(-)

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

Range-diff vs v1:

1: 5fb73ec0a8 ! 1: 0d083bc81f reftable/record: bound an object record's offset count by its length

     @@ Metadata
      Author: Matthew Suozzo <[email protected]>
      
       ## Commit message ##
     -    reftable/record: bound an object record's offset count by its length
     +    Makefile: build the reftable record unit tests
      
     -    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.
     +    The Meson build includes the reftable record unit tests, but the
     +    Makefile does not. The suite was dropped from the Makefile when the
     +    test was converted to the clar framework in 2596bef5841f (2025-07-24).
      
     -    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.
     +    Include the reftable record unit test suite in the Makefile test
     +    target.
      
     -    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
     +    Assisted-by: Claude Fable 5.1
          Signed-off-by: Matthew Suozzo <[email protected]>
      
       ## Makefile ##
     @@ Makefile: CLAR_TEST_SUITES += u-reftable-block
       CLAR_TEST_SUITES += u-reftable-stack
       CLAR_TEST_SUITES += u-reftable-table
       CLAR_TEST_SUITES += u-reftable-tree
     -
     - ## reftable/record.c ##
     -@@ reftable/record.c: 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;
     -
     - ## t/unit-tests/u-reftable-record.c ##
     -@@ t/unit-tests/u-reftable-record.c: 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] = {

-: ---------- > 2: 8f4a8389d0 reftable/basics: widen allocation size arguments to uintmax_t

gitgitgadget

From: Matthew Suozzo <[email protected]>

The Meson build includes the reftable record unit tests, but the Makefile does not. The suite was dropped from the Makefile when the test was converted to the clar framework in 2596bef5841f (2025-07-24).

Include the reftable record unit test suite in the Makefile test target.

---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

index a96be506b5..66b7270fc0 100644
@@ -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

gitgitgadget

From: Matthew Suozzo <[email protected]>

reftable_alloc_size() takes its arguments as size_t, but reftable_obj_record_decode() passes it a uint64_t count. On 32-bit platforms, a count wider than 32 bits is truncated before the overflow check, so the decoder can get a small allocation for a large count and write past its end. reftable_calloc() takes size_t arguments the same way.

Make reftable_alloc_size() and reftable_calloc() take uintmax_t arguments, so the check sees the full count.

---
 reftable/basics.c                |  9 +++++----
 reftable/basics.h                |  5 +++--
 t/unit-tests/u-reftable-basics.c | 18 ++++++++++++++++++
 3 files changed, 26 insertions(+), 6 deletions(-)

index f0442a46cf..6103b72b44 100644
@@ -44,18 +44,19 @@ void reftable_free(void *p)
 		free(p);
 }
 
-void *reftable_calloc(size_t nelem, size_t elsize)
+void *reftable_calloc(uintmax_t nelem, uintmax_t elsize)
 {
+	size_t size;
 	void *p;
 
-	if (nelem && elsize > SIZE_MAX / nelem)
+	if (reftable_alloc_size(nelem, elsize, &size) < 0)
 		return NULL;
 
-	p = reftable_malloc(nelem * elsize);
+	p = reftable_malloc(size);
 	if (!p)
 		return NULL;
 
-	memset(p, 0, nelem * elsize);
+	memset(p, 0, size);
 	return p;
 }
 

index e4b83b2b03..29d2b799ca 100644
@@ -184,10 +184,11 @@ size_t names_length(const char **names);
 void *reftable_malloc(size_t sz);
 void *reftable_realloc(void *p, size_t sz);
 void reftable_free(void *p);
-void *reftable_calloc(size_t nelem, size_t elsize);
+void *reftable_calloc(uintmax_t nelem, uintmax_t elsize);
 char *reftable_strdup(const char *str);
 
-static inline int reftable_alloc_size(size_t nelem, size_t elsize, size_t *out)
+static inline int reftable_alloc_size(uintmax_t nelem, uintmax_t elsize,
+				      size_t *out)
 {
 	if (nelem && elsize > SIZE_MAX / nelem)
 		return -1;

index c5d83b6714..37bf1ade5a 100644
@@ -200,6 +200,24 @@ void test_reftable_basics__put_get_be16(void)
 	cl_assert_equal_i(in, out);
 }
 
+void test_reftable_basics__alloc_size(void)
+{
+	size_t out;
+
+	cl_assert_equal_i(reftable_alloc_size(3, 5, &out), 0);
+	cl_assert_equal_i(out, 15);
+	cl_assert(reftable_alloc_size(SIZE_MAX, 2, &out) < 0);
+
+	/* Counts that do not fit in a size_t. */
+	if (SIZE_MAX < UINTMAX_MAX) {
+		cl_assert(reftable_alloc_size((uintmax_t)SIZE_MAX + 1,
+					      1, &out) < 0);
+		cl_assert(reftable_alloc_size(1, (uintmax_t)SIZE_MAX + 1,
+					      &out) < 0);
+		cl_assert(reftable_calloc((uintmax_t)SIZE_MAX + 1, 1) == NULL);
+	}
+}
+
 void test_reftable_basics__alloc_grow(void)
 {
 	int *arr = NULL, *old_arr;

gitgitgadget