* [PATCH 06/11] reftable/block: fix OOB read with bogus block size
From: Patrick Steinhardt @ 2026-06-24 8:23 UTC (permalink / raw)
To: git; +Cc: oxsignal
In-Reply-To: <20260624-pks-reftable-hardening-v1-0-66e4ce87c6b9@pks.im>
The block size is read from the block header, which is untrusted data.
We use it without verification to access the restart count at the end of
the block as well as to compute the restart table offset. With a bogus
block size that exceeds the data we have actually read this can lead to
an out-of-bounds read:
==1458284==ERROR: AddressSanitizer: SEGV on unknown address 0x7d8ff7de4b7d (pc 0x55555598c339 bp 0x7fffffff4ef0 sp 0x7fffffff4eb0 T0)
==1458284==The signal is caused by a READ memory access.
#0 0x55555598c339 in reftable_get_be16 ./build/../reftable/basics.h:118:9
#1 0x55555598bee2 in reftable_block_init ./build/../reftable/block.c:344:18
#2 0x555555813e0e in test_reftable_block__corrupt_block_size ./build/../t/unit-tests/u-reftable-block.c:540:8
#3 0x5555557f684e in clar_run_test ./build/../t/unit-tests/clar/clar.c:335:3
#4 0x5555557f2e69 in clar_run_suite ./build/../t/unit-tests/clar/clar.c:431:3
#5 0x5555557f2882 in clar_test_run ./build/../t/unit-tests/clar/clar.c:636:4
#6 0x5555557f375f in clar_test ./build/../t/unit-tests/clar/clar.c:687:11
#7 0x5555557fa49d in cmd_main ./build/../t/unit-tests/unit-test.c:62:8
#8 0x55555584b55a in main ./build/../common-main.c:9:11
#9 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#10 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#11 0x555555694c24 in _start (./build/t/unit-tests+0x140c24)
==1458284==Register values:
rax = 0x00007d8ff7de4b7d rbx = 0x00007fffffff4f00 rcx = 0x0000000000000006 rdx = 0x0000000000000010
rdi = 0x00007d8ff7de4b7d rsi = 0x00007bfff5cf0420 rbp = 0x00007fffffff4ef0 rsp = 0x00007fffffff4eb0
r8 = 0x00000f807eb960b8 r9 = 0x0000000000000001 r10 = 0x00007bfff5cf05e7 r11 = 0x000000000000000f
r12 = 0x00007fffffff58f8 r13 = 0x0000000000000001 r14 = 0x0000555555ee8160 r15 = 0x0000000000000000
AddressSanitizer can not provide additional info.
Verify that the claimed block size fits into the block data before using
it.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 9 +++++++++
t/unit-tests/u-reftable-block.c | 45 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 54 insertions(+)
diff --git a/reftable/block.c b/reftable/block.c
index b86cb9ec5a..4d6b11c2e7 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -340,6 +340,15 @@ int reftable_block_init(struct reftable_block *block,
full_block_size = block_size;
}
+ /*
+ * Ensure that we have sufficient data available now to satisfy the
+ * claimed block size.
+ */
+ if (block_size > block->block_data.len) {
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
+ }
+
restart_count = reftable_get_be16(block->block_data.data + block_size - 2);
restart_off = block_size - 2 - 3 * restart_count;
diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
index 40274af5c0..1f35aed91a 100644
--- a/t/unit-tests/u-reftable-block.c
+++ b/t/unit-tests/u-reftable-block.c
@@ -500,3 +500,48 @@ void test_reftable_block__corrupt_log_block_size(void)
block_writer_release(&writer);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_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_REF,
+ .u.ref = {
+ .value_type = REFTABLE_REF_VAL1,
+ .refname = (char *) "refs/heads/main",
+ },
+ };
+ 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_REF,
+ (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);
+
+ /*
+ * The block size is stored as a big-endian 24-bit integer right after
+ * the one-byte block type at the start of the block. Corrupt it to
+ * claim a size that is larger than the data we actually have. Reading
+ * the restart count and restart table relative to such a bogus block
+ * size must not access out-of-bounds memory.
+ */
+ reftable_put_be24((uint8_t *) data.buf + 1, 0xffffff);
+
+ block_source_from_buf(&source, &data);
+ cl_assert_equal_i(reftable_block_init(&block, &source, 0, 0, data.len,
+ REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_REF),
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_block_release(&block);
+ block_writer_release(&writer);
+ reftable_buf_release(&data);
+}
--
2.55.0.rc1.745.g43192e7977.dirty
^ permalink raw reply related
* [PATCH 07/11] reftable/block: fix OOB read with bogus restart count
From: Patrick Steinhardt @ 2026-06-24 8:23 UTC (permalink / raw)
To: git; +Cc: oxsignal
In-Reply-To: <20260624-pks-reftable-hardening-v1-0-66e4ce87c6b9@pks.im>
The restart count is stored in the last two bytes of a block. We use it
without verification to compute the offset of the restart table. With a
bogus restart count that is large enough this computation underflows,
and the subsequent reads via the restart table access out-of-bounds
memory:
==129439==ERROR: AddressSanitizer: SEGV on unknown address 0x7d90f6dcd0ad (pc 0x55555598ce89 bp 0x7fffffff4ed0 sp 0x7fffffff4e80 T0)
==129439==The signal is caused by a READ memory access.
#0 0x55555598ce89 in reftable_get_be24 ./git/build/../reftable/basics.h:125:9
#1 0x55555598eabf in block_restart_offset ./git/build/../reftable/block.c:407:9
#2 0x55555598e5d5 in restart_needle_less ./git/build/../reftable/block.c:431:17
#3 0x5555559887e2 in binsearch ./git/build/../reftable/basics.c:165:13
#4 0x55555598dfec in block_iter_seek_key ./git/build/../reftable/block.c:529:6
#5 0x555555814517 in test_reftable_block__corrupt_restart_count ./git/build/../t/unit-tests/u-reftable-block.c:593:15
#6 0x5555557f684e in clar_run_test ./git/build/../t/unit-tests/clar/clar.c:335:3
#7 0x5555557f2e69 in clar_run_suite ./git/build/../t/unit-tests/clar/clar.c:431:3
#8 0x5555557f2882 in clar_test_run ./git/build/../t/unit-tests/clar/clar.c:636:4
#9 0x5555557f375f in clar_test ./git/build/../t/unit-tests/clar/clar.c:687:11
#10 0x5555557fa49d in cmd_main ./git/build/../t/unit-tests/unit-test.c:62:8
#11 0x55555584c12a in main ./git/build/../common-main.c:9:11
#12 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#13 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#14 0x555555694c24 in _start (./git/build/t/unit-tests+0x140c24)
==129439==Register values:
rax = 0x00007d90f6dcd0ad rbx = 0x00007fffffff4f20 rcx = 0xf2f2f2f8f2f2f2f8 rdx = 0x0000000000000000
rdi = 0x00007d90f6dcd0ad rsi = 0x0000000000007fff rbp = 0x00007fffffff4ed0 rsp = 0x00007fffffff4e80
r8 = 0x0000000000000000 r9 = 0x0000000000000000 r10 = 0x0000000000000000 r11 = 0x0000000000000017
r12 = 0x00007fffffff58e8 r13 = 0x0000000000000001 r14 = 0x00007ffff7ffd000 r15 = 0x00005555560550b0
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ./git/build/../reftable/basics.h:125:9 in reftable_get_be24
Verify that the restart table actually fits into the block.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 4 ++++
t/unit-tests/u-reftable-block.c | 46 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 50 insertions(+)
diff --git a/reftable/block.c b/reftable/block.c
index 4d6b11c2e7..4d285aefd7 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -351,6 +351,10 @@ int reftable_block_init(struct reftable_block *block,
restart_count = reftable_get_be16(block->block_data.data + block_size - 2);
restart_off = block_size - 2 - 3 * restart_count;
+ if (restart_off < header_size + 4 || restart_off > block_size - 2) {
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
+ }
block->block_type = block_type;
block->hash_size = hash_size;
diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
index 1f35aed91a..ba410a0885 100644
--- a/t/unit-tests/u-reftable-block.c
+++ b/t/unit-tests/u-reftable-block.c
@@ -545,3 +545,49 @@ void test_reftable_block__corrupt_block_size(void)
block_writer_release(&writer);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_restart_count(void)
+{
+ struct reftable_block_source source = { 0 };
+ struct block_writer writer = {
+ .last_key = REFTABLE_BUF_INIT,
+ };
+ struct reftable_record rec = {
+ .type = REFTABLE_BLOCK_TYPE_REF,
+ .u.ref = {
+ .value_type = REFTABLE_REF_VAL1,
+ .refname = (char *) "refs/heads/main",
+ },
+ };
+ struct reftable_block block = { 0 };
+ struct reftable_buf data;
+ int block_size;
+
+ 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_REF,
+ (uint8_t *) data.buf, data.len,
+ 0, hash_size(REFTABLE_HASH_SHA1)));
+ cl_must_pass(block_writer_add(&writer, &rec));
+ block_size = block_writer_finish(&writer);
+ cl_assert(block_size > 0);
+
+ /*
+ * Corrupt the restart count to claim a bogus number of restart points.
+ * Note that this would only cause us to perform an out-of-bounds
+ * access when seeking into the block, but we want to refuse such a
+ * block outright.
+ */
+ reftable_put_be16((uint8_t *) data.buf + block_size - 2, 0xffff);
+
+ block_source_from_buf(&source, &data);
+ cl_assert_equal_i(reftable_block_init(&block, &source, 0, 0, data.len,
+ REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_REF),
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_block_release(&block);
+ block_writer_release(&writer);
+ reftable_buf_release(&data);
+}
--
2.55.0.rc1.745.g43192e7977.dirty
^ permalink raw reply related
* [PATCH 08/11] reftable/block: fix use of uninitialized memory when binsearch fails
From: Patrick Steinhardt @ 2026-06-24 8:23 UTC (permalink / raw)
To: git; +Cc: oxsignal
In-Reply-To: <20260624-pks-reftable-hardening-v1-0-66e4ce87c6b9@pks.im>
When doing the binary search through our restart offsets we may hit an
error in case `restart_needle_less()` fails to decode the record at the
given offset. While we correctly detect this case and error out, it will
cause us to call `reftable_record_release()` on the yet-uninitialized
record.
Fix this by initializing the record earlier.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/reftable/block.c b/reftable/block.c
index 4d285aefd7..89efce8751 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -517,6 +517,10 @@ int block_iter_seek_key(struct block_iter *it, struct reftable_buf *want)
int err = 0;
size_t i;
+ err = reftable_record_init(&rec, reftable_block_type(it->block));
+ if (err < 0)
+ goto done;
+
/*
* Perform a binary search over the block's restart points, which
* avoids doing a linear scan over the whole block. Like this, we
@@ -558,10 +562,6 @@ int block_iter_seek_key(struct block_iter *it, struct reftable_buf *want)
else
it->next_off = it->block->header_off + 4;
- err = reftable_record_init(&rec, reftable_block_type(it->block));
- if (err < 0)
- goto done;
-
/*
* We're looking for the last entry less than the wanted key so that
* the next call to `block_reader_next()` would yield the wanted
--
2.55.0.rc1.745.g43192e7977.dirty
^ permalink raw reply related
* [PATCH 09/11] reftable/block: fix OOB read with bogus restart offset
From: Patrick Steinhardt @ 2026-06-24 8:23 UTC (permalink / raw)
To: git; +Cc: oxsignal
In-Reply-To: <20260624-pks-reftable-hardening-v1-0-66e4ce87c6b9@pks.im>
Restart points encode records in a given block that do not use prefix
compression and that can thus immediately be seeked to. These offsets
are encoded in the restart table, where each offset needs to point at
one of the records of the block. We do not verify this though, so a
bogus restart offset may cause an out-of-bounds read:
==1472280==ERROR: AddressSanitizer: SEGV on unknown address 0x7d8ff7de5f7f (pc 0x55555599502b bp 0x7fffffff4df0 sp 0x7fffffff4d40 T0)
==1472280==The signal is caused by a READ memory access.
#0 0x55555599502b in get_var_int ./git/build/../reftable/record.c:30:6
#1 0x555555995c2a in reftable_decode_keylen ./git/build/../reftable/record.c:177:6
#2 0x55555598e85c in restart_needle_less ./git/build/../reftable/block.c:455:6
#3 0x55555598895f in binsearch ./git/build/../reftable/basics.c:175:9
#4 0x55555598e189 in block_iter_seek_key ./git/build/../reftable/block.c:543:6
#5 0x555555814aee in test_reftable_block__corrupt_restart_offset ./git/build/../t/unit-tests/u-reftable-block.c:636:20
#6 0x5555557f684e in clar_run_test ./git/build/../t/unit-tests/clar/clar.c:335:3
#7 0x5555557f2e69 in clar_run_suite ./git/build/../t/unit-tests/clar/clar.c:431:3
#8 0x5555557f2882 in clar_test_run ./git/build/../t/unit-tests/clar/clar.c:636:4
#9 0x5555557f375f in clar_test ./git/build/../t/unit-tests/clar/clar.c:687:11
#10 0x5555557fa49d in cmd_main ./git/build/../t/unit-tests/unit-test.c:62:8
#11 0x55555584c25a in main ./git/build/../common-main.c:9:11
#12 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#13 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#14 0x555555694c24 in _start (./git/build/t/unit-tests+0x140c24)
==1472280==Register values:
rax = 0x00007d8ff7de5f7f rbx = 0x00007fffffff4e00 rcx = 0x00007d8ff7de5f80 rdx = 0x00007bfff5b6af60
rdi = 0x00007bfff5b6af40 rsi = 0x00007bfff592dfa0 rbp = 0x00007fffffff4df0 rsp = 0x00007fffffff4d40
r8 = 0x00000000ff00002b r9 = 0x00007d8ff7de5f7f r10 = 0x00000f7ffeb25bf0 r11 = 0xf3f30000f1f1f1f1
r12 = 0x00007fffffff58f8 r13 = 0x0000000000000001 r14 = 0x00007ffff7ffd000 r15 = 0x0000555556055fd0
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ./git/build/../reftable/record.c:30:6 in get_var_int
Guard against such restart offsets and signal an error to the caller via
`args.error`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 9 ++++++++
t/unit-tests/u-reftable-block.c | 51 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 60 insertions(+)
diff --git a/reftable/block.c b/reftable/block.c
index 89efce8751..1fa81405d2 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -440,6 +440,15 @@ static int restart_needle_less(size_t idx, void *_args)
uint8_t extra;
int n;
+ /*
+ * The restart offset must point to a record, which is stored before
+ * the restart table. Verify that this is the case.
+ */
+ if (off >= args->block->restart_off) {
+ args->error = 1;
+ return -1;
+ }
+
/*
* Records at restart points are stored without prefix compression, so
* there is no need to fully decode the record key here. This removes
diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
index ba410a0885..77a054d082 100644
--- a/t/unit-tests/u-reftable-block.c
+++ b/t/unit-tests/u-reftable-block.c
@@ -591,3 +591,54 @@ void test_reftable_block__corrupt_restart_count(void)
block_writer_release(&writer);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_restart_offset(void)
+{
+ struct reftable_block_source source = { 0 };
+ struct block_writer writer = {
+ .last_key = REFTABLE_BUF_INIT,
+ };
+ struct reftable_record rec = {
+ .type = REFTABLE_BLOCK_TYPE_REF,
+ .u.ref = {
+ .value_type = REFTABLE_REF_VAL1,
+ .refname = (char *) "refs/heads/main",
+ },
+ };
+ struct reftable_block block = { 0 };
+ struct block_iter it = BLOCK_ITER_INIT;
+ struct reftable_buf want = REFTABLE_BUF_INIT;
+ 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_REF,
+ (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);
+
+ block_source_from_buf(&source, &data);
+ cl_must_pass(reftable_block_init(&block, &source, 0, 0, data.len,
+ REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_REF));
+
+ /*
+ * Corrupt the first restart offset, stored as a big-endian 24-bit
+ * integer at the start of the restart table, to point past the end of
+ * the records section. Seeking such a block must fail gracefully.
+ */
+ reftable_put_be24((uint8_t *) block.block_data.data + block.restart_off,
+ 0xffffff);
+
+ block_iter_init(&it, &block);
+ cl_must_pass(reftable_buf_addstr(&want, "refs/heads/main"));
+ cl_assert_equal_i(block_iter_seek_key(&it, &want), REFTABLE_FORMAT_ERROR);
+
+ reftable_buf_release(&want);
+ block_iter_close(&it);
+ reftable_block_release(&block);
+ block_writer_release(&writer);
+ reftable_buf_release(&data);
+}
--
2.55.0.rc1.745.g43192e7977.dirty
^ permalink raw reply related
* [PATCH 10/11] reftable/table: fix NULL pointer access when seeking to bogus offsets
From: Patrick Steinhardt @ 2026-06-24 8:23 UTC (permalink / raw)
To: git; +Cc: oxsignal
In-Reply-To: <20260624-pks-reftable-hardening-v1-0-66e4ce87c6b9@pks.im>
When seeking an iterator to an arbitrary offset we may return a positive
value in case the offset points beyond the block. This makes sense when
iterating through multiple blocks of the same section, as the positive
value indicates to us that we're at the end of the table.
But when the offset originates from a section or index offset it is
supposed to point at a valid block, so an out-of-bounds value means that
the table is corrupt. Treating it as a normal end-of-iteration causes us
to silently report an empty section instead of surfacing the corruption,
and we are left with a partially-initialized block. This may later on
cause a NULL pointer exception:
==1486841==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x55555598e02c bp 0x7fffffff4eb0 sp 0x7fffffff4e70 T0)
==1486841==The signal is caused by a READ memory access.
==1486841==Hint: address points to the zero page.
#0 0x55555598e02c in reftable_block_type ./git/build/../reftable/block.c:392:9
#1 0x55555598ee6e in block_iter_seek_key ./git/build/../reftable/block.c:536:35
#2 0x5555559ae553 in table_iter_seek_linear ./git/build/../reftable/table.c:344:8
#3 0x5555559adbff in table_iter_seek ./git/build/../reftable/table.c:450:9
#4 0x5555559ada9c in table_iter_seek_void ./git/build/../reftable/table.c:460:9
#5 0x555555992872 in reftable_iterator_seek_log_at ./git/build/../reftable/iter.c:281:9
#6 0x555555992953 in reftable_iterator_seek_log ./git/build/../reftable/iter.c:287:9
#7 0x55555583aa78 in test_reftable_table__seek_invalid_log_offset ./git/build/../t/unit-tests/u-reftable-table.c:257:20
#8 0x5555557f684e in clar_run_test ./git/build/../t/unit-tests/clar/clar.c:335:3
#9 0x5555557f2e69 in clar_run_suite ./git/build/../t/unit-tests/clar/clar.c:431:3
#10 0x5555557f2882 in clar_test_run ./git/build/../t/unit-tests/clar/clar.c:636:4
#11 0x5555557f375f in clar_test ./git/build/../t/unit-tests/clar/clar.c:687:11
#12 0x5555557fa49d in cmd_main ./git/build/../t/unit-tests/unit-test.c:62:8
#13 0x55555584cffa in main ./git/build/../common-main.c:9:11
#14 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#15 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#16 0x555555694c24 in _start (./git/build/t/unit-tests+0x140c24)
==1486841==Register values:
rax = 0x0000000000000000 rbx = 0x00007fffffff4ec0 rcx = 0x0000000000000000 rdx = 0x00007cfff6e2bd58
rdi = 0x00007cfff6e2bd58 rsi = 0x00007bfff5da1020 rbp = 0x00007fffffff4eb0 rsp = 0x00007fffffff4e70
r8 = 0x0000000000000000 r9 = 0x0000000000000002 r10 = 0x0000000000000000 r11 = 0x0000000000000017
r12 = 0x00007fffffff5908 r13 = 0x0000000000000001 r14 = 0x00007ffff7ffd000 r15 = 0x0000555556056e90
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ./git/build/../reftable/block.c:392:9 in reftable_block_type
==1486841==ABORTING
Fix this by returning a proper error in `table_iter_seek_to()` when the
offset ranges beyond the block.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/table.c | 2 ++
t/unit-tests/u-reftable-table.c | 63 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 65 insertions(+)
diff --git a/reftable/table.c b/reftable/table.c
index 56362df0ed..f4bc86a29d 100644
--- a/reftable/table.c
+++ b/reftable/table.c
@@ -242,6 +242,8 @@ static int table_iter_seek_to(struct table_iter *ti, uint64_t off, uint8_t typ)
int err;
err = table_init_block(ti->table, &ti->block, off, typ);
+ if (err > 0)
+ return REFTABLE_FORMAT_ERROR;
if (err != 0)
return err;
diff --git a/t/unit-tests/u-reftable-table.c b/t/unit-tests/u-reftable-table.c
index 14fae8b199..c7dca45e70 100644
--- a/t/unit-tests/u-reftable-table.c
+++ b/t/unit-tests/u-reftable-table.c
@@ -1,8 +1,11 @@
#include "unit-test.h"
#include "lib-reftable.h"
+#include "reftable/basics.h"
+#include "reftable/block.h"
#include "reftable/blocksource.h"
#include "reftable/constants.h"
#include "reftable/iter.h"
+#include "reftable/reftable-error.h"
#include "reftable/table.h"
#include "strbuf.h"
@@ -199,3 +202,63 @@ void test_reftable_table__block_iterator(void)
reftable_buf_release(&buf);
reftable_free(records);
}
+
+void test_reftable_table__seek_invalid_log_offset(void)
+{
+ struct reftable_ref_record refs[] = {
+ {
+ .refname = (char *) "refs/heads/main",
+ .value_type = REFTABLE_REF_VAL1,
+ .value.val1 = { 42 },
+ },
+ };
+ struct reftable_log_record logs[] = {
+ {
+ .refname = (char *) "refs/heads/main",
+ .update_index = 1,
+ .value_type = REFTABLE_LOG_UPDATE,
+ .value.update = {
+ .name = (char *) "user",
+ .email = (char *) "user@example.com",
+ .message = (char *) "message\n",
+ },
+ },
+ };
+ struct reftable_block_source source = { 0 };
+ struct reftable_log_record log = { 0 };
+ struct reftable_iterator it = { 0 };
+ struct reftable_table *table;
+ struct reftable_buf buf = REFTABLE_BUF_INIT;
+ size_t fsize = footer_size(1);
+ uint8_t *footer;
+
+ cl_reftable_write_to_buf(&buf, refs, ARRAY_SIZE(refs),
+ logs, ARRAY_SIZE(logs), NULL);
+
+ /*
+ * Corrupt the log section offset stored in the footer so that it points
+ * past the end of the table. The footer is checksummed, so we also have
+ * to recompute and rewrite the CRC.
+ */
+ footer = (uint8_t *) buf.buf + buf.len - fsize;
+ reftable_put_be64(footer + header_size(1) + 24, UINT64_MAX);
+ reftable_put_be32(footer + fsize - 4, crc32(0, footer, fsize - 4));
+
+ block_source_from_buf(&source, &buf);
+ cl_must_pass(reftable_table_new(&table, &source, "name"));
+
+ /*
+ * Seeking the log iterator must not crash even though the log section
+ * offset is bogus. As the offset points past the end of the table we
+ * know that the table is corrupt, so the seek must report a format
+ * error instead of pretending that the section is empty.
+ */
+ reftable_table_init_log_iterator(table, &it);
+ cl_assert_equal_i(reftable_iterator_seek_log(&it, ""),
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_log_record_release(&log);
+ reftable_iterator_destroy(&it);
+ reftable_table_decref(table);
+ reftable_buf_release(&buf);
+}
--
2.55.0.rc1.745.g43192e7977.dirty
^ permalink raw reply related
* [PATCH 11/11] reftable/table: fix OOB read on truncated table
From: Patrick Steinhardt @ 2026-06-24 8:23 UTC (permalink / raw)
To: git; +Cc: oxsignal
In-Reply-To: <20260624-pks-reftable-hardening-v1-0-66e4ce87c6b9@pks.im>
When opening a table we compute the size of its data section by
subtracting the footer size from the file size. We do not verify that
the file is actually large enough to contain both the header and the
footer though. With a truncated table the subtraction can thus
underflow, causing us to read the footer out of bounds:
SUMMARY: AddressSanitizer: heap-buffer-overflow (/home/pks/Development/git/build/t/unit-tests+0x2479a4) in __asan_memcpy
Shadow bytes around the buggy address:
0x7ccff6e0de80: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
0x7ccff6e0df00: fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa
0x7ccff6e0df80: fa fa fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x7ccff6e0e000: fd fd fd fd fa fa fa fa fa fa fa fa fd fd fd fd
0x7ccff6e0e080: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa
=>0x7ccff6e0e100: fa fa fa fa fa[fa]00 00 00 00 00 00 00 00 00 00
0x7ccff6e0e180: 00 00 00 00 00 00 00 04 fa fa fa fa fa fa fa fa
0x7ccff6e0e200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x7ccff6e0e280: 00 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7ccff6e0e300: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7ccff6e0e380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==1500371==ABORTING
Verify that the file is large enough to contain both the header and the
footer before computing the table size.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/table.c | 5 +++++
t/unit-tests/u-reftable-table.c | 28 ++++++++++++++++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/reftable/table.c b/reftable/table.c
index f4bc86a29d..b4d3f9e211 100644
--- a/reftable/table.c
+++ b/reftable/table.c
@@ -562,6 +562,11 @@ int reftable_table_new(struct reftable_table **out,
goto done;
}
+ if (file_size < header_size(t->version) + footer_size(t->version)) {
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
+ }
+
t->size = file_size - footer_size(t->version);
t->source = *source;
t->name = reftable_strdup(name);
diff --git a/t/unit-tests/u-reftable-table.c b/t/unit-tests/u-reftable-table.c
index c7dca45e70..28b0ef5258 100644
--- a/t/unit-tests/u-reftable-table.c
+++ b/t/unit-tests/u-reftable-table.c
@@ -262,3 +262,31 @@ void test_reftable_table__seek_invalid_log_offset(void)
reftable_table_decref(table);
reftable_buf_release(&buf);
}
+
+void test_reftable_table__new_with_truncated_table(void)
+{
+ struct reftable_ref_record refs[] = {
+ {
+ .refname = (char *) "refs/heads/main",
+ .value_type = REFTABLE_REF_VAL1,
+ .value.val1 = { 42 },
+ },
+ };
+ struct reftable_block_source source = { 0 };
+ struct reftable_table *table;
+ struct reftable_buf buf = REFTABLE_BUF_INIT;
+
+ cl_reftable_write_to_buf(&buf, refs, ARRAY_SIZE(refs), NULL, 0, NULL);
+
+ /*
+ * Truncate the table so that it is large enough to read the header, but
+ * too small to also contain the footer.
+ */
+ buf.len = footer_size(1) - 1;
+ block_source_from_buf(&source, &buf);
+
+ cl_assert_equal_i(reftable_table_new(&table, &source, "name"),
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_buf_release(&buf);
+}
--
2.55.0.rc1.745.g43192e7977.dirty
^ permalink raw reply related
* Re: [PATCH 3/3] connected: search promisor objects generically
From: Patrick Steinhardt @ 2026-06-24 9:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq4iiu1mrt.fsf@gitster.g>
On Mon, Jun 22, 2026 at 10:57:10AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> > - *
> > - * Before checking for promisor packs, be sure we have the
> > - * latest pack-files loaded into memory.
> > */
> > - odb_reprepare(the_repository->objects);
>
> Hmph?
Oh, I think I completely misread this as `odb_alternates_prepare()`,
which is something you typically see before loops like this. By using a
helper like `odb_for_each_object_ext()` we of course wouldn't have to
call that function anymore.
But this here is of course different, as this call would also cause us
to reload packfiles and loose objects.
> > do {
> > - struct packed_git *p;
> > -
> > - repo_for_each_pack(the_repository, p) {
> > - if (!p->pack_promisor)
> > - continue;
> > - if (find_pack_entry_one(oid, p))
> > - goto promisor_pack_found;
> > + opts.prefix = oid;
> > +
> > + err = odb_for_each_object_ext(the_repository->objects,
> > + NULL, promised_object_cb,
> > + NULL, &opts);
> > + if (err < 0)
> > + break;
> > + if (err > 0) {
> > + err = 0;
> > + continue;
> > }
>
> So we used to manually iterate and stop when we have a matching pack
> entry, but now "stop when we find" is done by promisor_object_cb
> callback that returns 1.
>
> What is the reason why we no longer odb_(re)prepare() upfront before
> going into the loop? Would it make us miss a newly added promisor
> packs? We will fall back to rev-list for correctness, so it may not
> matter, though.
So yes, this is a bug.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH 3/3] connected: search promisor objects generically
From: Patrick Steinhardt @ 2026-06-24 9:33 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <CAP8UFD1tqBBRiJV18xBMcDDT4Q7xCkqOLrtJGAO7o4oA=-Vr=w@mail.gmail.com>
On Tue, Jun 23, 2026 at 09:45:44AM +0200, Christian Couder wrote:
> On Mon, Jun 22, 2026 at 10:50 AM Patrick Steinhardt <ps@pks.im> wrote:
> > diff --git a/connected.c b/connected.c
> > index 7e26976832..9a666f0cdf 100644
> > --- a/connected.c
> > +++ b/connected.c
> > @@ -54,31 +66,30 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
> > * object is a promisor object. Instead, just make sure we
> > * received, in a promisor packfile, the objects pointed to by
> > * each wanted ref.
> > - *
> > - * Before checking for promisor packs, be sure we have the
> > - * latest pack-files loaded into memory.
> > */
> > - odb_reprepare(the_repository->objects);
>
> Like Junio, I am not sure it's correct to remove the
> `odb_reprepare(the_repository->objects)` call.
>
> I think it was added for good reasons in b739d971 (connected.c:
> reprepare packs for corner cases, 2020-03-13) and I am not sure
> odb_for_each_object_ext() is performing something similar.
>
> At least the commit message should mention this change and explain a
> bit why the reasons the call was added are not valid anymore.
Yeah, I think you're both correct. The only explanation I have is that I
might have repeatedly misread this as `odb_prepare_alternates()`, which
is something we often call before suck loops.
> > do {
> > - struct packed_git *p;
> > -
> > - repo_for_each_pack(the_repository, p) {
> > - if (!p->pack_promisor)
> > - continue;
> > - if (find_pack_entry_one(oid, p))
> > - goto promisor_pack_found;
> > + opts.prefix = oid;
> > +
> > + err = odb_for_each_object_ext(the_repository->objects,
> > + NULL, promised_object_cb,
> > + NULL, &opts);
> > + if (err < 0)
> > + break;
> > + if (err > 0) {
> > + err = 0;
> > + continue;
> > }
> > +
> > /*
> > * Fallback to rev-list with oid and the rest of the
> > * object IDs provided by fn.
> > */
> > goto no_promisor_pack_found;
> > -promisor_pack_found:
> > - ;
> > } while ((oid = fn(cb_data)) != NULL);
> > +
> > if (opt->err_fd)
> > close(opt->err_fd);
> > - return 0;
> > + return err;
> > }
> >
> > no_promisor_pack_found:
>
> These changes are difficult to understand as there are a number of
> `goto`, `break`, `return`, etc involved.
Yeah, agreed. I had my issues understanding this logic, too.
> I think it comes in the first place from check_connected() doing too
> many things, and adding a preparatory commit to refactor it would
> help.
>
> For example the preparatory commit could move a lot of code from
> check_connected() to the following new functions:
I'll give that a try, thanks!
Patrick
^ permalink raw reply
* [PATCH] gpg-interface: fix strip_cr_before_lf to only remove CR before LF
From: Antonio De Stefani @ 2026-06-24 9:36 UTC (permalink / raw)
To: git; +Cc: Antonio De Stefani
c4adea82 (Convert CR/LF to LF in tag signatures, 2008-07-11)
introduced CR stripping for GPG output on Windows, but intentionally
stripped all CR characters unconditionally to "keep the code simpler",
even though only \r\n sequences (Windows line endings) needed to be
normalized. 2f47eae2 (Split GPG interface into its own helper library,
2011-09-07) moved the code into gpg-interface.c, and 29b31577 (ssh
signing: add ssh key format and signing code, 2021-09-10) extracted
it into the remove_cr_after() helper when adding SSH signing support.
The original laziness was safe at the time because lone CR characters
are not expected in GPG signature output. However, the NEEDSWORK
comment left by a previous reader correctly identified that only
\r\n pairs should be stripped, not lone \r characters.
Fix the loop to skip \r only when immediately followed by \n, keeping
lone trailing CR characters intact. Rename the function to
strip_cr_before_lf to reflect its corrected behavior, and update
both call sites and their comments accordingly.
Signed-off-by: Antonio De Stefani <antonio.destefani08@gmail.com>
---
gpg-interface.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index dafd5371fa..95abf1ef4e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -990,21 +990,18 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
return ret;
}
-/*
- * Strip CR from the line endings, in case we are on Windows.
- * NEEDSWORK: make it trim only CRs before LFs and rename
- */
-static void remove_cr_after(struct strbuf *buffer, size_t offset)
+/* Strip CR before LF from the line endings, in case we are on Windows. */
+static void strip_cr_before_lf(struct strbuf *buffer, size_t offset)
{
size_t i, j;
for (i = j = offset; i < buffer->len; i++) {
- if (buffer->buf[i] != '\r') {
- if (i != j)
- buffer->buf[j] = buffer->buf[i];
- j++;
- }
+ if (buffer->buf[i] == '\r' &&
+ i + 1 < buffer->len && buffer->buf[i + 1] == '\n')
+ continue;
+ buffer->buf[j++] = buffer->buf[i];
}
+
strbuf_setlen(buffer, j);
}
@@ -1049,8 +1046,8 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
}
strbuf_release(&gpg_status);
- /* Strip CR from the line endings, in case we are on Windows. */
- remove_cr_after(signature, bottom);
+ /* Strip CR before LF from the line endings, in case we are on Windows. */
+ strip_cr_before_lf(signature, bottom);
return 0;
}
@@ -1136,8 +1133,8 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
ssh_signature_filename.buf);
goto out;
}
- /* Strip CR from the line endings, in case we are on Windows. */
- remove_cr_after(signature, bottom);
+ /* Strip CR before LF from the line endings, in case we are on Windows. */
+ strip_cr_before_lf(signature, bottom);
out:
if (key_file)
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 11/11] reftable/table: fix OOB read on truncated table
From: Patrick Steinhardt @ 2026-06-24 9:46 UTC (permalink / raw)
To: oxsignal; +Cc: git
In-Reply-To: <20260624181426.NJDNpVd1RE-qJjBVh5jtQg@awo.kakao.com>
On Wed, Jun 24, 2026 at 06:14:26PM +0900, oxsignal wrote:
> Hi Patrick,
>
> Thanks for the patch series, for adding the dedicated reftable fuzzer, and for
> the credit.
>
> I reviewed the cover letter and the reftable hardening patches. Patch 05/11
> matches the OOB write case I reported:
> the new minimum block-size validation before handling the log block prevents
> the bogus inflated-size underflow from reaching the inflate/copy path.
>
> The rest of the series also looks like a good cleanup of the corrupted reftable
> parser surface, especially the restart-count/restart-offset and truncated-table
> checks.
> If I find any remaining malformed-table case that is not covered by this
> series, I will follow up with the reproducer.
>
> Thanks again for handling this so quickly.
Perfect, thanks for the report and reading through the patches!
Patrick
^ permalink raw reply
* Re: [PATCH v3 4/4] notes: support an external command to display notes
From: Siddh Raman Pant @ 2026-06-24 9:53 UTC (permalink / raw)
To: j6t@kdbg.org
Cc: oswald.buddenhagen@gmx.de, gitster@pobox.com,
code@khaugsbakk.name, peff@peff.net, ps@pks.im,
git@vger.kernel.org, sandals@crustytoothpaste.net,
newren@gmail.com
In-Reply-To: <3a2ba6c0-4ced-4d2c-820e-401c2dff1dd1@kdbg.org>
[-- Attachment #1: Type: text/plain, Size: 2495 bytes --]
On Wed, Jun 24 2026 at 13:19:26 +0530, Johannes Sixt wrote:
> > One solution to this is to move the freshness policy out of git so that
> > it is someone else's problem. We can have a realtime fetch or faster
> > updation via external helper means. But unfortunately we lose the
> > coherence in the display of information, and so the user would end up
> > reinventing git log in his quest to have same workflow.
>
> You are presenting one solution here. But a more obvious solution would
> have been to make Git's notes implementation capable enough to keep up
> with the volume of notes that are produced by your team.
Git storage is inherently based on refs, so that would require massive
changes IMO. The actual fundamental problem here is that only the
latest state is useful at any given point of time, and not the past
history.
> Another solution would be to track the information outside of Git notes
> entirely, similar to how pull requests, issues, reviews, and
> conversations are tracked by Git hosters in databases outside of Git.
This is precisely what this allows for. The information is tracked
outside of Git, and the notes path just shows it along with the commit.
A developer works on the code using Git. An external website doesn't
allow the same level of coherence in display of information as a note.
The commit is a fundamental unit of change. IMO it makes sense for Git
to be able to show a note about it from a provided external medium.
> > Let's add support for notes.externalCommand, a protected-configuration
> > command that git runs as a long-lived helper when displaying notes. git
> > sends commit IDs to the helper and displays any returned text through
> > the existing notes formatting path. This keeps presentation in git
> > while letting the helper decide how fresh note text is obtained.
>
> To my eyes, this looks like an overengineered solution that helps one
> user of a niche feature of Git.
This can also allow for other uses too. For example, searching lore I
just found out that a colleague in Oracle Linux (Vegard) was trying to
solve a related problem in 2022:
https://lore.kernel.org/git/20220802075401.2393-1-vegard.nossum@oracle.com/
I think it was for achieving something like this more generally:
https://git.kernel.org/pub/scm/linux/kernel/git/vegard/linux.git/commit/?id=339f83612f3a569b194680768b22bf113c26a29d
An external notes command can be a solution for it.
Thanks,
Siddh
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH RFC v2 2/2] Move libgit.a sources into separate "lib/" directory
From: Patrick Steinhardt @ 2026-06-24 10:32 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, brian m. carlson, Elijah Newren, Derrick Stolee,
Phillip Wood
In-Reply-To: <xmqqcxxi3eov.fsf@gitster.g>
On Mon, Jun 22, 2026 at 06:08:48AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > The Git project is not exactly the easiest project to get started in:
> > it's written in C and POSIX shell, with bits of Perl, Rust and other
> > languages sprinkled into it. On top of that, the project has grown
> > somewhat organically over time, making the codebase hard to navigate.
> >
> > But there is a rather practical problem: finding your way around in our
> > project's tree is not easy. Doing a directory listing in the top-level
> > directory will present you with more than 550 files, which makes it
> > extremely hard for a newcomer to figure out what files they are even
> > supposed to look at.
>
> Well, many things already live in the dedicated corner of their own
> universe, like tests in t/, builtins in builtins/, and documentation
> in Documentation/. This is pretty much moving everything else in a
> single directory lib/. Surely there are things like top-level
> Makefile and other build- and ci-related things that do not move to
> lib/ for obvious reasons, but I view this move essentially to change
> "for core-ish and library-ish things, look at the top level
> directory" to "for core-ish and library-ish things look at lib/
> directory".
Right. It does drown out the things that have to live in the root
directory though, for example files like README.md or SECURITY.md.
> Would that make it easier to navigate? I am not sure. What I am
> sure is that this will force many in-flight topic (and soon to be
> in-flight because people are holding them back during the prerelease
> freeze period) to be updated, and it will make it harder to make
> fixes that can apply both to 2.55 and before and newer codebase.
That's definitely a downside, I agree.
I have a bit of a different angle on the second part: it's not uncommon
that projects move stuff around, and if Git cannot handle those
scenarios well that's a usability issue that we'd ideally fix. And by
doing such a rename we basically subject ourselves to the same pain that
other projects are seeing, which might give us more incentive to
actually fix those pain points.
This might be a bit of a weird take and might raise some eyebrows. But
it's basically a tooling issue, and we are the ones who provide the
tooling. So we're in the best position to fix it, and by doing so make
everyone elses lives easier, as well.
Who knows how good submodules would have been nowadays if we had used
them ourselves? :P
> So, my initial reaction is somewhat negative, but I am known to
> accept changes that I myself do not necessarily agree with, so...
That's fair, and just to state the obvious: I'm still happy if the
community decides that this is not worth doing. But from what I've seen
until now it seems like most feedback I got was rather positive. Which
honestly surprised me, I was expecting more pushback.
Thanks!
Patrick
^ permalink raw reply
* [PATCH v2 0/4] connected: search promisor objects generically
From: Patrick Steinhardt @ 2026-06-24 10:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Christian Couder
In-Reply-To: <20260622-pks-connected-generic-promisor-checks-v1-0-25eba2698202@pks.im>
Hi,
this patch series refactors "connected.c" so that we search for promisor
objects in a generic way instead of reaching into internal of the object
database. As a result, the connectivity checks will work properly in
repos that don't use packfiles in the first place.
The series is built on top of 8d96f09e92 (Merge branch
'js/objects-larger-than-4gb-on-windows', 2026-06-19) with
ps/odb-source-packed at 1bba3c035d (odb/source-packed: drop pointer to
"files" parent source, 2026-06-17) merged into it.
Changes in v2:
- Fix the accidentally-dropped call to `odb_reprepare()`.
- Add a preparatory commit that splits out `check_connected_promisor()`.
I think also splitting out `check_connected_rev_list()` would only
have diminishing returns, so I skipped that part.
- Link to v1: https://patch.msgid.link/20260622-pks-connected-generic-promisor-checks-v1-0-25eba2698202@pks.im
Thanks!
Patrick
---
Patrick Steinhardt (4):
odb/source-packed: extract logic to skip certain packs
odb/source-packed: support flags when iterating an object prefix
connected: split out promisor-based connectivity check
connected: search promisor objects generically
connected.c | 95 ++++++++++++++++++++++++++++++++++-------------------
odb/source-packed.c | 50 ++++++++++++++++++++--------
2 files changed, 98 insertions(+), 47 deletions(-)
Range-diff versus v1:
1: 6ff1fc8d89 = 1: a1a1af0fc6 odb/source-packed: extract logic to skip certain packs
2: 1022a1fdcc = 2: bd81a9e478 odb/source-packed: support flags when iterating an object prefix
3: 102fab7df2 < -: ---------- connected: search promisor objects generically
-: ---------- > 3: f39ef68c3e connected: split out promisor-based connectivity check
-: ---------- > 4: 558f30a6f2 connected: search promisor objects generically
---
base-commit: 4a8e7a446f41435e157131162dfe901eca9250fe
change-id: 20260612-pks-connected-generic-promisor-checks-2933bff3028d
^ permalink raw reply
* [PATCH v2 1/4] odb/source-packed: extract logic to skip certain packs
From: Patrick Steinhardt @ 2026-06-24 10:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Christian Couder
In-Reply-To: <20260624-pks-connected-generic-promisor-checks-v2-0-132d73ee47b9@pks.im>
The caller can pass flags that allow them to filter out specific kinds
of objects when iterating objects via `odb_for_each_object()`. This only
works for "normal" iteration though, as we `BUG()` when the user passes
flags and specifies an object prefix.
This limitation will be lifted in the next commit. Prepare for this by
extracting the logic that skips certain kinds of packs so that we can
easily reuse it.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
odb/source-packed.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 42c28fba0e..3afc4bf01f 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -126,6 +126,22 @@ static int match_hash(unsigned len, const unsigned char *a, const unsigned char
return 1;
}
+static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_flags flags)
+{
+ if ((flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
+ return true;
+ if ((flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&
+ !p->pack_promisor)
+ return true;
+ if ((flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&
+ p->pack_keep_in_core)
+ return true;
+ if ((flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&
+ p->pack_keep)
+ return true;
+ return false;
+}
+
static int for_each_prefixed_object_in_midx(
struct odb_source_packed *store,
struct multi_pack_index *m,
@@ -306,17 +322,9 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
for (e = packfile_store_get_packs(packed); e; e = e->next) {
struct packed_git *p = e->pack;
- if ((opts->flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
- continue;
- if ((opts->flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&
- !p->pack_promisor)
- continue;
- if ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&
- p->pack_keep_in_core)
- continue;
- if ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&
- p->pack_keep)
+ if (should_exclude_pack(p, opts->flags))
continue;
+
if (open_pack_index(p)) {
pack_errors = 1;
continue;
--
2.55.0.rc1.745.g43192e7977.dirty
^ permalink raw reply related
* [PATCH v2 2/4] odb/source-packed: support flags when iterating an object prefix
From: Patrick Steinhardt @ 2026-06-24 10:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Christian Couder
In-Reply-To: <20260624-pks-connected-generic-promisor-checks-v2-0-132d73ee47b9@pks.im>
Callers of `odb_for_each_object()` can specify an optional object name
prefix so that we only yield objects that match it. This is incompatible
though with passing flags at the same time, as we don't yet know to
handle them.
Loosen this restriction by calling `should_exclude_pack()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
odb/source-packed.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 3afc4bf01f..6f31f0ff94 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -148,6 +148,7 @@ static int for_each_prefixed_object_in_midx(
const struct odb_for_each_object_options *opts,
struct odb_source_packed_for_each_object_wrapper_data *data)
{
+ bool pack_errors = false;
int ret;
for (; m; m = m->base_midx) {
@@ -171,6 +172,20 @@ static int for_each_prefixed_object_in_midx(
const struct object_id *current = NULL;
struct object_id oid;
+ if (opts->flags) {
+ uint32_t pack_id = nth_midxed_pack_int_id(m, i);
+ struct packed_git *pack;
+
+ if (prepare_midx_pack(m, pack_id)) {
+ pack_errors = true;
+ continue;
+ }
+
+ pack = nth_midxed_pack(m, pack_id);
+ if (should_exclude_pack(pack, opts->flags))
+ continue;
+ }
+
current = nth_midxed_object_oid(&oid, m, i);
if (!match_hash(len, opts->prefix->hash, current->hash))
@@ -198,6 +213,8 @@ static int for_each_prefixed_object_in_midx(
ret = 0;
out:
+ if (!ret && pack_errors)
+ ret = -1;
return ret;
}
@@ -260,9 +277,6 @@ static int odb_source_packed_for_each_prefixed_object(
bool pack_errors = false;
int ret;
- if (opts->flags)
- BUG("flags unsupported");
-
store->skip_mru_updates = true;
m = get_multi_pack_index(store);
@@ -275,6 +289,8 @@ static int odb_source_packed_for_each_prefixed_object(
for (e = packfile_store_get_packs(store); e; e = e->next) {
if (e->pack->multi_pack_index)
continue;
+ if (should_exclude_pack(e->pack, opts->flags))
+ continue;
if (open_pack_index(e->pack)) {
pack_errors = true;
--
2.55.0.rc1.745.g43192e7977.dirty
^ permalink raw reply related
* [PATCH v2 3/4] connected: split out promisor-based connectivity check
From: Patrick Steinhardt @ 2026-06-24 10:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Christian Couder
In-Reply-To: <20260624-pks-connected-generic-promisor-checks-v2-0-132d73ee47b9@pks.im>
When performing a connectivity check in a partial clone we try to avoid
doing the connectivity check by checking whether all new tips are part
of a promisor pack. This makes use of the fact that we don't expect full
connectivity for promised objects anyway, so it's basically fine if
those objects are not fully connected.
The logic that handles this promisor-based check is somewhat hard to
read though as it uses nested loops and gotos. Pull it out into a
standalone function, which makes it a bit easier to reason about.
We'll also further simplify the function in the next commit.
Suggested-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
connected.c | 85 ++++++++++++++++++++++++++++++++++++-------------------------
1 file changed, 51 insertions(+), 34 deletions(-)
diff --git a/connected.c b/connected.c
index 7e26976832..d2b334173f 100644
--- a/connected.c
+++ b/connected.c
@@ -11,6 +11,49 @@
#include "packfile.h"
#include "promisor-remote.h"
+/*
+ * For partial clones, we don't want to have to do a regular connectivity check
+ * because we have to enumerate and exclude all promisor objects (slow), and
+ * then the connectivity check itself becomes a no-op because in a partial
+ * clone every object is a promisor object. Instead, just make sure we
+ * received, in a promisor packfile, the objects pointed to by each wanted ref.
+ *
+ * Before checking for promisor packs, be sure we have the latest pack-files
+ * loaded into memory.
+ *
+ * Returns 1 when all object IDs have been found in promisor packs, in which
+ * case we're fully connected and thus done. Returns 0 when we have found
+ * objects in non-promisor packs, in which case we'll have to fall back to the
+ * rev-list-based connectivity checks. Returns a negative error code on error.
+ */
+static int check_connected_promisor(oid_iterate_fn fn,
+ void *cb_data,
+ const struct object_id **oid)
+{
+ odb_reprepare(the_repository->objects);
+ do {
+ struct packed_git *p;
+
+ repo_for_each_pack(the_repository, p) {
+ if (!p->pack_promisor)
+ continue;
+ if (find_pack_entry_one(*oid, p))
+ goto promisor_pack_found;
+ }
+
+ /*
+ * We have found an object that is not part of a promisor pack,
+ * and thus we cannot skip the full connectivity check.
+ */
+ return 0;
+
+promisor_pack_found:
+ ;
+ } while ((*oid = fn(cb_data)) != NULL);
+
+ return 1;
+}
+
/*
* If we feed all the commits we want to verify to this command
*
@@ -46,42 +89,16 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
}
if (repo_has_promisor_remote(the_repository)) {
- /*
- * For partial clones, we don't want to have to do a regular
- * connectivity check because we have to enumerate and exclude
- * all promisor objects (slow), and then the connectivity check
- * itself becomes a no-op because in a partial clone every
- * object is a promisor object. Instead, just make sure we
- * received, in a promisor packfile, the objects pointed to by
- * each wanted ref.
- *
- * Before checking for promisor packs, be sure we have the
- * latest pack-files loaded into memory.
- */
- odb_reprepare(the_repository->objects);
- do {
- struct packed_git *p;
-
- repo_for_each_pack(the_repository, p) {
- if (!p->pack_promisor)
- continue;
- if (find_pack_entry_one(oid, p))
- goto promisor_pack_found;
- }
- /*
- * Fallback to rev-list with oid and the rest of the
- * object IDs provided by fn.
- */
- goto no_promisor_pack_found;
-promisor_pack_found:
- ;
- } while ((oid = fn(cb_data)) != NULL);
- if (opt->err_fd)
- close(opt->err_fd);
- return 0;
+ err = check_connected_promisor(fn, cb_data, &oid);
+ if (err) {
+ if (opt->err_fd)
+ close(opt->err_fd);
+ if (err > 0)
+ err = 0;
+ return err;
+ }
}
-no_promisor_pack_found:
if (opt->shallow_file) {
strvec_push(&rev_list.args, "--shallow-file");
strvec_push(&rev_list.args, opt->shallow_file);
--
2.55.0.rc1.745.g43192e7977.dirty
^ permalink raw reply related
* [PATCH v2 4/4] connected: search promisor objects generically
From: Patrick Steinhardt @ 2026-06-24 10:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Christian Couder
In-Reply-To: <20260624-pks-connected-generic-promisor-checks-v2-0-132d73ee47b9@pks.im>
When performing connectivity checks we have to figure out whether any of
the new objects are promisor objects, as we cannot assume full
connectivity if so.
This check is performed by iterating through all packfiles in the
repository and searching each of them for the given object. Of course,
this mechanism is quite specific to implementation details of the object
database, as we assume that it uses packfiles in the first place.
Refactor the logic so that we instead use `odb_for_each_object_ext()`
with an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`
flag. This will yield all objects that have the exact object name and
that are part of a promisor pack in a generic way.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
connected.c | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/connected.c b/connected.c
index d2b334173f..b557ff5db9 100644
--- a/connected.c
+++ b/connected.c
@@ -11,6 +11,13 @@
#include "packfile.h"
#include "promisor-remote.h"
+static int promised_object_cb(const struct object_id *oid UNUSED,
+ struct object_info *oi UNUSED,
+ void *payload UNUSED)
+{
+ return 1;
+}
+
/*
* For partial clones, we don't want to have to do a regular connectivity check
* because we have to enumerate and exclude all promisor objects (slow), and
@@ -30,25 +37,28 @@ static int check_connected_promisor(oid_iterate_fn fn,
void *cb_data,
const struct object_id **oid)
{
+ struct odb_for_each_object_options opts = {
+ .flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY,
+ .prefix_hex_len = the_repository->hash_algo->hexsz,
+ };
+ int err;
+
odb_reprepare(the_repository->objects);
do {
- struct packed_git *p;
+ opts.prefix = *oid;
- repo_for_each_pack(the_repository, p) {
- if (!p->pack_promisor)
- continue;
- if (find_pack_entry_one(*oid, p))
- goto promisor_pack_found;
- }
+ err = odb_for_each_object_ext(the_repository->objects,
+ NULL, promised_object_cb,
+ NULL, &opts);
+ if (err < 0)
+ return err;
/*
* We have found an object that is not part of a promisor pack,
* and thus we cannot skip the full connectivity check.
*/
- return 0;
-
-promisor_pack_found:
- ;
+ if (err > 0)
+ return 0;
} while ((*oid = fn(cb_data)) != NULL);
return 1;
--
2.55.0.rc1.745.g43192e7977.dirty
^ permalink raw reply related
* Re: [PATCH RFC v2 2/2] Move libgit.a sources into separate "lib/" directory
From: Oswald Buddenhagen @ 2026-06-24 11:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Patrick Steinhardt, git, brian m. carlson, Elijah Newren,
Derrick Stolee, Phillip Wood
In-Reply-To: <xmqqcxxi3eov.fsf@gitster.g>
On Mon, Jun 22, 2026 at 06:08:48AM -0700, Junio C Hamano wrote:
>it will make it harder to make
>fixes that can apply both to 2.55 and before and newer codebase.
>
which is why i would recommend applying this _before_ the release. it
affects only the build, which ci checks rather thoroughly, so the risk
of doing it late in the cycle is low.
this obviously won't help with backporting further back, nor will it
avoid having to rebase many pending branches, but see patrick's response
for that.
fwiw, i'm totally in favor of the change. i've always found the current
tree messy and confusing.
^ permalink raw reply
* Re: [PATCH/RFC 2/6] commit-reach: introduce struct paint_queue with per-side counters
From: Kristofer Karlsson @ 2026-06-24 11:25 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Kristofer Karlsson via GitGitGadget, git, Elijah Newren
In-Reply-To: <ec241a02-546c-4b5f-8ef7-06b4355d8fec@gmail.com>
On Tue, 23 Jun 2026 at 16:17, Derrick Stolee <stolee@gmail.com> wrote:
>
> I think this would be an appropriate way to handle this. If we
> pop and return NULL then it's ok that we removed data from the
> queue because it shouldn't be reused.
I have prepared v2 on GGG which I believe addresses all of the
feedback. The halt conditions now live inside paint_queue_get()
as you suggested.
I am not 100% happy with the halt-condition placement yet --
the existing loop in master already has several exit paths
(while condition, min_generation break, FIND_ALL break) and I
think there is an opportunity to consolidate them. But that is
a separate discussion and I do not want to derail this series.
I can propose some alternatives in a follow-up after this
lands.
Thanks,
Kristofer
^ permalink raw reply
* Re: [PATCH 2/6] object-file: propagate files transaction errors
From: Patrick Steinhardt @ 2026-06-24 11:26 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <20260624041920.2601961-3-jltobler@gmail.com>
On Tue, Jun 23, 2026 at 11:19:16PM -0500, Justin Tobler wrote:
> The "files" transaction backend may encounter errors related to managing
> the temporary directory used to stage objects, but silently ignores
> these errors. Instead return errors encountered in the
> `odb_transaction_files_{prepare,begin,commit}()` interfaces to allow
> callers to handle as needed.
Missing a then? "to handle as needed" -> "to handle them as needed"
Makes sense. It always felt a bit off that those functions didn't have a
way to signal errors to the caller.
> diff --git a/object-file.c b/object-file.c
> index a3eb8d71dd..18c2df75fb 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -499,7 +499,7 @@ struct odb_transaction_files {
> struct transaction_packfile packfile;
> };
>
> -static void odb_transaction_files_prepare(struct odb_transaction *base)
> +static int odb_transaction_files_prepare(struct odb_transaction *base)
> {
> struct odb_transaction_files *transaction =
> container_of_or_null(base, struct odb_transaction_files, base);
By the way, is there any reason why those functions are still hosted in
"object-file.c" instead of in "odb/source-files.c"? I should probably
know, but I forgot.
> @@ -511,11 +511,15 @@ static void odb_transaction_files_prepare(struct odb_transaction *base)
> * added at the time they call odb_transaction_files_begin.
> */
> if (!transaction || transaction->objdir)
> - return;
> + return 0;
>
> transaction->objdir = tmp_objdir_create(base->source->odb->repo, "bulk-fsync");
> - if (transaction->objdir)
> - tmp_objdir_replace_primary_odb(transaction->objdir, 0);
> + if (!transaction->objdir)
> + return -1;
Huh. So previously we just didn't handle this error at all and just
continued to tag along? Did that result in anything sensible or was this
just YOLOing it?
> @@ -542,13 +546,13 @@ static void fsync_loose_object_transaction(struct odb_transaction *base,
> /*
> * Cleanup after batch-mode fsync_object_files.
> */
> -static void flush_loose_object_transaction(struct odb_transaction_files *transaction)
> +static int flush_loose_object_transaction(struct odb_transaction_files *transaction)
Feels like this function should've been renamed in the preceding commit,
as well.
> {
> struct strbuf temp_path = STRBUF_INIT;
> struct tempfile *temp;
>
> if (!transaction->objdir)
> - return;
> + return 0;
>
> /*
> * Issue a full hardware flush against a temporary file to ensure
> @@ -570,8 +574,12 @@ static void flush_loose_object_transaction(struct odb_transaction_files *transac
There is a call to `xmks_tempfile()` hidden that can fail, but that
failure is already handled in that function itself by dying.
> * Make the object files visible in the primary ODB after their data is
> * fully durable.
> */
> - tmp_objdir_migrate(transaction->objdir);
> + if (tmp_objdir_migrate(transaction->objdir))
> + return -1;
Feels like another case of YOLOing it. The migration could have failed,
but we just ignored that failure and never told the user about it. The
result may be silent corruption, I assume?
> @@ -1670,27 +1678,34 @@ int read_loose_object(struct repository *repo,
> return ret;
> }
>
> -static void odb_transaction_files_commit(struct odb_transaction *base)
> +static int odb_transaction_files_commit(struct odb_transaction *base)
> {
> struct odb_transaction_files *transaction =
> container_of(base, struct odb_transaction_files, base);
>
> - flush_loose_object_transaction(transaction);
> + if (flush_loose_object_transaction(transaction))
> + return -1;
> flush_packfile_transaction(transaction);
> +
> + return 0;
> }
>
> -struct odb_transaction *odb_transaction_files_begin(struct odb_source *source)
> +int odb_transaction_files_begin(struct odb_source *source,
> + struct odb_transaction **out)
> {
> struct odb_transaction_files *transaction;
> struct object_database *odb = source->odb;
>
> - if (odb->transaction)
> - return NULL;
> + if (odb->transaction) {
> + *out = NULL;
> + return 0;
> + }
>
> transaction = xcalloc(1, sizeof(*transaction));
> transaction->base.source = source;
> transaction->base.commit = odb_transaction_files_commit;
> transaction->base.write_object_stream = odb_transaction_files_write_object_stream;
> + *out = &transaction->base;
>
> - return &transaction->base;
> + return 0;
> }
It's still somewhat fishy that we have this ODB-level transaction, but
that's a preexisting issue and thus outside the scope of this patch
series. Ideally though, it would be possible for there to be multiple
transactions, and it would be the caller's responsibility for juggling
these transactions. Just as it happens with reference transactions.
> diff --git a/odb/transaction.h b/odb/transaction.h
> index 854fda06f5..f4c1ebfaaa 100644
> --- a/odb/transaction.h
> +++ b/odb/transaction.h
> @@ -17,7 +17,7 @@ struct odb_transaction {
> struct odb_source *source;
>
> /* The ODB source specific callback invoked to commit a transaction. */
> - void (*commit)(struct odb_transaction *transaction);
> + int (*commit)(struct odb_transaction *transaction);
We might want to document the returned error code here.
Patrick
^ permalink raw reply
* Re: [PATCH 3/6] odb/transaction: propagate begin errors
From: Patrick Steinhardt @ 2026-06-24 11:26 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <20260624041920.2601961-4-jltobler@gmail.com>
On Tue, Jun 23, 2026 at 11:19:17PM -0500, Justin Tobler wrote:
> diff --git a/odb/transaction.c b/odb/transaction.c
> index b16e07aebf..d3de01db50 100644
> --- a/odb/transaction.c
> +++ b/odb/transaction.c
> @@ -2,14 +2,20 @@
> #include "odb/source.h"
> #include "odb/transaction.h"
>
> -struct odb_transaction *odb_transaction_begin(struct object_database *odb)
> +int odb_transaction_begin(struct object_database *odb,
> + struct odb_transaction **out)
> {
> - if (odb->transaction)
> - return NULL;
> + int ret;
>
> - odb_source_begin_transaction(odb->sources, &odb->transaction);
> + if (odb->transaction) {
> + *out = NULL;
> + return 0;
> + }
Hm. So we may return successful, but not set the `out` pointer to a
transaction. And...
> diff --git a/odb/transaction.h b/odb/transaction.h
> index f4c1ebfaaa..cd6d50f2e5 100644
> --- a/odb/transaction.h
> +++ b/odb/transaction.h
> @@ -33,11 +35,20 @@ struct odb_transaction {
> };
>
> /*
> - * Starts an ODB transaction. Subsequent objects are written to the transaction
> - * and not committed until odb_transaction_commit() is invoked on the
> - * transaction. If the ODB already has a pending transaction, NULL is returned.
> + * Starts an ODB transaction and returns it via `out`. Subsequent objects are
> + * written to the transaction and not committed until odb_transaction_commit()
> + * is invoked on the transaction. Returns 0 on success and a negative value on
> + * error. If the ODB already has a pending transaction, `out` is set to NULL.
> */
> -struct odb_transaction *odb_transaction_begin(struct object_database *odb);
> +int odb_transaction_begin(struct object_database *odb,
> + struct odb_transaction **out);
> +
> +static inline void odb_transaction_begin_or_die(struct object_database *odb,
> + struct odb_transaction **out)
> +{
> + if (odb_transaction_begin(odb, out))
> + die(_("failed to start ODB transaction"));
> +}
... we don't special-case that here, either. So a caller may invoke the
function, not die, but it might still not have a valid transaction. That
feels wrong to me.
Patrick
^ permalink raw reply
* Re: [PATCH 4/6] odb/transaction: propagate commit errors
From: Patrick Steinhardt @ 2026-06-24 11:26 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <20260624041920.2601961-5-jltobler@gmail.com>
On Tue, Jun 23, 2026 at 11:19:18PM -0500, Justin Tobler wrote:
> diff --git a/odb/transaction.h b/odb/transaction.h
> index cd6d50f2e5..7898770071 100644
> --- a/odb/transaction.h
> +++ b/odb/transaction.h
> @@ -54,7 +54,7 @@ static inline void odb_transaction_begin_or_die(struct object_database *odb,
> * Commits an ODB transaction making the written objects visible. If the
> * specified transaction is NULL, the function is a no-op.
> */
> -void odb_transaction_commit(struct odb_transaction *transaction);
> +int odb_transaction_commit(struct odb_transaction *transaction);
Should the function comment be amended, as well? We should definitely
point out that calling this with a NULL transaction also returns
success.
Patrick
^ permalink raw reply
* Re: [PATCH 5/6] odb/transaction: add transaction env interface
From: Patrick Steinhardt @ 2026-06-24 11:26 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <20260624041920.2601961-6-jltobler@gmail.com>
On Tue, Jun 23, 2026 at 11:19:19PM -0500, Justin Tobler wrote:
> The ODB transaction backend is responsible for creating/managing its own
> staging area for writing objects. Other child processes spawned by Git
> may need to access to uncommitted objects or write new objects in the
s/may need to access to/may need access to/
> staging area though.
>
> Introduce `odb_transaction_env()` which is expected to provide the set
> of environment variables needed by a child process to access the
> transaction staging area.
Possessive s is missing, I think.
> diff --git a/object-file.c b/object-file.c
> index 696f05dc2d..14064d188a 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1691,6 +1691,16 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
> return 0;
> }
>
> +static const char **odb_transaction_files_env(struct odb_transaction *base)
> +{
> + struct odb_transaction_files *transaction =
> + container_of(base, struct odb_transaction_files, base);
> +
> + odb_transaction_files_prepare(&transaction->base);
> +
> + return tmp_objdir_env(transaction->objdir);
> +}
> +
> int odb_transaction_files_begin(struct odb_source *source,
> struct odb_transaction **out)
> {
Makes sense. Transactions may have a different way to quarantine the
write than using a quarantine directory. So making this functionality
pluggable so that backends may expose a separate set of environment
variables feels sensible.
> diff --git a/odb/transaction.h b/odb/transaction.h
> index 7898770071..536458297b 100644
> --- a/odb/transaction.h
> +++ b/odb/transaction.h
> @@ -32,6 +32,16 @@ struct odb_transaction {
> int (*write_object_stream)(struct odb_transaction *transaction,
> struct odb_write_stream *stream, size_t len,
> struct object_id *oid);
> +
> + /*
> + * This callback is expected to return a NULL-terminated array of
> + * environment variables that a child process should inherit so
> + * that its object writes participate in the transaction. The
> + * returned array is owned by the backend and remains valid until
> + * the transaction ends. May return NULL when the backend does not
> + * need to expose any state to child processes.
> + */
> + const char **(*env)(struct odb_transaction *transaction);
Would it make more sense to adapt this function so that:
- It receives a `struct strvec` as input that the environment
variables are to be amended to.
- It returns a normal error code to indicate errors?
Patrick
^ permalink raw reply
* Re: [PATCH 6/6] builtin/receive-pack: stage incoming objects via ODB transactions
From: Patrick Steinhardt @ 2026-06-24 11:26 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <20260624041920.2601961-7-jltobler@gmail.com>
On Tue, Jun 23, 2026 at 11:19:20PM -0500, Justin Tobler wrote:
> Objects received by git-receive-pack(1) are quarantined in a temporary
> "incoming" directory and migrated into the object database prior to the
> reference updates. The quarantine is currently managed through
> `tmp_objdir` directly. In a pluggable ODB future, how exactly an object
> gets written to a transaction may vary for a given ODB source. Refactor
> git-receive-pack(1) to use the ODB transaction interfaces to manage the
> object staging area in a more agnostic manner accordingly.
>
> Note that the temporary directory created for git-receive-pack(1) is
> eagerly created and uses a different prefix name. This behavior is
A different prefix name compared to what?
> special cased in the "files" backend by having `odb_transaction_begin()`
> callers that require this behavior provide an `ODB_TRANSACTION_RECEIVE`
> flag.
Okay. I guess this is to retain existing behaviour where the temporary
directory is created lazily everywhere else. Makes me wonder whether we
should eventually change this to just unconditionally create the
directory in all cases so that we can drop this new flag.
It might've also made sense to split this commit up into two: one to
introduce the flag parameter, and then one to do the changes to
git-receive-pack(1).
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 19eb6a1b61..ee8e03e2ab 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -112,8 +112,6 @@ static enum {
> } use_keepalive;
> static int keepalive_in_sec = 5;
>
> -static struct tmp_objdir *tmp_objdir;
> -
> static struct proc_receive_ref {
> unsigned int want_add:1,
> want_delete:1,
I assume the goal is that we convert all other users of the tmp-objdir
subsystem to also use transactions eventually, so that this becomes an
implementation detail fo the files transaction?
> @@ -2106,14 +2104,13 @@ static void execute_commands(struct command *commands,
> * Now we'll start writing out refs, which means the objects need
> * to be in their final positions so that other processes can see them.
> */
> - if (tmp_objdir_migrate(tmp_objdir) < 0) {
> + if (odb_transaction_commit(the_repository->objects->transaction)) {
> for (cmd = commands; cmd; cmd = cmd->next) {
> if (!cmd->error_string)
> cmd->error_string = "unable to migrate objects to permanent storage";
> }
> return;
> }
> - tmp_objdir = NULL;
We don't need to unset the transaction because that's what
`odb_transaction_commit()` already does for us, I assume?
> @@ -2326,7 +2323,8 @@ static void push_header_arg(struct strvec *args, struct pack_header *hdr)
> ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
> }
>
> -static const char *unpack(int err_fd, struct shallow_info *si)
> +static const char *unpack(int err_fd, struct shallow_info *si,
> + struct odb_transaction *transaction)
> {
> struct pack_header hdr;
> const char *hdr_err;
It feels a bit weird that we sometimes pass the transaction as
parameter, whereas othertimes we access it via `the_repository`.
> @@ -2351,20 +2349,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
> strvec_push(&child.args, alt_shallow_file);
> }
>
> - tmp_objdir = tmp_objdir_create(the_repository, "incoming");
> - if (!tmp_objdir) {
> - if (err_fd > 0)
> - close(err_fd);
> - return "unable to create temporary object directory";
> - }
> - strvec_pushv(&child.env, tmp_objdir_env(tmp_objdir));
> -
> - /*
> - * Normally we just pass the tmp_objdir environment to the child
> - * processes that do the heavy lifting, but we may need to see these
> - * objects ourselves to set up shallow information.
> - */
> - tmp_objdir_add_as_alternate(tmp_objdir);
> + strvec_pushv(&child.env, odb_transaction_env(transaction));
Interesting, this here seems like a change in behaviour. Previously we
added the transactions as an alternate, but now we only propagate it via
the environment. I didn't see this mentioned in the commit message.
> @@ -2707,7 +2694,10 @@ int cmd_receive_pack(int argc,
> if (!si.nr_ours && !si.nr_theirs)
> shallow_update = 0;
> if (!delete_only(commands)) {
> - unpack_status = unpack_with_sideband(&si);
> + if (odb_transaction_begin(the_repository->objects, &transaction, ODB_TRANSACTION_RECEIVE))
> + unpack_status = "unable to start ODB transaction";
s/ODB/object/
This may be visible to the user, and "ODB" may mean nothing to them.
> diff --git a/object-file.c b/object-file.c
> index 14064d188a..e7958753ec 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1702,7 +1703,8 @@ static const char **odb_transaction_files_env(struct odb_transaction *base)
> }
>
> int odb_transaction_files_begin(struct odb_source *source,
> - struct odb_transaction **out)
> + struct odb_transaction **out,
> + enum odb_transaction_flags flags)
> {
> struct odb_transaction_files *transaction;
> struct object_database *odb = source->odb;
> @@ -1717,6 +1719,20 @@ int odb_transaction_files_begin(struct odb_source *source,
> transaction->base.commit = odb_transaction_files_commit;
> transaction->base.write_object_stream = odb_transaction_files_write_object_stream;
> transaction->base.env = odb_transaction_files_env;
> +
> + transaction->prefix = "bulk-fsync";
> + if (flags & ODB_TRANSACTION_RECEIVE) {
> + /*
> + * ODB transactions for git-receive-pack(1) eagerly create a
> + * temporary directory and use a different prefix.
> + */
> + transaction->prefix = "incoming";
> + if (odb_transaction_files_prepare(&transaction->base)) {
> + free(transaction);
> + return -1;
> + }
> + }
> +
Okay, makes sense. I really wonder whether we need to insist this much
on the exact name used by this, but better be safe than sorry for now I
guess.
And as mentioned before, I also wonder whether it really makes sense to
have the lazy creation of the tmp-objdir. Maybe add a NEEDSWORK item
here that mentions we want to investigate whether this is even needed at
all?
> diff --git a/odb/transaction.h b/odb/transaction.h
> index 536458297b..78392ff13d 100644
> --- a/odb/transaction.h
> +++ b/odb/transaction.h
> @@ -44,6 +43,10 @@ struct odb_transaction {
> const char **(*env)(struct odb_transaction *transaction);
> };
>
> +enum odb_transaction_flags {
> + ODB_TRANSACTION_RECEIVE = (1 << 0),
> +};
It's not clear at all what this flag does based on its name, so we
should have documentation for it.
Patrick
^ permalink raw reply
* Re: [PATCH 0/6] receive-pack: use ODB transactions to stage object writes
From: Patrick Steinhardt @ 2026-06-24 11:27 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <20260624041920.2601961-1-jltobler@gmail.com>
On Tue, Jun 23, 2026 at 11:19:14PM -0500, Justin Tobler wrote:
> Greetings,
>
> This patch series replaces direct usage of the `tmp_objdir` interfaces
> in git-receive-pack(1) to instead use the `odb_transaction` interfaces
> to create/manage a staging area to write objects to. The purpose of this
> change is to get git-receive-pack(1) one step closer to being ODB
> backend agnostic. For now, the object writes themselves are still
> "files" backend specific due to being handled by the git-index-pack(1)
> and git-unpack-objects(1) child processes. This will be tackled in a
> separate series though.
Thanks, this was a pleasant read. I've got a bunch of comments, but
overall I really like the direction of this patch series.
Patrick
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox