@matheusbsbs / matheus-bagliano / commits / d857469d85

reftable/reader: introduce refcounting

It was recently reported that concurrent reads and writes may cause the reftable backend to segfault. The root cause of this is that we do not properly keep track of reftable readers across reloads. Suppose that you have a reftable iterator and then decide to reload the stack while iterating through the iterator. When the stack has been rewritten since we have created the iterator, then we would end up discarding a subset of readers that may still be in use by the iterator. The consequence is that we now try to reference deallocated memory, which of course segfaults. One way to trigger this is in t5616, where some background maintenance jobs have been leaking from one test into another. This leads to stack traces like the following one: + git -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 --refetch origin AddressSanitizer:DEADLYSIGNAL ================================================================= ==657994==ERROR: AddressSanitizer: SEGV on unknown address 0x7fa0f0ec6089 (pc 0x55f23e52ddf9 bp 0x7ffe7bfa1700 sp 0x7ffe7bfa1700 T0) ==657994==The signal is caused by a READ memory access. #0 0x55f23e52ddf9 in get_var_int reftable/record.c:29 #1 0x55f23e53295e in reftable_decode_keylen reftable/record.c:170 #2 0x55f23e532cc0 in reftable_decode_key reftable/record.c:194 #3 0x55f23e54e72e in block_iter_next reftable/block.c:398 #4 0x55f23e5573dc in table_iter_next_in_block reftable/reader.c:240 #5 0x55f23e5573dc in table_iter_next reftable/reader.c:355 #6 0x55f23e5573dc in table_iter_next reftable/reader.c:339 #7 0x55f23e551283 in merged_iter_advance_subiter reftable/merged.c:69 #8 0x55f23e55169e in merged_iter_next_entry reftable/merged.c:123 #9 0x55f23e55169e in merged_iter_next_void reftable/merged.c:172 #10 0x55f23e537625 in reftable_iterator_next_ref reftable/generic.c:175 #11 0x55f23e2cf9c6 in reftable_ref_iterator_advance refs/reftable-backend.c:464 #12 0x55f23e2d996e in ref_iterator_advance refs/iterator.c:13 #13 0x55f23e2d996e in do_for_each_ref_iterator refs/iterator.c:452 #14 0x55f23dca6767 in get_ref_map builtin/fetch.c:623 #15 0x55f23dca6767 in do_fetch builtin/fetch.c:1659 #16 0x55f23dca6767 in fetch_one builtin/fetch.c:2133 #17 0x55f23dca6767 in cmd_fetch builtin/fetch.c:2432 #18 0x55f23dba7764 in run_builtin git.c:484 #19 0x55f23dba7764 in handle_builtin git.c:741 #20 0x55f23dbab61e in run_argv git.c:805 #21 0x55f23dbab61e in cmd_main git.c:1000 #22 0x55f23dba4781 in main common-main.c:64 #23 0x7fa0f063fc89 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 #24 0x7fa0f063fd44 in __libc_start_main_impl ../csu/libc-start.c:360 #25 0x55f23dba6ad0 in _start (git+0xadfad0) (BuildId: 803b2b7f59beb03d7849fb8294a8e2145dd4aa27) While it is somewhat awkward that the maintenance processes survive tests in the first place, it is totally expected that reftables should work alright with concurrent writers. Seemingly they don't. The only underlying resource that we need to care about in this context is the reftable reader, which is responsible for reading a single table from disk. These readers get discarded immediately (unless reused) when calling `reftable_stack_reload()`, which is wrong. We can only close them once we know that there are no iterators using them anymore. Prepare for a fix by converting the reftable readers to be refcounted. Reported-by: Jeff King <peff@peff.net> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Aug 23, 2024 at 16:12 UTC d857469d850b5e020de181ec07806872531d35e7

8 files changed +46 -25

reftable/reader.c

+14 -2

@@ -621,6 +621,7 @@ int reftable_reader_new(struct reftable_reader **out,

621 r->source = *source;

622 r->name = xstrdup(name);

623 r->hash_id = 0;

624 + r->refcount = 1;

625

626 err = block_source_read_block(source, &footer, r->size,

627 footer_size(r->version));

@@ -645,10 +646,21 @@ done:

646 return err;

647 }

648

648 -void reftable_reader_free(struct reftable_reader *r)

649 +void reftable_reader_incref(struct reftable_reader *r)

650 +{

651 + if (!r->refcount)

652 + BUG("cannot increment ref counter of dead reader");

653 + r->refcount++;

654 +}

655 +

656 +void reftable_reader_decref(struct reftable_reader *r)

657 {

658 if (!r)

659 return;

660 + if (!r->refcount)

661 + BUG("cannot decrement ref counter of dead reader");

662 + if (--r->refcount)

663 + return;

664 block_source_close(&r->source);

665 FREE_AND_NULL(r->name);

666 reftable_free(r);

@@ -812,7 +824,7 @@ int reftable_reader_print_blocks(const char *tablename)

824 }

825

826 done:

815 - reftable_reader_free(r);

827 + reftable_reader_decref(r);

828 table_iter_close(&ti);

829 return err;

830 }

reftable/reader.h

@@ -50,6 +50,8 @@ struct reftable_reader {

50 struct reftable_reader_offsets ref_offsets;

51 struct reftable_reader_offsets obj_offsets;

52 struct reftable_reader_offsets log_offsets;

53 +

54 + uint64_t refcount;

55 };

57 const char *reader_name(struct reftable_reader *r);

reftable/readwrite_test.c

+9 -9

@@ -279,7 +279,7 @@ static void test_log_write_read(void)

279 /* cleanup. */

280 strbuf_release(&buf);

281 free_names(names);

282 - reftable_reader_free(reader);

282 + reftable_reader_decref(reader);

283 }

284

285 static void test_log_zlib_corruption(void)

@@ -341,7 +341,7 @@ static void test_log_zlib_corruption(void)

341 reftable_iterator_destroy(&it);

342

343 /* cleanup. */

344 - reftable_reader_free(reader);

344 + reftable_reader_decref(reader);

345 strbuf_release(&buf);

346 }

347

@@ -383,7 +383,7 @@ static void test_table_read_write_sequential(void)

383 EXPECT(j == N);

384

385 reftable_iterator_destroy(&it);

386 - reftable_reader_free(reader);

386 + reftable_reader_decref(reader);

387 strbuf_release(&buf);

388 free_names(names);

389 }

@@ -431,7 +431,7 @@ static void test_table_read_api(void)

431 }

432 reftable_iterator_destroy(&it);

433 reftable_free(names);

434 - reftable_reader_free(reader);

434 + reftable_reader_decref(reader);

435 strbuf_release(&buf);

436 }

437

@@ -498,7 +498,7 @@ static void test_table_read_write_seek(int index, int hash_id)

498 reftable_free(names[i]);

499 }

500 reftable_free(names);

501 - reftable_reader_free(reader);

501 + reftable_reader_decref(reader);

502 }

503

504 static void test_table_read_write_seek_linear(void)

@@ -611,7 +611,7 @@ static void test_table_refs_for(int indexed)

611 strbuf_release(&buf);

612 free_names(want_names);

613 reftable_iterator_destroy(&it);

614 - reftable_reader_free(reader);

614 + reftable_reader_decref(reader);

615 }

616

617 static void test_table_refs_for_no_index(void)

@@ -657,7 +657,7 @@ static void test_write_empty_table(void)

657 EXPECT(err > 0);

658

659 reftable_iterator_destroy(&it);

660 - reftable_reader_free(rd);

660 + reftable_reader_decref(rd);

661 strbuf_release(&buf);

662 }

663

@@ -863,7 +863,7 @@ static void test_write_multiple_indices(void)

863

864 reftable_iterator_destroy(&it);

865 reftable_writer_free(writer);

866 - reftable_reader_free(reader);

866 + reftable_reader_decref(reader);

867 strbuf_release(&writer_buf);

868 strbuf_release(&buf);

869 }

@@ -919,7 +919,7 @@ static void test_write_multi_level_index(void)

919

920 reftable_iterator_destroy(&it);

921 reftable_writer_free(writer);

922 - reftable_reader_free(reader);

922 + reftable_reader_decref(reader);

923 strbuf_release(&writer_buf);

924 strbuf_release(&buf);

925 }

reftable/reftable-reader.h

+12 -3

@@ -33,6 +33,18 @@ struct reftable_reader;

33 int reftable_reader_new(struct reftable_reader **pp,

34 struct reftable_block_source *src, const char *name);

36 +/*

37 + * Manage the reference count of the reftable reader. A newly initialized

38 + * reader starts with a refcount of 1 and will be deleted once the refcount has

39 + * reached 0.

40 + *

41 + * This is required because readers may have longer lifetimes than the stack

42 + * they belong to. The stack may for example be reloaded while the old tables

43 + * are still being accessed by an iterator.

44 + */

45 +void reftable_reader_incref(struct reftable_reader *reader);

46 +void reftable_reader_decref(struct reftable_reader *reader);

47 +

48 /* Initialize a reftable iterator for reading refs. */

49 void reftable_reader_init_ref_iterator(struct reftable_reader *r,

50 struct reftable_iterator *it);

@@ -44,9 +56,6 @@ void reftable_reader_init_log_iterator(struct reftable_reader *r,

56 /* returns the hash ID used in this table. */

57 uint32_t reftable_reader_hash_id(struct reftable_reader *r);

47 -/* closes and deallocates a reader. */

48 -void reftable_reader_free(struct reftable_reader *);

49 -

59 /* return an iterator for the refs pointing to `oid`. */

60 int reftable_reader_refs_for(struct reftable_reader *r,

61 struct reftable_iterator *it, uint8_t *oid);

reftable/stack.c

+4 -4

@@ -186,7 +186,7 @@ void reftable_stack_destroy(struct reftable_stack *st)

186 if (names && !has_name(names, name)) {

187 stack_filename(&filename, st, name);

188 }

189 - reftable_reader_free(st->readers[i]);

189 + reftable_reader_decref(st->readers[i]);

190

191 if (filename.len) {

192 /* On Windows, can only unlink after closing. */

@@ -290,7 +290,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st,

290 const char *name = reader_name(cur[i]);

291 stack_filename(&table_path, st, name);

292

293 - reftable_reader_free(cur[i]);

293 + reftable_reader_decref(cur[i]);

294

295 /* On Windows, can only unlink after closing. */

296 unlink(table_path.buf);

@@ -299,7 +299,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st,

299

300 done:

301 for (i = 0; i < new_readers_len; i++)

302 - reftable_reader_free(new_readers[i]);

302 + reftable_reader_decref(new_readers[i]);

303 reftable_free(new_readers);

304 reftable_free(cur);

305 strbuf_release(&table_path);

@@ -1534,7 +1534,7 @@ static void remove_maybe_stale_table(struct reftable_stack *st, uint64_t max,

1534 goto done;

1535

1536 update_idx = reftable_reader_max_update_index(rd);

1537 - reftable_reader_free(rd);

1537 + reftable_reader_decref(rd);

1538

1539 if (update_idx <= max) {

1540 unlink(table_path.buf);

reftable/stack_test.c

+2 -4

@@ -1036,10 +1036,8 @@ static void test_reftable_stack_compaction_concurrent(void)

1036 static void unclean_stack_close(struct reftable_stack *st)

1037 {

1038 /* break abstraction boundary to simulate unclean shutdown. */

1039 - int i = 0;

1040 - for (; i < st->readers_len; i++) {

1041 - reftable_reader_free(st->readers[i]);

1042 - }

1039 + for (size_t i = 0; i < st->readers_len; i++)

1040 + reftable_reader_decref(st->readers[i]);

1041 st->readers_len = 0;

1042 FREE_AND_NULL(st->readers);

1043 }

t/helper/test-reftable.c

+1 -1

@@ -152,7 +152,7 @@ static int dump_reftable(const char *tablename)

152

153 done:

154 reftable_merged_table_free(mt);

155 - reftable_reader_free(r);

155 + reftable_reader_decref(r);

156 return err;

157 }

158

t/unit-tests/t-reftable-merged.c

+2 -2

@@ -115,7 +115,7 @@ merged_table_from_records(struct reftable_ref_record **refs,

115 static void readers_destroy(struct reftable_reader **readers, const size_t n)

116 {

117 for (size_t i = 0; i < n; i++)

118 - reftable_reader_free(readers[i]);

118 + reftable_reader_decref(readers[i]);

119 reftable_free(readers);

120 }

121

@@ -437,7 +437,7 @@ static void t_default_write_opts(void)

437 err = reftable_merged_table_new(&merged, &rd, 1, GIT_SHA1_FORMAT_ID);

438 check(!err);

439

440 - reftable_reader_free(rd);

440 + reftable_reader_decref(rd);

441 reftable_merged_table_free(merged);

442 strbuf_release(&buf);

443 }