{"thread":{"id":"61975","subject":"[PATCH 00/10] reftable: fix reload with active iterators","startedAt":"2024-08-19T15:39:43Z","lastAt":"2024-08-23T16:49:55Z","messageCount":38,"participants":["Patrick Steinhardt","karthik nayak","Jeff King","Junio C Hamano","Justin Tobler"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"501271","messageId":"cover.1724080006.git.ps@pks.im","threadId":"61975","inReplyTo":null,"subject":"[PATCH 00/10] reftable: fix reload with active iterators","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-19T15:39:38Z","receivedAt":"2024-08-19T15:39:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nas reported by Peff in [1], the reftable backend fails in t5616 in a\nracy manner. The cause of this is that git-maintenance(1) processes leak\ninto subsequent tests and perform concurrent compaction of the stack.\nThe reftable backend is expected to handle this gracefully, but doesn't.\n\nThe issue can surface whenever reloading the reftable stack while an\niterator is active. In case some of the tables got compacted, we will\nend up closing them and thus the iterator now tries to use those closed\ntables.\n\nThis patch series fixes that issue by starting to refcount the readers.\nEach iterator will bump the refcount, thus avoiding the problem. While\nat it, I also found a second issue where we segfault when reloading a\ntable fails while reusing one of the table readers. In this scenario, we\nwould end up releasing the reader of the stack itself, even though it\nwould still be used by it.\n\nThis patch series addresses those issues by making the reftable reader\nrefcounted. The first 6 patches do some simplifications of code which is\nclose. The remaining 4 ones introduce refcounting and wire it up as\nrequired.\n\nWith this, the following command now passes:\n\n    make -C .. -j20 SANITIZE=address && GIT_TEST_DEFAULT_REF_FORMAT=reftable ./t5616-partial-clone.sh --run=1-16 --stress\n\nThe patch series builds on top of Junio's ps/reftable-drop-generic at\n1a3c3870ee (reftable/generic: drop interface, 2024-08-14). It's only on\nseen right now, but would otherise cause a bunch of conflicts.\n\nThanks!\n\n[1]: <20240817121424.GA2439299@coredump.intra.peff.net>\n\nPatrick Steinhardt (10):\n  reftable/blocksource: drop malloc block source\n  reftable/stack: inline `stack_compact_range_stats()`\n  reftable/reader: rename `reftable_new_reader()`\n  reftable/reader: inline `init_reader()`\n  reftable/reader: inline `reader_close()`\n  reftable/stack: fix broken refnames in `write_n_ref_tables()`\n  reftable/reader: introduce refcounting\n  reftable/reader: keep readers alive during iteration\n  reftable/stack: reorder swapping in the reloaded stack contents\n  reftable/stack: fix segfault when reload with reused readers fails\n\n reftable/block_test.c            |   3 +-\n reftable/blocksource.c           |  20 -----\n reftable/blocksource.h           |   2 -\n reftable/reader.c                | 149 ++++++++++++++++---------------\n reftable/reader.h                |   5 +-\n reftable/readwrite_test.c        |  85 +++++++++---------\n reftable/reftable-reader.h       |  19 ++--\n reftable/stack.c                 |  90 +++++++++++--------\n reftable/stack_test.c            | 115 +++++++++++++++++++++++-\n t/helper/test-reftable.c         |   4 +-\n t/unit-tests/t-reftable-merged.c |  10 +--\n 11 files changed, 311 insertions(+), 191 deletions(-)\n\n\nbase-commit: 1a3c3870ee1a65b0579ccbac6b18e22b8c44c5b4\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501272","messageId":"fee3d3523eb867c6b45f4c133fb3b001be1ddd63.1724080006.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724080006.git.ps@pks.im","subject":"[PATCH 01/10] reftable/blocksource: drop malloc block source","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-19T15:39:41Z","receivedAt":"2024-08-19T15:39:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reftable blocksource provides a generic interface to read blocks via\ndifferent sources, e.g. from disk or from memory. One of the block\nsources is the malloc block source, which can in theory read data from\nmemory. We nowadays also have a strbuf block source though, which\nprovides essentially the same functionality with better ergonomics.\n\nAdapt the only remaining user of the malloc block source in our tests\nto use the strbuf block source, instead, and remove the now-unused\nmalloc block source.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/block_test.c  |  3 ++-\n reftable/blocksource.c | 20 --------------------\n reftable/blocksource.h |  2 --\n 3 files changed, 2 insertions(+), 23 deletions(-)\n\ndiff --git a/reftable/block_test.c b/reftable/block_test.c\nindex 90aecd5a7c..de8f426a42 100644\n--- a/reftable/block_test.c\n+++ b/reftable/block_test.c\n@@ -34,11 +34,12 @@ static void test_block_read_write(void)\n \tstruct block_reader br = { 0 };\n \tstruct block_iter it = BLOCK_ITER_INIT;\n \tint j = 0;\n+\tstruct strbuf block_data = STRBUF_INIT;\n \tstruct strbuf want = STRBUF_INIT;\n \n \tREFTABLE_CALLOC_ARRAY(block.data, block_size);\n \tblock.len = block_size;\n-\tblock.source = malloc_block_source();\n+\tblock_source_from_strbuf(&block.source, &block_data);\n \tblock_writer_init(&bw, BLOCK_TYPE_REF, block.data, block_size,\n \t\t\t  header_off, hash_size(GIT_SHA1_FORMAT_ID));\n \ndiff --git a/reftable/blocksource.c b/reftable/blocksource.c\nindex eeed254ba9..1774853011 100644\n--- a/reftable/blocksource.c\n+++ b/reftable/blocksource.c\n@@ -55,26 +55,6 @@ void block_source_from_strbuf(struct reftable_block_source *bs,\n \tbs->arg = buf;\n }\n \n-static void malloc_return_block(void *b, struct reftable_block *dest)\n-{\n-\tif (dest->len)\n-\t\tmemset(dest->data, 0xff, dest->len);\n-\treftable_free(dest->data);\n-}\n-\n-static struct reftable_block_source_vtable malloc_vtable = {\n-\t.return_block = &malloc_return_block,\n-};\n-\n-static struct reftable_block_source malloc_block_source_instance = {\n-\t.ops = &malloc_vtable,\n-};\n-\n-struct reftable_block_source malloc_block_source(void)\n-{\n-\treturn malloc_block_source_instance;\n-}\n-\n struct file_block_source {\n \tuint64_t size;\n \tunsigned char *data;\ndiff --git a/reftable/blocksource.h b/reftable/blocksource.h\nindex 072e2727ad..659a27b406 100644\n--- a/reftable/blocksource.h\n+++ b/reftable/blocksource.h\n@@ -17,6 +17,4 @@ struct reftable_block_source;\n void block_source_from_strbuf(struct reftable_block_source *bs,\n \t\t\t      struct strbuf *buf);\n \n-struct reftable_block_source malloc_block_source(void);\n-\n #endif\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501273","messageId":"3c0cf2bf46f4106d7c0b30c863946262639b2351.1724080006.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724080006.git.ps@pks.im","subject":"[PATCH 02/10] reftable/stack: inline `stack_compact_range_stats()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-19T15:39:44Z","receivedAt":"2024-08-19T15:39:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The only difference between `stack_compact_range_stats()` and\n`stack_compact_range()` is that the former updates stats on failure,\nwhereas the latter doesn't. There are no callers anymore that do not\nwant their stats updated though, making the indirection unnecessary.\n\nInline the stat updates into `stack_compact_range()`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 16 ++++------------\n 1 file changed, 4 insertions(+), 12 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex d3a95d2f1d..891ea971b7 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -1328,17 +1328,9 @@ static int stack_compact_range(struct reftable_stack *st,\n \tstrbuf_release(&table_name);\n \tfree_names(names);\n \n-\treturn err;\n-}\n-\n-static int stack_compact_range_stats(struct reftable_stack *st,\n-\t\t\t\t     size_t first, size_t last,\n-\t\t\t\t     struct reftable_log_expiry_config *config,\n-\t\t\t\t     unsigned int flags)\n-{\n-\tint err = stack_compact_range(st, first, last, config, flags);\n \tif (err == REFTABLE_LOCK_ERROR)\n \t\tst->stats.failures++;\n+\n \treturn err;\n }\n \n@@ -1346,7 +1338,7 @@ int reftable_stack_compact_all(struct reftable_stack *st,\n \t\t\t       struct reftable_log_expiry_config *config)\n {\n \tsize_t last = st->merged->readers_len ? st->merged->readers_len - 1 : 0;\n-\treturn stack_compact_range_stats(st, 0, last, config, 0);\n+\treturn stack_compact_range(st, 0, last, config, 0);\n }\n \n static int segment_size(struct segment *s)\n@@ -1452,8 +1444,8 @@ int reftable_stack_auto_compact(struct reftable_stack *st)\n \t\t\t\t\t   st->opts.auto_compaction_factor);\n \treftable_free(sizes);\n \tif (segment_size(&seg) > 0)\n-\t\treturn stack_compact_range_stats(st, seg.start, seg.end - 1,\n-\t\t\t\t\t\t NULL, STACK_COMPACT_RANGE_BEST_EFFORT);\n+\t\treturn stack_compact_range(st, seg.start, seg.end - 1,\n+\t\t\t\t\t   NULL, STACK_COMPACT_RANGE_BEST_EFFORT);\n \n \treturn 0;\n }\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501274","messageId":"e658b372f046616779cf49c6c9346a49c2ce1485.1724080006.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724080006.git.ps@pks.im","subject":"[PATCH 03/10] reftable/reader: rename `reftable_new_reader()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-19T15:39:47Z","receivedAt":"2024-08-19T15:39:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Rename the `reftable_new_reader()` function to `reftable_reader_new()`\nto match our coding guidelines.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/reader.c                | 4 ++--\n reftable/reader.h                | 2 +-\n reftable/readwrite_test.c        | 6 +++---\n reftable/reftable-reader.h       | 4 ++--\n reftable/stack.c                 | 4 ++--\n t/helper/test-reftable.c         | 2 +-\n t/unit-tests/t-reftable-merged.c | 6 +++---\n 7 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 082cf00b60..ea4fdea90b 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -637,7 +637,7 @@ void reader_close(struct reftable_reader *r)\n \tFREE_AND_NULL(r->name);\n }\n \n-int reftable_new_reader(struct reftable_reader **p,\n+int reftable_reader_new(struct reftable_reader **p,\n \t\t\tstruct reftable_block_source *src, char const *name)\n {\n \tstruct reftable_reader *rd = reftable_calloc(1, sizeof(*rd));\n@@ -786,7 +786,7 @@ int reftable_reader_print_blocks(const char *tablename)\n \tif (err < 0)\n \t\tgoto done;\n \n-\terr = reftable_new_reader(&r, &src, tablename);\n+\terr = reftable_reader_new(&r, &src, tablename);\n \tif (err < 0)\n \t\tgoto done;\n \ndiff --git a/reftable/reader.h b/reftable/reader.h\nindex a2c204d523..d8cbfd6404 100644\n--- a/reftable/reader.h\n+++ b/reftable/reader.h\n@@ -52,7 +52,7 @@ struct reftable_reader {\n \tstruct reftable_reader_offsets log_offsets;\n };\n \n-int init_reader(struct reftable_reader *r, struct reftable_block_source *source,\n+int reader_init(struct reftable_reader *r, struct reftable_block_source *source,\n \t\tconst char *name);\n void reader_close(struct reftable_reader *r);\n const char *reader_name(struct reftable_reader *r);\ndiff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c\nindex f411abfe9c..101cdb5cd6 100644\n--- a/reftable/readwrite_test.c\n+++ b/reftable/readwrite_test.c\n@@ -646,7 +646,7 @@ static void test_write_empty_table(void)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = reftable_new_reader(&rd, &source, \"filename\");\n+\terr = reftable_reader_new(&rd, &source, \"filename\");\n \tEXPECT_ERR(err);\n \n \treftable_reader_init_ref_iterator(rd, &it);\n@@ -850,7 +850,7 @@ static void test_write_multiple_indices(void)\n \tEXPECT(stats->log_stats.index_offset > 0);\n \n \tblock_source_from_strbuf(&source, &writer_buf);\n-\terr = reftable_new_reader(&reader, &source, \"filename\");\n+\terr = reftable_reader_new(&reader, &source, \"filename\");\n \tEXPECT_ERR(err);\n \n \t/*\n@@ -907,7 +907,7 @@ static void test_write_multi_level_index(void)\n \tEXPECT(stats->ref_stats.max_index_level == 2);\n \n \tblock_source_from_strbuf(&source, &writer_buf);\n-\terr = reftable_new_reader(&reader, &source, \"filename\");\n+\terr = reftable_reader_new(&reader, &source, \"filename\");\n \tEXPECT_ERR(err);\n \n \t/*\ndiff --git a/reftable/reftable-reader.h b/reftable/reftable-reader.h\nindex 69621c5b0f..8a05c84685 100644\n--- a/reftable/reftable-reader.h\n+++ b/reftable/reftable-reader.h\n@@ -23,14 +23,14 @@\n /* The reader struct is a handle to an open reftable file. */\n struct reftable_reader;\n \n-/* reftable_new_reader opens a reftable for reading. If successful,\n+/* reftable_reader_new opens a reftable for reading. If successful,\n  * returns 0 code and sets pp. The name is used for creating a\n  * stack. Typically, it is the basename of the file. The block source\n  * `src` is owned by the reader, and is closed on calling\n  * reftable_reader_destroy(). On error, the block source `src` is\n  * closed as well.\n  */\n-int reftable_new_reader(struct reftable_reader **pp,\n+int reftable_reader_new(struct reftable_reader **pp,\n \t\t\tstruct reftable_block_source *src, const char *name);\n \n /* Initialize a reftable iterator for reading refs. */\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 891ea971b7..c72435b059 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -258,7 +258,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t\t\tif (err < 0)\n \t\t\t\tgoto done;\n \n-\t\t\terr = reftable_new_reader(&rd, &src, name);\n+\t\t\terr = reftable_reader_new(&rd, &src, name);\n \t\t\tif (err < 0)\n \t\t\t\tgoto done;\n \t\t}\n@@ -1532,7 +1532,7 @@ static void remove_maybe_stale_table(struct reftable_stack *st, uint64_t max,\n \tif (err < 0)\n \t\tgoto done;\n \n-\terr = reftable_new_reader(&rd, &src, name);\n+\terr = reftable_reader_new(&rd, &src, name);\n \tif (err < 0)\n \t\tgoto done;\n \ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex c1942156b5..87c2f42aae 100644\n--- a/t/helper/test-reftable.c\n+++ b/t/helper/test-reftable.c\n@@ -139,7 +139,7 @@ static int dump_reftable(const char *tablename)\n \tif (err < 0)\n \t\tgoto done;\n \n-\terr = reftable_new_reader(&r, &src, tablename);\n+\terr = reftable_reader_new(&r, &src, tablename);\n \tif (err < 0)\n \t\tgoto done;\n \ndiff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c\nindex 93345c6c8b..8f51aca1b6 100644\n--- a/t/unit-tests/t-reftable-merged.c\n+++ b/t/unit-tests/t-reftable-merged.c\n@@ -102,7 +102,7 @@ merged_table_from_records(struct reftable_ref_record **refs,\n \t\twrite_test_table(&buf[i], refs[i], sizes[i]);\n \t\tblock_source_from_strbuf(&(*source)[i], &buf[i]);\n \n-\t\terr = reftable_new_reader(&(*readers)[i], &(*source)[i],\n+\t\terr = reftable_reader_new(&(*readers)[i], &(*source)[i],\n \t\t\t\t\t  \"name\");\n \t\tcheck(!err);\n \t}\n@@ -277,7 +277,7 @@ merged_table_from_log_records(struct reftable_log_record **logs,\n \t\twrite_test_log_table(&buf[i], logs[i], sizes[i], i + 1);\n \t\tblock_source_from_strbuf(&(*source)[i], &buf[i]);\n \n-\t\terr = reftable_new_reader(&(*readers)[i], &(*source)[i],\n+\t\terr = reftable_reader_new(&(*readers)[i], &(*source)[i],\n \t\t\t\t\t  \"name\");\n \t\tcheck(!err);\n \t}\n@@ -426,7 +426,7 @@ static void t_default_write_opts(void)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = reftable_new_reader(&rd, &source, \"filename\");\n+\terr = reftable_reader_new(&rd, &source, \"filename\");\n \tcheck(!err);\n \n \thash_id = reftable_reader_hash_id(rd);\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501275","messageId":"f628b7dafb9f4b253ff27c116b5f7f29794a3520.1724080006.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724080006.git.ps@pks.im","subject":"[PATCH 04/10] reftable/reader: inline `init_reader()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-19T15:39:54Z","receivedAt":"2024-08-19T15:39:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Most users use an allocated version of the `reftable_reader`, except for\nsome tests. We are about to convert the reader to become refcounted\nthough, and providing the ability to keep a reader on the stack makes\nthis conversion harder than necessary.\n\nUpdate the tests to use `reftable_reader_new()` instead to prepare for\nthis change.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/reader.c         | 122 +++++++++++++++++++-------------------\n reftable/reader.h         |   2 -\n reftable/readwrite_test.c |  73 ++++++++++++-----------\n 3 files changed, 98 insertions(+), 99 deletions(-)\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex ea4fdea90b..9239679ad9 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -162,58 +162,6 @@ static int parse_footer(struct reftable_reader *r, uint8_t *footer,\n \treturn err;\n }\n \n-int init_reader(struct reftable_reader *r, struct reftable_block_source *source,\n-\t\tconst char *name)\n-{\n-\tstruct reftable_block footer = { NULL };\n-\tstruct reftable_block header = { NULL };\n-\tint err = 0;\n-\tuint64_t file_size = block_source_size(source);\n-\n-\t/* Need +1 to read type of first block. */\n-\tuint32_t read_size = header_size(2) + 1; /* read v2 because it's larger.  */\n-\tmemset(r, 0, sizeof(struct reftable_reader));\n-\n-\tif (read_size > file_size) {\n-\t\terr = REFTABLE_FORMAT_ERROR;\n-\t\tgoto done;\n-\t}\n-\n-\terr = block_source_read_block(source, &header, 0, read_size);\n-\tif (err != read_size) {\n-\t\terr = REFTABLE_IO_ERROR;\n-\t\tgoto done;\n-\t}\n-\n-\tif (memcmp(header.data, \"REFT\", 4)) {\n-\t\terr = REFTABLE_FORMAT_ERROR;\n-\t\tgoto done;\n-\t}\n-\tr->version = header.data[4];\n-\tif (r->version != 1 && r->version != 2) {\n-\t\terr = REFTABLE_FORMAT_ERROR;\n-\t\tgoto done;\n-\t}\n-\n-\tr->size = file_size - footer_size(r->version);\n-\tr->source = *source;\n-\tr->name = xstrdup(name);\n-\tr->hash_id = 0;\n-\n-\terr = block_source_read_block(source, &footer, r->size,\n-\t\t\t\t      footer_size(r->version));\n-\tif (err != footer_size(r->version)) {\n-\t\terr = REFTABLE_IO_ERROR;\n-\t\tgoto done;\n-\t}\n-\n-\terr = parse_footer(r, footer.data, header.data);\n-done:\n-\treftable_block_done(&footer);\n-\treftable_block_done(&header);\n-\treturn err;\n-}\n-\n struct table_iter {\n \tstruct reftable_reader *r;\n \tuint8_t typ;\n@@ -637,16 +585,68 @@ void reader_close(struct reftable_reader *r)\n \tFREE_AND_NULL(r->name);\n }\n \n-int reftable_reader_new(struct reftable_reader **p,\n-\t\t\tstruct reftable_block_source *src, char const *name)\n+int reftable_reader_new(struct reftable_reader **out,\n+\t\t\tstruct reftable_block_source *source, char const *name)\n {\n-\tstruct reftable_reader *rd = reftable_calloc(1, sizeof(*rd));\n-\tint err = init_reader(rd, src, name);\n-\tif (err == 0) {\n-\t\t*p = rd;\n-\t} else {\n-\t\tblock_source_close(src);\n-\t\treftable_free(rd);\n+\tstruct reftable_block footer = { 0 };\n+\tstruct reftable_block header = { 0 };\n+\tstruct reftable_reader *r;\n+\tuint64_t file_size = block_source_size(source);\n+\tuint32_t read_size;\n+\tint err;\n+\n+\tREFTABLE_CALLOC_ARRAY(r, 1);\n+\n+\t/*\n+\t * We need one extra byte to read the type of first block. We also\n+\t * pretend to always be reading v2 of the format because it is larger.\n+\t */\n+\tread_size = header_size(2) + 1;\n+\tif (read_size > file_size) {\n+\t\terr = REFTABLE_FORMAT_ERROR;\n+\t\tgoto done;\n+\t}\n+\n+\terr = block_source_read_block(source, &header, 0, read_size);\n+\tif (err != read_size) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n+\t}\n+\n+\tif (memcmp(header.data, \"REFT\", 4)) {\n+\t\terr = REFTABLE_FORMAT_ERROR;\n+\t\tgoto done;\n+\t}\n+\tr->version = header.data[4];\n+\tif (r->version != 1 && r->version != 2) {\n+\t\terr = REFTABLE_FORMAT_ERROR;\n+\t\tgoto done;\n+\t}\n+\n+\tr->size = file_size - footer_size(r->version);\n+\tr->source = *source;\n+\tr->name = xstrdup(name);\n+\tr->hash_id = 0;\n+\n+\terr = block_source_read_block(source, &footer, r->size,\n+\t\t\t\t      footer_size(r->version));\n+\tif (err != footer_size(r->version)) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n+\t}\n+\n+\terr = parse_footer(r, footer.data, header.data);\n+\tif (err)\n+\t\tgoto done;\n+\n+\t*out = r;\n+\n+done:\n+\treftable_block_done(&footer);\n+\treftable_block_done(&header);\n+\tif (err) {\n+\t\treftable_free(r);\n+\t\tblock_source_close(source);\n \t}\n \treturn err;\n }\ndiff --git a/reftable/reader.h b/reftable/reader.h\nindex d8cbfd6404..762cd6de66 100644\n--- a/reftable/reader.h\n+++ b/reftable/reader.h\n@@ -52,8 +52,6 @@ struct reftable_reader {\n \tstruct reftable_reader_offsets log_offsets;\n };\n \n-int reader_init(struct reftable_reader *r, struct reftable_block_source *source,\n-\t\tconst char *name);\n void reader_close(struct reftable_reader *r);\n const char *reader_name(struct reftable_reader *r);\n \ndiff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c\nindex 101cdb5cd6..2f2ff787b2 100644\n--- a/reftable/readwrite_test.c\n+++ b/reftable/readwrite_test.c\n@@ -195,7 +195,7 @@ static void test_log_write_read(void)\n \tstruct reftable_log_record log = { NULL };\n \tint n;\n \tstruct reftable_iterator it = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { NULL };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n@@ -236,10 +236,10 @@ static void test_log_write_read(void)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = init_reader(&rd, &source, \"file.log\");\n+\terr = reftable_reader_new(&reader, &source, \"file.log\");\n \tEXPECT_ERR(err);\n \n-\treftable_reader_init_ref_iterator(&rd, &it);\n+\treftable_reader_init_ref_iterator(reader, &it);\n \n \terr = reftable_iterator_seek_ref(&it, names[N - 1]);\n \tEXPECT_ERR(err);\n@@ -254,7 +254,7 @@ static void test_log_write_read(void)\n \treftable_iterator_destroy(&it);\n \treftable_ref_record_release(&ref);\n \n-\treftable_reader_init_log_iterator(&rd, &it);\n+\treftable_reader_init_log_iterator(reader, &it);\n \n \terr = reftable_iterator_seek_log(&it, \"\");\n \tEXPECT_ERR(err);\n@@ -279,7 +279,7 @@ static void test_log_write_read(void)\n \t/* cleanup. */\n \tstrbuf_release(&buf);\n \tfree_names(names);\n-\treader_close(&rd);\n+\treftable_reader_free(reader);\n }\n \n static void test_log_zlib_corruption(void)\n@@ -288,7 +288,7 @@ static void test_log_zlib_corruption(void)\n \t\t.block_size = 256,\n \t};\n \tstruct reftable_iterator it = { 0 };\n-\tstruct reftable_reader rd = { 0 };\n+\tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n@@ -331,18 +331,18 @@ static void test_log_zlib_corruption(void)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = init_reader(&rd, &source, \"file.log\");\n+\terr = reftable_reader_new(&reader, &source, \"file.log\");\n \tEXPECT_ERR(err);\n \n-\treftable_reader_init_log_iterator(&rd, &it);\n+\treftable_reader_init_log_iterator(reader, &it);\n \terr = reftable_iterator_seek_log(&it, \"refname\");\n \tEXPECT(err == REFTABLE_ZLIB_ERROR);\n \n \treftable_iterator_destroy(&it);\n \n \t/* cleanup. */\n+\treftable_reader_free(reader);\n \tstrbuf_release(&buf);\n-\treader_close(&rd);\n }\n \n static void test_table_read_write_sequential(void)\n@@ -352,7 +352,7 @@ static void test_table_read_write_sequential(void)\n \tint N = 50;\n \tstruct reftable_iterator it = { NULL };\n \tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_reader *reader;\n \tint err = 0;\n \tint j = 0;\n \n@@ -360,10 +360,10 @@ static void test_table_read_write_sequential(void)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = init_reader(&rd, &source, \"file.ref\");\n+\terr = reftable_reader_new(&reader, &source, \"file.ref\");\n \tEXPECT_ERR(err);\n \n-\treftable_reader_init_ref_iterator(&rd, &it);\n+\treftable_reader_init_ref_iterator(reader, &it);\n \terr = reftable_iterator_seek_ref(&it, \"\");\n \tEXPECT_ERR(err);\n \n@@ -381,11 +381,11 @@ static void test_table_read_write_sequential(void)\n \t\treftable_ref_record_release(&ref);\n \t}\n \tEXPECT(j == N);\n+\n \treftable_iterator_destroy(&it);\n+\treftable_reader_free(reader);\n \tstrbuf_release(&buf);\n \tfree_names(names);\n-\n-\treader_close(&rd);\n }\n \n static void test_table_write_small_table(void)\n@@ -404,7 +404,7 @@ static void test_table_read_api(void)\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 50;\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { NULL };\n \tint err;\n \tint i;\n@@ -415,10 +415,10 @@ static void test_table_read_api(void)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = init_reader(&rd, &source, \"file.ref\");\n+\terr = reftable_reader_new(&reader, &source, \"file.ref\");\n \tEXPECT_ERR(err);\n \n-\treftable_reader_init_ref_iterator(&rd, &it);\n+\treftable_reader_init_ref_iterator(reader, &it);\n \terr = reftable_iterator_seek_ref(&it, names[0]);\n \tEXPECT_ERR(err);\n \n@@ -431,7 +431,7 @@ static void test_table_read_api(void)\n \t}\n \treftable_iterator_destroy(&it);\n \treftable_free(names);\n-\treader_close(&rd);\n+\treftable_reader_free(reader);\n \tstrbuf_release(&buf);\n }\n \n@@ -440,7 +440,7 @@ static void test_table_read_write_seek(int index, int hash_id)\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 50;\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { NULL };\n \tint err;\n \tint i = 0;\n@@ -453,18 +453,18 @@ static void test_table_read_write_seek(int index, int hash_id)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = init_reader(&rd, &source, \"file.ref\");\n+\terr = reftable_reader_new(&reader, &source, \"file.ref\");\n \tEXPECT_ERR(err);\n-\tEXPECT(hash_id == reftable_reader_hash_id(&rd));\n+\tEXPECT(hash_id == reftable_reader_hash_id(reader));\n \n \tif (!index) {\n-\t\trd.ref_offsets.index_offset = 0;\n+\t\treader->ref_offsets.index_offset = 0;\n \t} else {\n-\t\tEXPECT(rd.ref_offsets.index_offset > 0);\n+\t\tEXPECT(reader->ref_offsets.index_offset > 0);\n \t}\n \n \tfor (i = 1; i < N; i++) {\n-\t\treftable_reader_init_ref_iterator(&rd, &it);\n+\t\treftable_reader_init_ref_iterator(reader, &it);\n \t\terr = reftable_iterator_seek_ref(&it, names[i]);\n \t\tEXPECT_ERR(err);\n \t\terr = reftable_iterator_next_ref(&it, &ref);\n@@ -480,7 +480,7 @@ static void test_table_read_write_seek(int index, int hash_id)\n \tstrbuf_addstr(&pastLast, names[N - 1]);\n \tstrbuf_addstr(&pastLast, \"/\");\n \n-\treftable_reader_init_ref_iterator(&rd, &it);\n+\treftable_reader_init_ref_iterator(reader, &it);\n \terr = reftable_iterator_seek_ref(&it, pastLast.buf);\n \tif (err == 0) {\n \t\tstruct reftable_ref_record ref = { NULL };\n@@ -498,7 +498,7 @@ static void test_table_read_write_seek(int index, int hash_id)\n \t\treftable_free(names[i]);\n \t}\n \treftable_free(names);\n-\treader_close(&rd);\n+\treftable_reader_free(reader);\n }\n \n static void test_table_read_write_seek_linear(void)\n@@ -530,7 +530,7 @@ static void test_table_refs_for(int indexed)\n \tint i = 0;\n \tint n;\n \tint err;\n-\tstruct reftable_reader rd;\n+\tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { NULL };\n \n \tstruct strbuf buf = STRBUF_INIT;\n@@ -579,18 +579,18 @@ static void test_table_refs_for(int indexed)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = init_reader(&rd, &source, \"file.ref\");\n+\terr = reftable_reader_new(&reader, &source, \"file.ref\");\n \tEXPECT_ERR(err);\n \tif (!indexed) {\n-\t\trd.obj_offsets.is_present = 0;\n+\t\treader->obj_offsets.is_present = 0;\n \t}\n \n-\treftable_reader_init_ref_iterator(&rd, &it);\n+\treftable_reader_init_ref_iterator(reader, &it);\n \terr = reftable_iterator_seek_ref(&it, \"\");\n \tEXPECT_ERR(err);\n \treftable_iterator_destroy(&it);\n \n-\terr = reftable_reader_refs_for(&rd, &it, want_hash);\n+\terr = reftable_reader_refs_for(reader, &it, want_hash);\n \tEXPECT_ERR(err);\n \n \tj = 0;\n@@ -611,7 +611,7 @@ static void test_table_refs_for(int indexed)\n \tstrbuf_release(&buf);\n \tfree_names(want_names);\n \treftable_iterator_destroy(&it);\n-\treader_close(&rd);\n+\treftable_reader_free(reader);\n }\n \n static void test_table_refs_for_no_index(void)\n@@ -928,11 +928,11 @@ static void test_corrupt_table_empty(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_reader *reader;\n \tint err;\n \n \tblock_source_from_strbuf(&source, &buf);\n-\terr = init_reader(&rd, &source, \"file.log\");\n+\terr = reftable_reader_new(&reader, &source, \"file.log\");\n \tEXPECT(err == REFTABLE_FORMAT_ERROR);\n }\n \n@@ -941,13 +941,14 @@ static void test_corrupt_table(void)\n \tuint8_t zeros[1024] = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_reader *reader;\n \tint err;\n \tstrbuf_add(&buf, zeros, sizeof(zeros));\n \n \tblock_source_from_strbuf(&source, &buf);\n-\terr = init_reader(&rd, &source, \"file.log\");\n+\terr = reftable_reader_new(&reader, &source, \"file.log\");\n \tEXPECT(err == REFTABLE_FORMAT_ERROR);\n+\n \tstrbuf_release(&buf);\n }\n \n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501276","messageId":"4a9fe15042795f5b2bd4d4e17cf771d86e4eea47.1724080006.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724080006.git.ps@pks.im","subject":"[PATCH 05/10] reftable/reader: inline `reader_close()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-19T15:39:57Z","receivedAt":"2024-08-19T15:40:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Same as with the preceding commit, we also provide a `reader_close()`\nfunction that allows the caller to close a reader without freeing it.\nThis is unnecessary now that all users will have an allocated version of\nthe reader.\n\nInline it into `reftable_reader_free()`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/reader.c | 9 ++-------\n reftable/reader.h | 1 -\n reftable/stack.c  | 5 +----\n 3 files changed, 3 insertions(+), 12 deletions(-)\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 9239679ad9..037417fcf6 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -579,12 +579,6 @@ void reftable_reader_init_log_iterator(struct reftable_reader *r,\n \treader_init_iter(r, it, BLOCK_TYPE_LOG);\n }\n \n-void reader_close(struct reftable_reader *r)\n-{\n-\tblock_source_close(&r->source);\n-\tFREE_AND_NULL(r->name);\n-}\n-\n int reftable_reader_new(struct reftable_reader **out,\n \t\t\tstruct reftable_block_source *source, char const *name)\n {\n@@ -655,7 +649,8 @@ void reftable_reader_free(struct reftable_reader *r)\n {\n \tif (!r)\n \t\treturn;\n-\treader_close(r);\n+\tblock_source_close(&r->source);\n+\tFREE_AND_NULL(r->name);\n \treftable_free(r);\n }\n \ndiff --git a/reftable/reader.h b/reftable/reader.h\nindex 762cd6de66..88b4f3b421 100644\n--- a/reftable/reader.h\n+++ b/reftable/reader.h\n@@ -52,7 +52,6 @@ struct reftable_reader {\n \tstruct reftable_reader_offsets log_offsets;\n };\n \n-void reader_close(struct reftable_reader *r);\n const char *reader_name(struct reftable_reader *r);\n \n void reader_init_iter(struct reftable_reader *r,\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex c72435b059..0ac9cdf8de 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -290,7 +290,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t\t\tconst char *name = reader_name(cur[i]);\n \t\t\tstack_filename(&table_path, st, name);\n \n-\t\t\treader_close(cur[i]);\n \t\t\treftable_reader_free(cur[i]);\n \n \t\t\t/* On Windows, can only unlink after closing. */\n@@ -299,10 +298,8 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t}\n \n done:\n-\tfor (i = 0; i < new_readers_len; i++) {\n-\t\treader_close(new_readers[i]);\n+\tfor (i = 0; i < new_readers_len; i++)\n \t\treftable_reader_free(new_readers[i]);\n-\t}\n \treftable_free(new_readers);\n \treftable_free(cur);\n \tstrbuf_release(&table_path);\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501277","messageId":"4965402e7bf5a09da551a8cdc8c12dd87515e2ba.1724080006.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724080006.git.ps@pks.im","subject":"[PATCH 06/10] reftable/stack: fix broken refnames in `write_n_ref_tables()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-19T15:40:00Z","receivedAt":"2024-08-19T15:40:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `write_n_ref_tables()` helper function writes N references in\nseparate tables. We never reset the computed name of those references\nthough, leading us to end up with unexpected names.\n\nFix this by resetting the buffer.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack_test.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 42044ed8a3..de0669b7b8 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -125,6 +125,7 @@ static void write_n_ref_tables(struct reftable_stack *st,\n \t\t\t.value_type = REFTABLE_REF_VAL1,\n \t\t};\n \n+\t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"refs/heads/branch-%04u\", (unsigned) i);\n \t\tref.refname = buf.buf;\n \t\tset_test_hash(ref.value.val1, i);\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501278","messageId":"fc0ed68d4675bcd4c89bf63419ec6e8b6b7f5fca.1724080006.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724080006.git.ps@pks.im","subject":"[PATCH 07/10] reftable/reader: introduce refcounting","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-19T15:40:03Z","receivedAt":"2024-08-19T15:40:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It was recently reported that concurrent reads and writes may cause the\nreftable backend to segfault. The root cause of this is that we do not\nproperly keep track of reftable readers across reloads.\n\nSuppose that you have a reftable iterator and then decide to reload the\nstack while iterating through the iterator. When the stack has been\nrewritten since we have created the iterator, then we would end up\ndiscarding a subset of readers that may still be in use by the iterator.\nThe consequence is that we now try to reference deallocated memory,\nwhich of course segfaults.\n\nOne way to trigger this is in t5616, where some background maintenance\njobs have been leaking from one test into another. This leads to stack\ntraces like the following one:\n\n  + git -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 --refetch origin\n  AddressSanitizer:DEADLYSIGNAL\n  =================================================================\n  ==657994==ERROR: AddressSanitizer: SEGV on unknown address 0x7fa0f0ec6089 (pc 0x55f23e52ddf9 bp\n0x7ffe7bfa1700 sp 0x7ffe7bfa1700 T0)\n  ==657994==The signal is caused by a READ memory access.\n      #0 0x55f23e52ddf9 in get_var_int reftable/record.c:29\n      #1 0x55f23e53295e in reftable_decode_keylen reftable/record.c:170\n      #2 0x55f23e532cc0 in reftable_decode_key reftable/record.c:194\n      #3 0x55f23e54e72e in block_iter_next reftable/block.c:398\n      #4 0x55f23e5573dc in table_iter_next_in_block reftable/reader.c:240\n      #5 0x55f23e5573dc in table_iter_next reftable/reader.c:355\n      #6 0x55f23e5573dc in table_iter_next reftable/reader.c:339\n      #7 0x55f23e551283 in merged_iter_advance_subiter reftable/merged.c:69\n      #8 0x55f23e55169e in merged_iter_next_entry reftable/merged.c:123\n      #9 0x55f23e55169e in merged_iter_next_void reftable/merged.c:172\n      #10 0x55f23e537625 in reftable_iterator_next_ref reftable/generic.c:175\n      #11 0x55f23e2cf9c6 in reftable_ref_iterator_advance refs/reftable-backend.c:464\n      #12 0x55f23e2d996e in ref_iterator_advance refs/iterator.c:13\n      #13 0x55f23e2d996e in do_for_each_ref_iterator refs/iterator.c:452\n      #14 0x55f23dca6767 in get_ref_map builtin/fetch.c:623\n      #15 0x55f23dca6767 in do_fetch builtin/fetch.c:1659\n      #16 0x55f23dca6767 in fetch_one builtin/fetch.c:2133\n      #17 0x55f23dca6767 in cmd_fetch builtin/fetch.c:2432\n      #18 0x55f23dba7764 in run_builtin git.c:484\n      #19 0x55f23dba7764 in handle_builtin git.c:741\n      #20 0x55f23dbab61e in run_argv git.c:805\n      #21 0x55f23dbab61e in cmd_main git.c:1000\n      #22 0x55f23dba4781 in main common-main.c:64\n      #23 0x7fa0f063fc89 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n      #24 0x7fa0f063fd44 in __libc_start_main_impl ../csu/libc-start.c:360\n      #25 0x55f23dba6ad0 in _start (git+0xadfad0) (BuildId: 803b2b7f59beb03d7849fb8294a8e2145dd4aa27)\n\nWhile it is somewhat awkward that the maintenance processes survive\ntests in the first place, it is totally expected that reftables should\nwork alright with concurrent writers. Seemingly they don't.\n\nThe only underlying resource that we need to care about in this context\nis the reftable reader, which is responsible for reading a single table\nfrom disk. These readers get discarded immediately (unless reused) when\ncalling `reftable_stack_reload()`, which is wrong. We can only close\nthem once we know that there are no iterators using them anymore.\n\nPrepare for a fix by converting the reftable readers to be refcounted.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/reader.c                | 16 ++++++++++++++--\n reftable/reader.h                |  2 ++\n reftable/readwrite_test.c        | 18 +++++++++---------\n reftable/reftable-reader.h       | 15 ++++++++++++---\n reftable/stack.c                 |  8 ++++----\n reftable/stack_test.c            |  6 ++----\n t/helper/test-reftable.c         |  2 +-\n t/unit-tests/t-reftable-merged.c |  4 ++--\n 8 files changed, 46 insertions(+), 25 deletions(-)\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 037417fcf6..64a0953e68 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -621,6 +621,7 @@ int reftable_reader_new(struct reftable_reader **out,\n \tr->source = *source;\n \tr->name = xstrdup(name);\n \tr->hash_id = 0;\n+\tr->refcount = 1;\n \n \terr = block_source_read_block(source, &footer, r->size,\n \t\t\t\t      footer_size(r->version));\n@@ -645,10 +646,21 @@ int reftable_reader_new(struct reftable_reader **out,\n \treturn err;\n }\n \n-void reftable_reader_free(struct reftable_reader *r)\n+void reftable_reader_incref(struct reftable_reader *r)\n+{\n+\tif (!r->refcount)\n+\t\tBUG(\"cannot increment ref counter of dead reader\");\n+\tr->refcount++;\n+}\n+\n+void reftable_reader_decref(struct reftable_reader *r)\n {\n \tif (!r)\n \t\treturn;\n+\tif (!r->refcount)\n+\t\tBUG(\"cannot decrement ref counter of dead reader\");\n+\tif (--r->refcount)\n+\t\treturn;\n \tblock_source_close(&r->source);\n \tFREE_AND_NULL(r->name);\n \treftable_free(r);\n@@ -812,7 +824,7 @@ int reftable_reader_print_blocks(const char *tablename)\n \t}\n \n done:\n-\treftable_reader_free(r);\n+\treftable_reader_decref(r);\n \ttable_iter_close(&ti);\n \treturn err;\n }\ndiff --git a/reftable/reader.h b/reftable/reader.h\nindex 88b4f3b421..3710ee09b4 100644\n--- a/reftable/reader.h\n+++ b/reftable/reader.h\n@@ -50,6 +50,8 @@ struct reftable_reader {\n \tstruct reftable_reader_offsets ref_offsets;\n \tstruct reftable_reader_offsets obj_offsets;\n \tstruct reftable_reader_offsets log_offsets;\n+\n+\tuint64_t refcount;\n };\n \n const char *reader_name(struct reftable_reader *r);\ndiff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c\nindex 2f2ff787b2..0494e7955a 100644\n--- a/reftable/readwrite_test.c\n+++ b/reftable/readwrite_test.c\n@@ -279,7 +279,7 @@ static void test_log_write_read(void)\n \t/* cleanup. */\n \tstrbuf_release(&buf);\n \tfree_names(names);\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n }\n \n static void test_log_zlib_corruption(void)\n@@ -341,7 +341,7 @@ static void test_log_zlib_corruption(void)\n \treftable_iterator_destroy(&it);\n \n \t/* cleanup. */\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n \tstrbuf_release(&buf);\n }\n \n@@ -383,7 +383,7 @@ static void test_table_read_write_sequential(void)\n \tEXPECT(j == N);\n \n \treftable_iterator_destroy(&it);\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n \tstrbuf_release(&buf);\n \tfree_names(names);\n }\n@@ -431,7 +431,7 @@ static void test_table_read_api(void)\n \t}\n \treftable_iterator_destroy(&it);\n \treftable_free(names);\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n \tstrbuf_release(&buf);\n }\n \n@@ -498,7 +498,7 @@ static void test_table_read_write_seek(int index, int hash_id)\n \t\treftable_free(names[i]);\n \t}\n \treftable_free(names);\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n }\n \n static void test_table_read_write_seek_linear(void)\n@@ -611,7 +611,7 @@ static void test_table_refs_for(int indexed)\n \tstrbuf_release(&buf);\n \tfree_names(want_names);\n \treftable_iterator_destroy(&it);\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n }\n \n static void test_table_refs_for_no_index(void)\n@@ -657,7 +657,7 @@ static void test_write_empty_table(void)\n \tEXPECT(err > 0);\n \n \treftable_iterator_destroy(&it);\n-\treftable_reader_free(rd);\n+\treftable_reader_decref(rd);\n \tstrbuf_release(&buf);\n }\n \n@@ -863,7 +863,7 @@ static void test_write_multiple_indices(void)\n \n \treftable_iterator_destroy(&it);\n \treftable_writer_free(writer);\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n \tstrbuf_release(&writer_buf);\n \tstrbuf_release(&buf);\n }\n@@ -919,7 +919,7 @@ static void test_write_multi_level_index(void)\n \n \treftable_iterator_destroy(&it);\n \treftable_writer_free(writer);\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n \tstrbuf_release(&writer_buf);\n \tstrbuf_release(&buf);\n }\ndiff --git a/reftable/reftable-reader.h b/reftable/reftable-reader.h\nindex 8a05c84685..a600452b56 100644\n--- a/reftable/reftable-reader.h\n+++ b/reftable/reftable-reader.h\n@@ -33,6 +33,18 @@ struct reftable_reader;\n int reftable_reader_new(struct reftable_reader **pp,\n \t\t\tstruct reftable_block_source *src, const char *name);\n \n+/*\n+ * Manage the reference count of the reftable reader. A newly initialized\n+ * reader starts with a refcount of 1 and will be deleted once the refcount has\n+ * reached 0.\n+ *\n+ * This is required because readers may have longer lifetimes than the stack\n+ * they belong to. The stack may for example be reloaded while the old tables\n+ * are still being accessed by an iterator.\n+ */\n+void reftable_reader_incref(struct reftable_reader *reader);\n+void reftable_reader_decref(struct reftable_reader *reader);\n+\n /* Initialize a reftable iterator for reading refs. */\n void reftable_reader_init_ref_iterator(struct reftable_reader *r,\n \t\t\t\t       struct reftable_iterator *it);\n@@ -44,9 +56,6 @@ void reftable_reader_init_log_iterator(struct reftable_reader *r,\n /* returns the hash ID used in this table. */\n uint32_t reftable_reader_hash_id(struct reftable_reader *r);\n \n-/* closes and deallocates a reader. */\n-void reftable_reader_free(struct reftable_reader *);\n-\n /* return an iterator for the refs pointing to `oid`. */\n int reftable_reader_refs_for(struct reftable_reader *r,\n \t\t\t     struct reftable_iterator *it, uint8_t *oid);\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 0ac9cdf8de..8e85f8b4d9 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -186,7 +186,7 @@ void reftable_stack_destroy(struct reftable_stack *st)\n \t\t\tif (names && !has_name(names, name)) {\n \t\t\t\tstack_filename(&filename, st, name);\n \t\t\t}\n-\t\t\treftable_reader_free(st->readers[i]);\n+\t\t\treftable_reader_decref(st->readers[i]);\n \n \t\t\tif (filename.len) {\n \t\t\t\t/* On Windows, can only unlink after closing. */\n@@ -290,7 +290,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t\t\tconst char *name = reader_name(cur[i]);\n \t\t\tstack_filename(&table_path, st, name);\n \n-\t\t\treftable_reader_free(cur[i]);\n+\t\t\treftable_reader_decref(cur[i]);\n \n \t\t\t/* On Windows, can only unlink after closing. */\n \t\t\tunlink(table_path.buf);\n@@ -299,7 +299,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \n done:\n \tfor (i = 0; i < new_readers_len; i++)\n-\t\treftable_reader_free(new_readers[i]);\n+\t\treftable_reader_decref(new_readers[i]);\n \treftable_free(new_readers);\n \treftable_free(cur);\n \tstrbuf_release(&table_path);\n@@ -1534,7 +1534,7 @@ static void remove_maybe_stale_table(struct reftable_stack *st, uint64_t max,\n \t\tgoto done;\n \n \tupdate_idx = reftable_reader_max_update_index(rd);\n-\treftable_reader_free(rd);\n+\treftable_reader_decref(rd);\n \n \tif (update_idx <= max) {\n \t\tunlink(table_path.buf);\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex de0669b7b8..bc3bf77274 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -1036,10 +1036,8 @@ static void test_reftable_stack_compaction_concurrent(void)\n static void unclean_stack_close(struct reftable_stack *st)\n {\n \t/* break abstraction boundary to simulate unclean shutdown. */\n-\tint i = 0;\n-\tfor (; i < st->readers_len; i++) {\n-\t\treftable_reader_free(st->readers[i]);\n-\t}\n+\tfor (size_t i = 0; i < st->readers_len; i++)\n+\t\treftable_reader_decref(st->readers[i]);\n \tst->readers_len = 0;\n \tFREE_AND_NULL(st->readers);\n }\ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex 87c2f42aae..f6d855a9d7 100644\n--- a/t/helper/test-reftable.c\n+++ b/t/helper/test-reftable.c\n@@ -152,7 +152,7 @@ static int dump_reftable(const char *tablename)\n \n done:\n \treftable_merged_table_free(mt);\n-\treftable_reader_free(r);\n+\treftable_reader_decref(r);\n \treturn err;\n }\n \ndiff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c\nindex 8f51aca1b6..081d3c8b69 100644\n--- a/t/unit-tests/t-reftable-merged.c\n+++ b/t/unit-tests/t-reftable-merged.c\n@@ -115,7 +115,7 @@ merged_table_from_records(struct reftable_ref_record **refs,\n static void readers_destroy(struct reftable_reader **readers, const size_t n)\n {\n \tfor (size_t i = 0; i < n; i++)\n-\t\treftable_reader_free(readers[i]);\n+\t\treftable_reader_decref(readers[i]);\n \treftable_free(readers);\n }\n \n@@ -437,7 +437,7 @@ static void t_default_write_opts(void)\n \terr = reftable_merged_table_new(&merged, &rd, 1, GIT_SHA1_FORMAT_ID);\n \tcheck(!err);\n \n-\treftable_reader_free(rd);\n+\treftable_reader_decref(rd);\n \treftable_merged_table_free(merged);\n \tstrbuf_release(&buf);\n }\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501279","messageId":"026820562882afb31d7224c90722e09bef835340.1724080006.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724080006.git.ps@pks.im","subject":"[PATCH 08/10] reftable/reader: keep readers alive during iteration","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-19T15:40:07Z","receivedAt":"2024-08-19T15:40:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The lifetime of a table iterator may surive the lifetime of a reader\nwhen the stack gets reloaded. Keep the reader from being released by\nincreasing its refcount while the iterator is still being used.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/reader.c     |  2 ++\n reftable/stack_test.c | 49 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 51 insertions(+)\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 64a0953e68..f877099087 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -175,6 +175,7 @@ static int table_iter_init(struct table_iter *ti, struct reftable_reader *r)\n {\n \tstruct block_iter bi = BLOCK_ITER_INIT;\n \tmemset(ti, 0, sizeof(*ti));\n+\treftable_reader_incref(r);\n \tti->r = r;\n \tti->bi = bi;\n \treturn 0;\n@@ -262,6 +263,7 @@ static void table_iter_close(struct table_iter *ti)\n {\n \ttable_iter_block_done(ti);\n \tblock_iter_close(&ti->bi);\n+\treftable_reader_decref(ti->r);\n }\n \n static int table_iter_next_block(struct table_iter *ti)\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex bc3bf77274..91e716dc0a 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -1076,6 +1076,54 @@ static void test_reftable_stack_compaction_concurrent_clean(void)\n \tclear_dir(dir);\n }\n \n+static void test_reftable_stack_read_across_reload(void)\n+{\n+\tstruct reftable_write_options opts = { 0 };\n+\tstruct reftable_stack *st1 = NULL, *st2 = NULL;\n+\tstruct reftable_ref_record rec = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tint err;\n+\n+\t/* Create a first stack and set up an iterator for it. */\n+\terr = reftable_new_stack(&st1, dir, &opts);\n+\tEXPECT_ERR(err);\n+\twrite_n_ref_tables(st1, 2);\n+\tEXPECT(st1->merged->readers_len == 2);\n+\treftable_stack_init_ref_iterator(st1, &it);\n+\terr = reftable_iterator_seek_ref(&it, \"\");\n+\tEXPECT_ERR(err);\n+\n+\t/* Set up a second stack for the same directory and compact it. */\n+\terr = reftable_new_stack(&st2, dir, &opts);\n+\tEXPECT_ERR(err);\n+\tEXPECT(st2->merged->readers_len == 2);\n+\terr = reftable_stack_compact_all(st2, NULL);\n+\tEXPECT_ERR(err);\n+\n+\t/*\n+\t * Verify that we can continue to use the old iterator even after we\n+\t * have reloaded its stack.\n+\t */\n+\terr = reftable_stack_reload(st1);\n+\tEXPECT_ERR(err);\n+\tEXPECT(st2->merged->readers_len == 1);\n+\terr = reftable_iterator_next_ref(&it, &rec);\n+\tEXPECT_ERR(err);\n+\tEXPECT(!strcmp(rec.refname, \"refs/heads/branch-0000\"));\n+\terr = reftable_iterator_next_ref(&it, &rec);\n+\tEXPECT_ERR(err);\n+\tEXPECT(!strcmp(rec.refname, \"refs/heads/branch-0001\"));\n+\terr = reftable_iterator_next_ref(&it, &rec);\n+\tEXPECT(err > 0);\n+\n+\treftable_ref_record_release(&rec);\n+\treftable_iterator_destroy(&it);\n+\treftable_stack_destroy(st1);\n+\treftable_stack_destroy(st2);\n+\tclear_dir(dir);\n+}\n+\n int stack_test_main(int argc, const char *argv[])\n {\n \tRUN_TEST(test_empty_add);\n@@ -1098,6 +1146,7 @@ int stack_test_main(int argc, const char *argv[])\n \tRUN_TEST(test_reftable_stack_auto_compaction_fails_gracefully);\n \tRUN_TEST(test_reftable_stack_update_index_check);\n \tRUN_TEST(test_reftable_stack_uptodate);\n+\tRUN_TEST(test_reftable_stack_read_across_reload);\n \tRUN_TEST(test_suggest_compaction_segment);\n \tRUN_TEST(test_suggest_compaction_segment_nothing);\n \treturn 0;\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501280","messageId":"d98316fbf4c440609462475d823abd2ba29eed41.1724080006.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724080006.git.ps@pks.im","subject":"[PATCH 09/10] reftable/stack: reorder swapping in the reloaded stack contents","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-19T15:40:10Z","receivedAt":"2024-08-19T15:40:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The code flow of how we swap in the reloaded stack contents is somewhat\nconvoluted because we switch back and forth between swapping in\ndifferent parts of the stack.\n\nReorder the code to simplify it. We now first close and unlink the old\ntables which do not get reused before we update the stack to point to\nthe new stack.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 34 +++++++++++++++++++---------------\n 1 file changed, 19 insertions(+), 15 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 8e85f8b4d9..0247222258 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -273,30 +273,34 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \tif (err < 0)\n \t\tgoto done;\n \n-\tst->readers_len = new_readers_len;\n-\tif (st->merged)\n-\t\treftable_merged_table_free(st->merged);\n-\tif (st->readers) {\n-\t\treftable_free(st->readers);\n-\t}\n-\tst->readers = new_readers;\n-\tnew_readers = NULL;\n-\tnew_readers_len = 0;\n-\n-\tnew_merged->suppress_deletions = 1;\n-\tst->merged = new_merged;\n+\t/*\n+\t * Close the old, non-reused readers and proactively try to unlink\n+\t * them. This is done for systems like Windows, where the underlying\n+\t * file of such an open reader wouldn't have been possible to be\n+\t * unlinked by the compacting process.\n+\t */\n \tfor (i = 0; i < cur_len; i++) {\n \t\tif (cur[i]) {\n \t\t\tconst char *name = reader_name(cur[i]);\n \t\t\tstack_filename(&table_path, st, name);\n-\n \t\t\treftable_reader_decref(cur[i]);\n-\n-\t\t\t/* On Windows, can only unlink after closing. */\n \t\t\tunlink(table_path.buf);\n \t\t}\n \t}\n \n+\t/* Update the stack to point to the new tables. */\n+\tif (st->merged)\n+\t\treftable_merged_table_free(st->merged);\n+\tnew_merged->suppress_deletions = 1;\n+\tst->merged = new_merged;\n+\n+\tif (st->readers)\n+\t\treftable_free(st->readers);\n+\tst->readers = new_readers;\n+\tst->readers_len = new_readers_len;\n+\tnew_readers = NULL;\n+\tnew_readers_len = 0;\n+\n done:\n \tfor (i = 0; i < new_readers_len; i++)\n \t\treftable_reader_decref(new_readers[i]);\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501281","messageId":"b777818ea998806b75a1ed84a5ceefd28f0ca523.1724080006.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724080006.git.ps@pks.im","subject":"[PATCH 10/10] reftable/stack: fix segfault when reload with reused readers fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-19T15:40:14Z","receivedAt":"2024-08-19T15:40:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It is expected that reloading the stack fails with concurrent writers,\ne.g. because a table that we just wanted to read just got compacted.\nIn case we decided to reuse readers this will cause a segfault though\nbecause we unconditionally release all new readers, including the reused\nones. As those are still referenced by the current stack, the result is\nthat we will eventually try to dereference those already-freed readers.\n\nFix this bug by incrementing the refcount of reused readers temporarily.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c      | 23 +++++++++++++++++\n reftable/stack_test.c | 59 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 82 insertions(+)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 0247222258..ce0a35216b 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -226,6 +226,8 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n {\n \tsize_t cur_len = !st->merged ? 0 : st->merged->readers_len;\n \tstruct reftable_reader **cur = stack_copy_readers(st, cur_len);\n+\tstruct reftable_reader **reused = NULL;\n+\tsize_t reused_len = 0, reused_alloc = 0;\n \tsize_t names_len = names_length(names);\n \tstruct reftable_reader **new_readers =\n \t\treftable_calloc(names_len, sizeof(*new_readers));\n@@ -245,6 +247,18 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t\t\tif (cur[i] && 0 == strcmp(cur[i]->name, name)) {\n \t\t\t\trd = cur[i];\n \t\t\t\tcur[i] = NULL;\n+\n+\t\t\t\t/*\n+\t\t\t\t * When reloading the stack fails, we end up\n+\t\t\t\t * releasing all new readers. This also\n+\t\t\t\t * includes the reused readers, even though\n+\t\t\t\t * they are still in used by the old stack. We\n+\t\t\t\t * thus need to keep them alive here, which we\n+\t\t\t\t * do by bumping their refcount.\n+\t\t\t\t */\n+\t\t\t\tREFTABLE_ALLOC_GROW(reused, reused_len + 1, reused_alloc);\n+\t\t\t\treused[reused_len++] = rd;\n+\t\t\t\treftable_reader_incref(rd);\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n@@ -301,10 +315,19 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \tnew_readers = NULL;\n \tnew_readers_len = 0;\n \n+\t/*\n+\t * Decrement the refcount of reused readers again. This only needs to\n+\t * happen on the successful case, because on the unsuccessful one we\n+\t * decrement their refcount via `new_readers`.\n+\t */\n+\tfor (i = 0; i < reused_len; i++)\n+\t\treftable_reader_decref(reused[i]);\n+\n done:\n \tfor (i = 0; i < new_readers_len; i++)\n \t\treftable_reader_decref(new_readers[i]);\n \treftable_free(new_readers);\n+\treftable_free(reused);\n \treftable_free(cur);\n \tstrbuf_release(&table_path);\n \treturn err;\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 91e716dc0a..30cdb1739c 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -10,6 +10,7 @@ license that can be found in the LICENSE file or at\n \n #include \"system.h\"\n \n+#include \"copy.h\"\n #include \"reftable-reader.h\"\n #include \"merged.h\"\n #include \"basics.h\"\n@@ -1124,6 +1125,63 @@ static void test_reftable_stack_read_across_reload(void)\n \tclear_dir(dir);\n }\n \n+static void test_reftable_stack_reload_with_missing_table(void)\n+{\n+\tstruct reftable_write_options opts = { 0 };\n+\tstruct reftable_stack *st = NULL;\n+\tstruct reftable_ref_record rec = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n+\tstruct strbuf table_path = STRBUF_INIT, content = STRBUF_INIT;\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tint err;\n+\n+\t/* Create a first stack and set up an iterator for it. */\n+\terr = reftable_new_stack(&st, dir, &opts);\n+\tEXPECT_ERR(err);\n+\twrite_n_ref_tables(st, 2);\n+\tEXPECT(st->merged->readers_len == 2);\n+\treftable_stack_init_ref_iterator(st, &it);\n+\terr = reftable_iterator_seek_ref(&it, \"\");\n+\tEXPECT_ERR(err);\n+\n+\t/*\n+\t * Update the tables.list file with some garbage data, while reusing\n+\t * our old readers. This should trigger a partial reload of the stack,\n+\t * where we try to reuse our old readers.\n+\t*/\n+\tstrbuf_addf(&content, \"%s\\n\", st->readers[0]->name);\n+\tstrbuf_addf(&content, \"%s\\n\", st->readers[1]->name);\n+\tstrbuf_addstr(&content, \"garbage\\n\");\n+\tstrbuf_addf(&table_path, \"%s.lock\", st->list_file);\n+\twrite_file_buf(table_path.buf, content.buf, content.len);\n+\terr = rename(table_path.buf, st->list_file);\n+\tEXPECT_ERR(err);\n+\n+\terr = reftable_stack_reload(st);\n+\tEXPECT(err == -4);\n+\tEXPECT(st->merged->readers_len == 2);\n+\n+\t/*\n+\t * Even though the reload has failed, we should be able to continue\n+\t * using the iterator.\n+\t*/\n+\terr = reftable_iterator_next_ref(&it, &rec);\n+\tEXPECT_ERR(err);\n+\tEXPECT(!strcmp(rec.refname, \"refs/heads/branch-0000\"));\n+\terr = reftable_iterator_next_ref(&it, &rec);\n+\tEXPECT_ERR(err);\n+\tEXPECT(!strcmp(rec.refname, \"refs/heads/branch-0001\"));\n+\terr = reftable_iterator_next_ref(&it, &rec);\n+\tEXPECT(err > 0);\n+\n+\treftable_ref_record_release(&rec);\n+\treftable_iterator_destroy(&it);\n+\treftable_stack_destroy(st);\n+\tstrbuf_release(&table_path);\n+\tstrbuf_release(&content);\n+\tclear_dir(dir);\n+}\n+\n int stack_test_main(int argc, const char *argv[])\n {\n \tRUN_TEST(test_empty_add);\n@@ -1147,6 +1205,7 @@ int stack_test_main(int argc, const char *argv[])\n \tRUN_TEST(test_reftable_stack_update_index_check);\n \tRUN_TEST(test_reftable_stack_uptodate);\n \tRUN_TEST(test_reftable_stack_read_across_reload);\n+\tRUN_TEST(test_reftable_stack_reload_with_missing_table);\n \tRUN_TEST(test_suggest_compaction_segment);\n \tRUN_TEST(test_suggest_compaction_segment_nothing);\n \treturn 0;\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501492","messageId":"CAOLa=ZR+bbYrYrZ79zPY3eYWQRQo97NfmcJXS=-nD842g7=XiQ@mail.gmail.com","threadId":"61975","inReplyTo":"cover.1724080006.git.ps@pks.im","subject":"Re: [PATCH 00/10] reftable: fix reload with active iterators","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-08-22T08:13:11Z","receivedAt":"2024-08-22T08:13:13Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Hi,\n>\n> as reported by Peff in [1], the reftable backend fails in t5616 in a\n> racy manner. The cause of this is that git-maintenance(1) processes leak\n> into subsequent tests and perform concurrent compaction of the stack.\n> The reftable backend is expected to handle this gracefully, but doesn't.\n>\n\nI've not looked into the recent changes made to `git-maintenance(1)` but\nmy understanding is that, now, `git-maintenance(1)` can detach itself\nwithout relying on `git-gc(1)`'s detach.\n\n> The issue can surface whenever reloading the reftable stack while an\n> iterator is active. In case some of the tables got compacted, we will\n> end up closing them and thus the iterator now tries to use those closed\n> tables.\n>\n\nMake sense till here.\n\n> This patch series fixes that issue by starting to refcount the readers.\n\nNot sure what you mean by 'refcount' here though.\n\n> Each iterator will bump the refcount, thus avoiding the problem. While\n> at it, I also found a second issue where we segfault when reloading a\n> table fails while reusing one of the table readers. In this scenario, we\n> would end up releasing the reader of the stack itself, even though it\n> would still be used by it.\n>\n> This patch series addresses those issues by making the reftable reader\n> refcounted. The first 6 patches do some simplifications of code which is\n> close. The remaining 4 ones introduce refcounting and wire it up as\n> required.\n>\n\nPerhaps it becomes more clearer as I read through the patches.\n\n[snip]\n"},{"id":"501498","messageId":"CAOLa=ZQNArqiSaaNHfp_jd1CRsX7-iV4iMyaxG+JUVA5gGj2VA@mail.gmail.com","threadId":"61975","inReplyTo":"3c0cf2bf46f4106d7c0b30c863946262639b2351.1724080006.git.ps@pks.im","subject":"Re: [PATCH 02/10] reftable/stack: inline `stack_compact_range_stats()`","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-08-22T08:23:48Z","receivedAt":"2024-08-22T08:23:49Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The only difference between `stack_compact_range_stats()` and\n> `stack_compact_range()` is that the former updates stats on failure,\n> whereas the latter doesn't. There are no callers anymore that do not\n> want their stats updated though, making the indirection unnecessary.\n>\n> Inline the stat updates into `stack_compact_range()`.\n>\n\nThis also ensures that we don't have users who end up using\n`stack_compact_range()` because they didn't know about\n`stack_compact_range_stats()`. Which was the case before, I think, I\nremember one of the compaction function calling `stack_compact_range()`\ndirectly. I see it is no longer the case though. Nice.\n\n[snip]\n"},{"id":"501527","messageId":"CAOLa=ZThdm98qrcQVu2uXkHJ2meEnJJCsBSPSLMQeSwsojQ-Fg@mail.gmail.com","threadId":"61975","inReplyTo":"fc0ed68d4675bcd4c89bf63419ec6e8b6b7f5fca.1724080006.git.ps@pks.im","subject":"Re: [PATCH 07/10] reftable/reader: introduce refcounting","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-08-22T09:47:43Z","receivedAt":"2024-08-22T09:47:44Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> It was recently reported that concurrent reads and writes may cause the\n> reftable backend to segfault. The root cause of this is that we do not\n> properly keep track of reftable readers across reloads.\n>\n> Suppose that you have a reftable iterator and then decide to reload the\n> stack while iterating through the iterator. When the stack has been\n> rewritten since we have created the iterator, then we would end up\n> discarding a subset of readers that may still be in use by the iterator.\n> The consequence is that we now try to reference deallocated memory,\n> which of course segfaults.\n>\n> One way to trigger this is in t5616, where some background maintenance\n> jobs have been leaking from one test into another. This leads to stack\n> traces like the following one:\n>\n>   + git -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 --refetch origin\n>   AddressSanitizer:DEADLYSIGNAL\n>   =================================================================\n>   ==657994==ERROR: AddressSanitizer: SEGV on unknown address 0x7fa0f0ec6089 (pc 0x55f23e52ddf9 bp\n> 0x7ffe7bfa1700 sp 0x7ffe7bfa1700 T0)\n>   ==657994==The signal is caused by a READ memory access.\n>       #0 0x55f23e52ddf9 in get_var_int reftable/record.c:29\n>       #1 0x55f23e53295e in reftable_decode_keylen reftable/record.c:170\n>       #2 0x55f23e532cc0 in reftable_decode_key reftable/record.c:194\n>       #3 0x55f23e54e72e in block_iter_next reftable/block.c:398\n>       #4 0x55f23e5573dc in table_iter_next_in_block reftable/reader.c:240\n>       #5 0x55f23e5573dc in table_iter_next reftable/reader.c:355\n>       #6 0x55f23e5573dc in table_iter_next reftable/reader.c:339\n>       #7 0x55f23e551283 in merged_iter_advance_subiter reftable/merged.c:69\n>       #8 0x55f23e55169e in merged_iter_next_entry reftable/merged.c:123\n>       #9 0x55f23e55169e in merged_iter_next_void reftable/merged.c:172\n>       #10 0x55f23e537625 in reftable_iterator_next_ref reftable/generic.c:175\n>       #11 0x55f23e2cf9c6 in reftable_ref_iterator_advance refs/reftable-backend.c:464\n>       #12 0x55f23e2d996e in ref_iterator_advance refs/iterator.c:13\n>       #13 0x55f23e2d996e in do_for_each_ref_iterator refs/iterator.c:452\n>       #14 0x55f23dca6767 in get_ref_map builtin/fetch.c:623\n>       #15 0x55f23dca6767 in do_fetch builtin/fetch.c:1659\n>       #16 0x55f23dca6767 in fetch_one builtin/fetch.c:2133\n>       #17 0x55f23dca6767 in cmd_fetch builtin/fetch.c:2432\n>       #18 0x55f23dba7764 in run_builtin git.c:484\n>       #19 0x55f23dba7764 in handle_builtin git.c:741\n>       #20 0x55f23dbab61e in run_argv git.c:805\n>       #21 0x55f23dbab61e in cmd_main git.c:1000\n>       #22 0x55f23dba4781 in main common-main.c:64\n>       #23 0x7fa0f063fc89 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n>       #24 0x7fa0f063fd44 in __libc_start_main_impl ../csu/libc-start.c:360\n>       #25 0x55f23dba6ad0 in _start (git+0xadfad0) (BuildId: 803b2b7f59beb03d7849fb8294a8e2145dd4aa27)\n>\n\nThe stacktrace is for iterating over refs, what I don't understand is\nwhere in this flow do we actually reload the stack.\n\n> While it is somewhat awkward that the maintenance processes survive\n> tests in the first place, it is totally expected that reftables should\n> work alright with concurrent writers. Seemingly they don't.\n>\n> The only underlying resource that we need to care about in this context\n> is the reftable reader, which is responsible for reading a single table\n> from disk. These readers get discarded immediately (unless reused) when\n> calling `reftable_stack_reload()`, which is wrong. We can only close\n> them once we know that there are no iterators using them anymore.\n>\n> Prepare for a fix by converting the reftable readers to be refcounted.\n>\n\nOkay so my understanding is that `refcounted` refers to a reference\ncount which keeps tracks of the stacks which are referring to the\nreader. The name is also used in `struct blame_origin` in blame.{c,h}.\nMakes a lot more sense now :)\n\n> Reported-by: Jeff King <peff@peff.net>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  reftable/reader.c                | 16 ++++++++++++++--\n>  reftable/reader.h                |  2 ++\n>  reftable/readwrite_test.c        | 18 +++++++++---------\n>  reftable/reftable-reader.h       | 15 ++++++++++++---\n>  reftable/stack.c                 |  8 ++++----\n>  reftable/stack_test.c            |  6 ++----\n>  t/helper/test-reftable.c         |  2 +-\n>  t/unit-tests/t-reftable-merged.c |  4 ++--\n>  8 files changed, 46 insertions(+), 25 deletions(-)\n>\n> diff --git a/reftable/reader.c b/reftable/reader.c\n> index 037417fcf6..64a0953e68 100644\n> --- a/reftable/reader.c\n> +++ b/reftable/reader.c\n> @@ -621,6 +621,7 @@ int reftable_reader_new(struct reftable_reader **out,\n>  \tr->source = *source;\n>  \tr->name = xstrdup(name);\n>  \tr->hash_id = 0;\n> +\tr->refcount = 1;\n>\n\nSo when the reader is initialized by someone, this sets the counter to one.\n\n>  \terr = block_source_read_block(source, &footer, r->size,\n>  \t\t\t\t      footer_size(r->version));\n> @@ -645,10 +646,21 @@ int reftable_reader_new(struct reftable_reader **out,\n>  \treturn err;\n>  }\n>\n> -void reftable_reader_free(struct reftable_reader *r)\n> +void reftable_reader_incref(struct reftable_reader *r)\n> +{\n> +\tif (!r->refcount)\n> +\t\tBUG(\"cannot increment ref counter of dead reader\");\n> +\tr->refcount++;\n> +}\n> +\n> +void reftable_reader_decref(struct reftable_reader *r)\n>  {\n>  \tif (!r)\n>  \t\treturn;\n> +\tif (!r->refcount)\n> +\t\tBUG(\"cannot decrement ref counter of dead reader\");\n> +\tif (--r->refcount)\n> +\t\treturn;\n>  \tblock_source_close(&r->source);\n>  \tFREE_AND_NULL(r->name);\n>  \treftable_free(r);\n> @@ -812,7 +824,7 @@ int reftable_reader_print_blocks(const char *tablename)\n>  \t}\n>\n>  done:\n> -\treftable_reader_free(r);\n> +\treftable_reader_decref(r);\n>  \ttable_iter_close(&ti);\n>  \treturn err;\n>  }\n>\n\nWe remove the `_free()` function and instead introduce `_incref()` and\n`_decref()` to increase and decrease the counter. The latter takes over\nthe functionality of `_free()` when counter hits 0.\n\n> diff --git a/reftable/reader.h b/reftable/reader.h\n> index 88b4f3b421..3710ee09b4 100644\n> --- a/reftable/reader.h\n> +++ b/reftable/reader.h\n> @@ -50,6 +50,8 @@ struct reftable_reader {\n>  \tstruct reftable_reader_offsets ref_offsets;\n>  \tstruct reftable_reader_offsets obj_offsets;\n>  \tstruct reftable_reader_offsets log_offsets;\n> +\n> +\tuint64_t refcount;\n\nWonder if there is a chance that we decrement refcount from 0 and hence\ncause a wraparound.\n\n[snip]\n\nI have some questions regarding my own understanding, but the patch\nlooks good to me.\n"},{"id":"501529","messageId":"ZscilS_kLcJyHThx@framework","threadId":"61975","inReplyTo":"CAOLa=ZThdm98qrcQVu2uXkHJ2meEnJJCsBSPSLMQeSwsojQ-Fg@mail.gmail.com","subject":"Re: [PATCH 07/10] reftable/reader: introduce refcounting","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-22T11:35:49Z","receivedAt":"2024-08-22T11:35:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 22, 2024 at 02:47:43AM -0700, karthik nayak wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > It was recently reported that concurrent reads and writes may cause the\n> > reftable backend to segfault. The root cause of this is that we do not\n> > properly keep track of reftable readers across reloads.\n> >\n> > Suppose that you have a reftable iterator and then decide to reload the\n> > stack while iterating through the iterator. When the stack has been\n> > rewritten since we have created the iterator, then we would end up\n> > discarding a subset of readers that may still be in use by the iterator.\n> > The consequence is that we now try to reference deallocated memory,\n> > which of course segfaults.\n> >\n> > One way to trigger this is in t5616, where some background maintenance\n> > jobs have been leaking from one test into another. This leads to stack\n> > traces like the following one:\n> >\n> >   + git -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 --refetch origin\n> >   AddressSanitizer:DEADLYSIGNAL\n> >   =================================================================\n> >   ==657994==ERROR: AddressSanitizer: SEGV on unknown address 0x7fa0f0ec6089 (pc 0x55f23e52ddf9 bp\n> > 0x7ffe7bfa1700 sp 0x7ffe7bfa1700 T0)\n> >   ==657994==The signal is caused by a READ memory access.\n> >       #0 0x55f23e52ddf9 in get_var_int reftable/record.c:29\n> >       #1 0x55f23e53295e in reftable_decode_keylen reftable/record.c:170\n> >       #2 0x55f23e532cc0 in reftable_decode_key reftable/record.c:194\n> >       #3 0x55f23e54e72e in block_iter_next reftable/block.c:398\n> >       #4 0x55f23e5573dc in table_iter_next_in_block reftable/reader.c:240\n> >       #5 0x55f23e5573dc in table_iter_next reftable/reader.c:355\n> >       #6 0x55f23e5573dc in table_iter_next reftable/reader.c:339\n> >       #7 0x55f23e551283 in merged_iter_advance_subiter reftable/merged.c:69\n> >       #8 0x55f23e55169e in merged_iter_next_entry reftable/merged.c:123\n> >       #9 0x55f23e55169e in merged_iter_next_void reftable/merged.c:172\n> >       #10 0x55f23e537625 in reftable_iterator_next_ref reftable/generic.c:175\n> >       #11 0x55f23e2cf9c6 in reftable_ref_iterator_advance refs/reftable-backend.c:464\n> >       #12 0x55f23e2d996e in ref_iterator_advance refs/iterator.c:13\n> >       #13 0x55f23e2d996e in do_for_each_ref_iterator refs/iterator.c:452\n> >       #14 0x55f23dca6767 in get_ref_map builtin/fetch.c:623\n> >       #15 0x55f23dca6767 in do_fetch builtin/fetch.c:1659\n> >       #16 0x55f23dca6767 in fetch_one builtin/fetch.c:2133\n> >       #17 0x55f23dca6767 in cmd_fetch builtin/fetch.c:2432\n> >       #18 0x55f23dba7764 in run_builtin git.c:484\n> >       #19 0x55f23dba7764 in handle_builtin git.c:741\n> >       #20 0x55f23dbab61e in run_argv git.c:805\n> >       #21 0x55f23dbab61e in cmd_main git.c:1000\n> >       #22 0x55f23dba4781 in main common-main.c:64\n> >       #23 0x7fa0f063fc89 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n> >       #24 0x7fa0f063fd44 in __libc_start_main_impl ../csu/libc-start.c:360\n> >       #25 0x55f23dba6ad0 in _start (git+0xadfad0) (BuildId: 803b2b7f59beb03d7849fb8294a8e2145dd4aa27)\n> >\n> \n> The stacktrace is for iterating over refs, what I don't understand is\n> where in this flow do we actually reload the stack.\n\nBasically, whenever you call into the reftable backend we check whether\nwe need to reload the stack. So, when creating a reftable iterator,\nreading a single ref, writing refs and so on. So in the above code flow\nwe had a ref iterator, but during iteration we ended up reading other\nrefs, as well.\n\n> > While it is somewhat awkward that the maintenance processes survive\n> > tests in the first place, it is totally expected that reftables should\n> > work alright with concurrent writers. Seemingly they don't.\n> >\n> > The only underlying resource that we need to care about in this context\n> > is the reftable reader, which is responsible for reading a single table\n> > from disk. These readers get discarded immediately (unless reused) when\n> > calling `reftable_stack_reload()`, which is wrong. We can only close\n> > them once we know that there are no iterators using them anymore.\n> >\n> > Prepare for a fix by converting the reftable readers to be refcounted.\n> >\n> \n> Okay so my understanding is that `refcounted` refers to a reference\n> count which keeps tracks of the stacks which are referring to the\n> reader. The name is also used in `struct blame_origin` in blame.{c,h}.\n> Makes a lot more sense now :)\n\nYup.\n\n> > diff --git a/reftable/reader.h b/reftable/reader.h\n> > index 88b4f3b421..3710ee09b4 100644\n> > --- a/reftable/reader.h\n> > +++ b/reftable/reader.h\n> > @@ -50,6 +50,8 @@ struct reftable_reader {\n> >  \tstruct reftable_reader_offsets ref_offsets;\n> >  \tstruct reftable_reader_offsets obj_offsets;\n> >  \tstruct reftable_reader_offsets log_offsets;\n> > +\n> > +\tuint64_t refcount;\n> \n> Wonder if there is a chance that we decrement refcount from 0 and hence\n> cause a wraparound.\n\nThis should never happen in practice. And if it does, we would hit a\nBUG():\n\n    void reftable_reader_decref(struct reftable_reader *r)\n    {\n    \tif (!r)\n    \t\treturn;\n    \tif (!r->refcount)\n    \t\tBUG(\"cannot decrement ref counter of dead reader\");\n    \tif (--r->refcount)\n    \t\treturn;\n    \tblock_source_close(&r->source);\n    \tFREE_AND_NULL(r->name);\n    \treftable_free(r);\n    }\n\nIf the refcount is at zero already, we hit the bug.\n\nPatrick\n"},{"id":"501531","messageId":"20240822124100.GA1070988@coredump.intra.peff.net","threadId":"61975","inReplyTo":"cover.1724080006.git.ps@pks.im","subject":"Re: [PATCH 00/10] reftable: fix reload with active iterators","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-22T12:41:00Z","receivedAt":"2024-08-22T12:41:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 19, 2024 at 05:39:38PM +0200, Patrick Steinhardt wrote:\n\n> This patch series fixes that issue by starting to refcount the readers.\n> Each iterator will bump the refcount, thus avoiding the problem. While\n> at it, I also found a second issue where we segfault when reloading a\n> table fails while reusing one of the table readers. In this scenario, we\n> would end up releasing the reader of the stack itself, even though it\n> would still be used by it.\n\nI gave a fairly cursory look over this, as I'm not all that familiar\nwith the reftable code. But everything looked pretty sensible to me.\n\nI wondered how we might test this. It looks like you checked the\n--stress output (which I confirmed seems fixed for me), along with a\nsynthetic test directly calling various reftable functions. Both seem\ngood to me.\n\nI had hoped to be able to have a non-racy external test, where running\nactual Git commands showed the segfault in a deterministic way. But I\ncouldn't come up with one. One trick we've used for \"pausing\" a reading\nprocess is to run \"cat-file --batch\" from a fifo. You can ask it to do\nsome ref lookups, then while it waits for more input, update the\nreftable, and then ask it to do more.\n\nBut I don't think that's sufficient here, as the race happens while we\nare actually iterating. You'd really need some for_each_ref() callback\nthat blocks in some externally controllable way. Possibly you could do\nsomething clever with partial-clone lazy fetches (where you stall the\nfetch and then do ref updates in the middle), but that is getting pretty\nconvoluted.\n\nSo I think the tests you included seem like a good place to stop.\n\nI did have a little trouble applying this for testing. I wanted to do it\non 'next', which has the maintenance changes to cause the race. But\nmerging it there ended up with a lot of conflicts with other reftable\ntopics (especially the tests). I was able to resolve them all, but you\nmight be able to make Junio's life easier by coordinating the topics a\nbit.\n\n-Peff\n"},{"id":"501533","messageId":"Zsc03CSJ3Ece5L6s@tanuki","threadId":"61975","inReplyTo":"20240822124100.GA1070988@coredump.intra.peff.net","subject":"Re: [PATCH 00/10] reftable: fix reload with active iterators","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-22T12:53:48Z","receivedAt":"2024-08-22T12:53:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 22, 2024 at 08:41:00AM -0400, Jeff King wrote:\n> On Mon, Aug 19, 2024 at 05:39:38PM +0200, Patrick Steinhardt wrote:\n> \n> > This patch series fixes that issue by starting to refcount the readers.\n> > Each iterator will bump the refcount, thus avoiding the problem. While\n> > at it, I also found a second issue where we segfault when reloading a\n> > table fails while reusing one of the table readers. In this scenario, we\n> > would end up releasing the reader of the stack itself, even though it\n> > would still be used by it.\n> \n> I gave a fairly cursory look over this, as I'm not all that familiar\n> with the reftable code. But everything looked pretty sensible to me.\n\nThanks for your review, appreciated!\n\n> I wondered how we might test this. It looks like you checked the\n> --stress output (which I confirmed seems fixed for me), along with a\n> synthetic test directly calling various reftable functions. Both seem\n> good to me.\n> \n> I had hoped to be able to have a non-racy external test, where running\n> actual Git commands showed the segfault in a deterministic way. But I\n> couldn't come up with one. One trick we've used for \"pausing\" a reading\n> process is to run \"cat-file --batch\" from a fifo. You can ask it to do\n> some ref lookups, then while it waits for more input, update the\n> reftable, and then ask it to do more.\n> \n> But I don't think that's sufficient here, as the race happens while we\n> are actually iterating. You'd really need some for_each_ref() callback\n> that blocks in some externally controllable way. Possibly you could do\n> something clever with partial-clone lazy fetches (where you stall the\n> fetch and then do ref updates in the middle), but that is getting pretty\n> convoluted.\n> \n> So I think the tests you included seem like a good place to stop.\n\nYeah. I think demonstrating the issues on the API level is okayish. It\nwould of course have been nice to also do it via our shell scripts, but\nas you say, it feels rather fragile.\n\n> I did have a little trouble applying this for testing. I wanted to do it\n> on 'next', which has the maintenance changes to cause the race. But\n> merging it there ended up with a lot of conflicts with other reftable\n> topics (especially the tests). I was able to resolve them all, but you\n> might be able to make Junio's life easier by coordinating the topics a\n> bit.\n\nI can certainly do that. I know that there are conflicts both with the\npatch series dropping the generic tables and with the patch series that\nmove the reftable unit tests into our own codebase. I did address the\nformer by basing it on top of that series, but didn't yet address the\nlatter.\n\nI'm okay with waiting a bit until most of the conflicting topics land. I\nguess most of them should be close to landing anyway. Alternatively, I\ncan also pull all of them in as dependencies.\n\nPatrick\n"},{"id":"501539","messageId":"xmqqr0agvbi3.fsf@gitster.g","threadId":"61975","inReplyTo":"20240822124100.GA1070988@coredump.intra.peff.net","subject":"Re: [PATCH 00/10] reftable: fix reload with active iterators","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-22T15:11:00Z","receivedAt":"2024-08-22T15:11:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But I don't think that's sufficient here, as the race happens while we\n> are actually iterating. You'd really need some for_each_ref() callback\n> that blocks in some externally controllable way. Possibly you could do\n> something clever with partial-clone lazy fetches (where you stall the\n> fetch and then do ref updates in the middle), but that is getting pretty\n> convoluted.\n\n;-)\n\n> So I think the tests you included seem like a good place to stop.\n\nYeah, I agree with this assessment.\n\n> I did have a little trouble applying this for testing. I wanted to do it\n> on 'next', which has the maintenance changes to cause the race. But\n> merging it there ended up with a lot of conflicts with other reftable\n> topics (especially the tests). I was able to resolve them all, but you\n> might be able to make Junio's life easier by coordinating the topics a\n> bit.\n\nI haven't tried merging the topic to 'next', jumping over other\ntopics in flight in 'seen'.  Thanks for a heads up, but I am hoping\nthat my rerere database is fairly complete for this topic by now.\n\nThanks.\n"},{"id":"501540","messageId":"xmqqmsl4vb9w.fsf@gitster.g","threadId":"61975","inReplyTo":"Zsc03CSJ3Ece5L6s@tanuki","subject":"Re: [PATCH 00/10] reftable: fix reload with active iterators","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-22T15:15:55Z","receivedAt":"2024-08-22T15:15:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> I can certainly do that. I know that there are conflicts both with the\n> patch series dropping the generic tables and with the patch series that\n> move the reftable unit tests into our own codebase. I did address the\n> former by basing it on top of that series, but didn't yet address the\n> latter.\n>\n> I'm okay with waiting a bit until most of the conflicting topics land. I\n> guess most of them should be close to landing anyway. Alternatively, I\n> can also pull all of them in as dependencies.\n\nAs long as the resolution you see in 'seen' looks acceptable, I'd\nrather prefer you do nothing, than a rebase that _reduces_ the\nnumber of conflicts (but does not completely eliminate them), when I\nalready have a working resolution recorded in my rerere database.\n\nOf course, updates to polish the substance of the topic are very\nmuch appreciated, and I'll redo the resolution in such a case if\nneeded.  That's one of the things the maintainers do.\n\nThanks.\n"},{"id":"501555","messageId":"7qfrzzhsplzmmjdygglfyum7d6qroiggeqonqptkzltgtcrdw5@wlukk42x6thn","threadId":"61975","inReplyTo":"e658b372f046616779cf49c6c9346a49c2ce1485.1724080006.git.ps@pks.im","subject":"Re: [PATCH 03/10] reftable/reader: rename `reftable_new_reader()`","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-08-22T18:51:08Z","receivedAt":"2024-08-22T18:52:04Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/08/19 05:39PM, Patrick Steinhardt wrote:\n> Rename the `reftable_new_reader()` function to `reftable_reader_new()`\n> to match our coding guidelines.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n[snip]  \n> -int init_reader(struct reftable_reader *r, struct reftable_block_source *source,\n> +int reader_init(struct reftable_reader *r, struct reftable_block_source *source,\n>  \t\tconst char *name);\n\nHere we also rename `init_reader()` to `reader_init()`, but do not\nupdate its call sites resulting in build errors. Since we remove this in\nthe next patch anyway, let's drop this change.\n\n-Justin\n"},{"id":"501558","messageId":"xmqqbk1kqsr6.fsf@gitster.g","threadId":"61975","inReplyTo":"7qfrzzhsplzmmjdygglfyum7d6qroiggeqonqptkzltgtcrdw5@wlukk42x6thn","subject":"Re: [PATCH 03/10] reftable/reader: rename `reftable_new_reader()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-22T19:09:33Z","receivedAt":"2024-08-22T19:09:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler <jltobler@gmail.com> writes:\n\n> On 24/08/19 05:39PM, Patrick Steinhardt wrote:\n>> Rename the `reftable_new_reader()` function to `reftable_reader_new()`\n>> to match our coding guidelines.\n>> \n>> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n>> ---\n> [snip]  \n>> -int init_reader(struct reftable_reader *r, struct reftable_block_source *source,\n>> +int reader_init(struct reftable_reader *r, struct reftable_block_source *source,\n>>  \t\tconst char *name);\n>\n> Here we also rename `init_reader()` to `reader_init()`, but do not\n> update its call sites resulting in build errors. Since we remove this in\n> the next patch anyway, let's drop this change.\n\nTrue.  The actual definition of the function is also left\nunmodified.  Let me locally edit the hunk out.  As you pointed out,\nthe next step does mention the \"new\" name in order to remove it, so\nthere needs a cascading adjustment there, but the fallout is fairly\nsmall (see the attached range-diff).\n\nThanks.\n\n 1:  348438a69e =  1:  348438a69e reftable/blocksource: drop malloc block source\n 2:  ed8bb0e345 =  2:  ed8bb0e345 reftable/stack: inline `stack_compact_range_stats()`\n 3:  80cf24d54f !  3:  6b5466771c reftable/reader: rename `reftable_new_reader()`\n    @@ reftable/reader.c: int reftable_reader_print_blocks(const char *tablename)\n      \t\tgoto done;\n      \n     \n    - ## reftable/reader.h ##\n    -@@ reftable/reader.h: struct reftable_reader {\n    - \tstruct reftable_reader_offsets log_offsets;\n    - };\n    - \n    --int init_reader(struct reftable_reader *r, struct reftable_block_source *source,\n    -+int reader_init(struct reftable_reader *r, struct reftable_block_source *source,\n    - \t\tconst char *name);\n    - void reader_close(struct reftable_reader *r);\n    - const char *reader_name(struct reftable_reader *r);\n    -\n      ## reftable/readwrite_test.c ##\n     @@ reftable/readwrite_test.c: static void test_write_empty_table(void)\n      \n 4:  ce93c128f5 !  4:  bc1eeb32ef reftable/reader: inline `init_reader()`\n    @@ reftable/reader.h: struct reftable_reader {\n      \tstruct reftable_reader_offsets log_offsets;\n      };\n      \n    --int reader_init(struct reftable_reader *r, struct reftable_block_source *source,\n    +-int init_reader(struct reftable_reader *r, struct reftable_block_source *source,\n     -\t\tconst char *name);\n      void reader_close(struct reftable_reader *r);\n      const char *reader_name(struct reftable_reader *r);\n 5:  079a3d365e =  5:  3e244b036a reftable/reader: inline `reader_close()`\n 6:  7e951256c6 =  6:  41c537e44b reftable/stack: fix broken refnames in `write_n_ref_tables()`\n 7:  8e99330cff =  7:  b7596c4c32 reftable/reader: introduce refcounting\n 8:  2383c196a6 =  8:  9f28628ecc reftable/reader: keep readers alive during iteration\n 9:  8af7c4485f =  9:  51df76889c reftable/stack: reorder swapping in the reloaded stack contents\n10:  1ccdac05ab = 10:  32bbcc46b9 reftable/stack: fix segfault when reload with reused readers fails\n"},{"id":"501570","messageId":"Zsgcr6MkBbXSQlBA@tanuki","threadId":"61975","inReplyTo":"xmqqbk1kqsr6.fsf@gitster.g","subject":"Re: [PATCH 03/10] reftable/reader: rename `reftable_new_reader()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-23T05:22:55Z","receivedAt":"2024-08-23T05:23:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 22, 2024 at 12:09:33PM -0700, Junio C Hamano wrote:\n> Justin Tobler <jltobler@gmail.com> writes:\n> \n> > On 24/08/19 05:39PM, Patrick Steinhardt wrote:\n> >> Rename the `reftable_new_reader()` function to `reftable_reader_new()`\n> >> to match our coding guidelines.\n> >> \n> >> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> >> ---\n> > [snip]  \n> >> -int init_reader(struct reftable_reader *r, struct reftable_block_source *source,\n> >> +int reader_init(struct reftable_reader *r, struct reftable_block_source *source,\n> >>  \t\tconst char *name);\n> >\n> > Here we also rename `init_reader()` to `reader_init()`, but do not\n> > update its call sites resulting in build errors. Since we remove this in\n> > the next patch anyway, let's drop this change.\n\nWell spotted! I originally wanted to fix that function's name, too. But\nthen I realized it's not even needed in the first place.\n\n> True.  The actual definition of the function is also left\n> unmodified.  Let me locally edit the hunk out.  As you pointed out,\n> the next step does mention the \"new\" name in order to remove it, so\n> there needs a cascading adjustment there, but the fallout is fairly\n> small (see the attached range-diff).\n\nThanks. The range diff looks as expected to me.\n\nPatrick\n"},{"id":"501571","messageId":"ZsgctL0HGnQtAwQf@tanuki","threadId":"61975","inReplyTo":"xmqqmsl4vb9w.fsf@gitster.g","subject":"Re: [PATCH 00/10] reftable: fix reload with active iterators","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-23T05:23:00Z","receivedAt":"2024-08-23T05:23:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 22, 2024 at 08:15:55AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > I can certainly do that. I know that there are conflicts both with the\n> > patch series dropping the generic tables and with the patch series that\n> > move the reftable unit tests into our own codebase. I did address the\n> > former by basing it on top of that series, but didn't yet address the\n> > latter.\n> >\n> > I'm okay with waiting a bit until most of the conflicting topics land. I\n> > guess most of them should be close to landing anyway. Alternatively, I\n> > can also pull all of them in as dependencies.\n> \n> As long as the resolution you see in 'seen' looks acceptable, I'd\n> rather prefer you do nothing, than a rebase that _reduces_ the\n> number of conflicts (but does not completely eliminate them), when I\n> already have a working resolution recorded in my rerere database.\n> \n> Of course, updates to polish the substance of the topic are very\n> much appreciated, and I'll redo the resolution in such a case if\n> needed.  That's one of the things the maintainers do.\n\nYup, the merge in `seen` looks good to me. Thanks!\n\nPatrick\n"},{"id":"501578","messageId":"CAOLa=ZRf9cGyB23EUn_Mb1inJuqNVA85tGGN6SWxcVjbc98QQg@mail.gmail.com","threadId":"61975","inReplyTo":"ZscilS_kLcJyHThx@framework","subject":"Re: [PATCH 07/10] reftable/reader: introduce refcounting","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-08-23T10:14:11Z","receivedAt":"2024-08-23T10:14:13Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Thu, Aug 22, 2024 at 02:47:43AM -0700, karthik nayak wrote:\n>> Patrick Steinhardt <ps@pks.im> writes:\n>>\n>> > It was recently reported that concurrent reads and writes may cause the\n>> > reftable backend to segfault. The root cause of this is that we do not\n>> > properly keep track of reftable readers across reloads.\n>> >\n>> > Suppose that you have a reftable iterator and then decide to reload the\n>> > stack while iterating through the iterator. When the stack has been\n>> > rewritten since we have created the iterator, then we would end up\n>> > discarding a subset of readers that may still be in use by the iterator.\n>> > The consequence is that we now try to reference deallocated memory,\n>> > which of course segfaults.\n>> >\n>> > One way to trigger this is in t5616, where some background maintenance\n>> > jobs have been leaking from one test into another. This leads to stack\n>> > traces like the following one:\n>> >\n>> >   + git -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 --refetch origin\n>> >   AddressSanitizer:DEADLYSIGNAL\n>> >   =================================================================\n>> >   ==657994==ERROR: AddressSanitizer: SEGV on unknown address 0x7fa0f0ec6089 (pc 0x55f23e52ddf9 bp\n>> > 0x7ffe7bfa1700 sp 0x7ffe7bfa1700 T0)\n>> >   ==657994==The signal is caused by a READ memory access.\n>> >       #0 0x55f23e52ddf9 in get_var_int reftable/record.c:29\n>> >       #1 0x55f23e53295e in reftable_decode_keylen reftable/record.c:170\n>> >       #2 0x55f23e532cc0 in reftable_decode_key reftable/record.c:194\n>> >       #3 0x55f23e54e72e in block_iter_next reftable/block.c:398\n>> >       #4 0x55f23e5573dc in table_iter_next_in_block reftable/reader.c:240\n>> >       #5 0x55f23e5573dc in table_iter_next reftable/reader.c:355\n>> >       #6 0x55f23e5573dc in table_iter_next reftable/reader.c:339\n>> >       #7 0x55f23e551283 in merged_iter_advance_subiter reftable/merged.c:69\n>> >       #8 0x55f23e55169e in merged_iter_next_entry reftable/merged.c:123\n>> >       #9 0x55f23e55169e in merged_iter_next_void reftable/merged.c:172\n>> >       #10 0x55f23e537625 in reftable_iterator_next_ref reftable/generic.c:175\n>> >       #11 0x55f23e2cf9c6 in reftable_ref_iterator_advance refs/reftable-backend.c:464\n>> >       #12 0x55f23e2d996e in ref_iterator_advance refs/iterator.c:13\n>> >       #13 0x55f23e2d996e in do_for_each_ref_iterator refs/iterator.c:452\n>> >       #14 0x55f23dca6767 in get_ref_map builtin/fetch.c:623\n>> >       #15 0x55f23dca6767 in do_fetch builtin/fetch.c:1659\n>> >       #16 0x55f23dca6767 in fetch_one builtin/fetch.c:2133\n>> >       #17 0x55f23dca6767 in cmd_fetch builtin/fetch.c:2432\n>> >       #18 0x55f23dba7764 in run_builtin git.c:484\n>> >       #19 0x55f23dba7764 in handle_builtin git.c:741\n>> >       #20 0x55f23dbab61e in run_argv git.c:805\n>> >       #21 0x55f23dbab61e in cmd_main git.c:1000\n>> >       #22 0x55f23dba4781 in main common-main.c:64\n>> >       #23 0x7fa0f063fc89 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n>> >       #24 0x7fa0f063fd44 in __libc_start_main_impl ../csu/libc-start.c:360\n>> >       #25 0x55f23dba6ad0 in _start (git+0xadfad0) (BuildId: 803b2b7f59beb03d7849fb8294a8e2145dd4aa27)\n>> >\n>>\n>> The stacktrace is for iterating over refs, what I don't understand is\n>> where in this flow do we actually reload the stack.\n>\n> Basically, whenever you call into the reftable backend we check whether\n> we need to reload the stack. So, when creating a reftable iterator,\n> reading a single ref, writing refs and so on. So in the above code flow\n> we had a ref iterator, but during iteration we ended up reading other\n> refs, as well.\n>\n\nYes, makes sense. Thanks for clarifying.\n\n>> > While it is somewhat awkward that the maintenance processes survive\n>> > tests in the first place, it is totally expected that reftables should\n>> > work alright with concurrent writers. Seemingly they don't.\n>> >\n>> > The only underlying resource that we need to care about in this context\n>> > is the reftable reader, which is responsible for reading a single table\n>> > from disk. These readers get discarded immediately (unless reused) when\n>> > calling `reftable_stack_reload()`, which is wrong. We can only close\n>> > them once we know that there are no iterators using them anymore.\n>> >\n>> > Prepare for a fix by converting the reftable readers to be refcounted.\n>> >\n>>\n>> Okay so my understanding is that `refcounted` refers to a reference\n>> count which keeps tracks of the stacks which are referring to the\n>> reader. The name is also used in `struct blame_origin` in blame.{c,h}.\n>> Makes a lot more sense now :)\n>\n> Yup.\n>\n>> > diff --git a/reftable/reader.h b/reftable/reader.h\n>> > index 88b4f3b421..3710ee09b4 100644\n>> > --- a/reftable/reader.h\n>> > +++ b/reftable/reader.h\n>> > @@ -50,6 +50,8 @@ struct reftable_reader {\n>> >  \tstruct reftable_reader_offsets ref_offsets;\n>> >  \tstruct reftable_reader_offsets obj_offsets;\n>> >  \tstruct reftable_reader_offsets log_offsets;\n>> > +\n>> > +\tuint64_t refcount;\n>>\n>> Wonder if there is a chance that we decrement refcount from 0 and hence\n>> cause a wraparound.\n>\n> This should never happen in practice. And if it does, we would hit a\n> BUG():\n>\n>     void reftable_reader_decref(struct reftable_reader *r)\n>     {\n>     \tif (!r)\n>     \t\treturn;\n>     \tif (!r->refcount)\n>     \t\tBUG(\"cannot decrement ref counter of dead reader\");\n>     \tif (--r->refcount)\n>     \t\treturn;\n>     \tblock_source_close(&r->source);\n>     \tFREE_AND_NULL(r->name);\n>     \treftable_free(r);\n>     }\n>\n> If the refcount is at zero already, we hit the bug.\n>\n> Patrick\n\nYup, this seems correct. Thanks.\n"},{"id":"501579","messageId":"CAOLa=ZQarx37De=xmD73D3dPzxp0j8pfb-TBc4u6nKkJn_TT1g@mail.gmail.com","threadId":"61975","inReplyTo":"026820562882afb31d7224c90722e09bef835340.1724080006.git.ps@pks.im","subject":"Re: [PATCH 08/10] reftable/reader: keep readers alive during iteration","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-08-23T10:21:27Z","receivedAt":"2024-08-23T10:21:29Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The lifetime of a table iterator may surive the lifetime of a reader\n\ns/surive/survive\n\n> when the stack gets reloaded. Keep the reader from being released by\n> increasing its refcount while the iterator is still being used.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  reftable/reader.c     |  2 ++\n>  reftable/stack_test.c | 49 +++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 51 insertions(+)\n>\n> diff --git a/reftable/reader.c b/reftable/reader.c\n> index 64a0953e68..f877099087 100644\n> --- a/reftable/reader.c\n> +++ b/reftable/reader.c\n> @@ -175,6 +175,7 @@ static int table_iter_init(struct table_iter *ti, struct reftable_reader *r)\n>  {\n>  \tstruct block_iter bi = BLOCK_ITER_INIT;\n>  \tmemset(ti, 0, sizeof(*ti));\n> +\treftable_reader_incref(r);\n>  \tti->r = r;\n>  \tti->bi = bi;\n>  \treturn 0;\n> @@ -262,6 +263,7 @@ static void table_iter_close(struct table_iter *ti)\n>  {\n>  \ttable_iter_block_done(ti);\n>  \tblock_iter_close(&ti->bi);\n> +\treftable_reader_decref(ti->r);\n>  }\n>\n>  static int table_iter_next_block(struct table_iter *ti)\n> diff --git a/reftable/stack_test.c b/reftable/stack_test.c\n> index bc3bf77274..91e716dc0a 100644\n> --- a/reftable/stack_test.c\n> +++ b/reftable/stack_test.c\n> @@ -1076,6 +1076,54 @@ static void test_reftable_stack_compaction_concurrent_clean(void)\n>  \tclear_dir(dir);\n>  }\n>\n> +static void test_reftable_stack_read_across_reload(void)\n> +{\n> +\tstruct reftable_write_options opts = { 0 };\n> +\tstruct reftable_stack *st1 = NULL, *st2 = NULL;\n> +\tstruct reftable_ref_record rec = { 0 };\n> +\tstruct reftable_iterator it = { 0 };\n> +\tchar *dir = get_tmp_dir(__LINE__);\n> +\tint err;\n> +\n> +\t/* Create a first stack and set up an iterator for it. */\n> +\terr = reftable_new_stack(&st1, dir, &opts);\n> +\tEXPECT_ERR(err);\n> +\twrite_n_ref_tables(st1, 2);\n> +\tEXPECT(st1->merged->readers_len == 2);\n> +\treftable_stack_init_ref_iterator(st1, &it);\n> +\terr = reftable_iterator_seek_ref(&it, \"\");\n> +\tEXPECT_ERR(err);\n> +\n> +\t/* Set up a second stack for the same directory and compact it. */\n> +\terr = reftable_new_stack(&st2, dir, &opts);\n> +\tEXPECT_ERR(err);\n> +\tEXPECT(st2->merged->readers_len == 2);\n> +\terr = reftable_stack_compact_all(st2, NULL);\n\nShouldn't we verify that `EXPECT(st2->merged->readers_len == 1);` here?\n\n> +\tEXPECT_ERR(err);\n> +\n> +\t/*\n> +\t * Verify that we can continue to use the old iterator even after we\n> +\t * have reloaded its stack.\n> +\t */\n> +\terr = reftable_stack_reload(st1);\n> +\tEXPECT_ERR(err);\n> +\tEXPECT(st2->merged->readers_len == 1);\n\nOh we do it here, I would've expected it above the `st1` reload. And\nprobably expected `EXPECT(st1->merged->readers_len == 2);` here to\nconfirm that we still have the readers.\n\n> +\terr = reftable_iterator_next_ref(&it, &rec);\n> +\tEXPECT_ERR(err);\n> +\tEXPECT(!strcmp(rec.refname, \"refs/heads/branch-0000\"));\n> +\terr = reftable_iterator_next_ref(&it, &rec);\n> +\tEXPECT_ERR(err);\n> +\tEXPECT(!strcmp(rec.refname, \"refs/heads/branch-0001\"));\n> +\terr = reftable_iterator_next_ref(&it, &rec);\n> +\tEXPECT(err > 0);\n> +\n> +\treftable_ref_record_release(&rec);\n> +\treftable_iterator_destroy(&it);\n> +\treftable_stack_destroy(st1);\n> +\treftable_stack_destroy(st2);\n> +\tclear_dir(dir);\n> +}\n> +\n>  int stack_test_main(int argc, const char *argv[])\n>  {\n>  \tRUN_TEST(test_empty_add);\n> @@ -1098,6 +1146,7 @@ int stack_test_main(int argc, const char *argv[])\n>  \tRUN_TEST(test_reftable_stack_auto_compaction_fails_gracefully);\n>  \tRUN_TEST(test_reftable_stack_update_index_check);\n>  \tRUN_TEST(test_reftable_stack_uptodate);\n> +\tRUN_TEST(test_reftable_stack_read_across_reload);\n>  \tRUN_TEST(test_suggest_compaction_segment);\n>  \tRUN_TEST(test_suggest_compaction_segment_nothing);\n>  \treturn 0;\n> --\n> 2.46.0.164.g477ce5ccd6.dirty\n"},{"id":"501596","messageId":"ZsiRzzRqI-VDu0-S@tanuki","threadId":"61975","inReplyTo":"CAOLa=ZQarx37De=xmD73D3dPzxp0j8pfb-TBc4u6nKkJn_TT1g@mail.gmail.com","subject":"Re: [PATCH 08/10] reftable/reader: keep readers alive during iteration","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-23T13:42:45Z","receivedAt":"2024-08-23T13:42:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 23, 2024 at 03:21:27AM -0700, karthik nayak wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> > diff --git a/reftable/stack_test.c b/reftable/stack_test.c\n> > index bc3bf77274..91e716dc0a 100644\n> > --- a/reftable/stack_test.c\n> > +++ b/reftable/stack_test.c\n> > @@ -1076,6 +1076,54 @@ static void test_reftable_stack_compaction_concurrent_clean(void)\n> >  \tclear_dir(dir);\n> >  }\n> >\n> > +static void test_reftable_stack_read_across_reload(void)\n> > +{\n> > +\tstruct reftable_write_options opts = { 0 };\n> > +\tstruct reftable_stack *st1 = NULL, *st2 = NULL;\n> > +\tstruct reftable_ref_record rec = { 0 };\n> > +\tstruct reftable_iterator it = { 0 };\n> > +\tchar *dir = get_tmp_dir(__LINE__);\n> > +\tint err;\n> > +\n> > +\t/* Create a first stack and set up an iterator for it. */\n> > +\terr = reftable_new_stack(&st1, dir, &opts);\n> > +\tEXPECT_ERR(err);\n> > +\twrite_n_ref_tables(st1, 2);\n> > +\tEXPECT(st1->merged->readers_len == 2);\n> > +\treftable_stack_init_ref_iterator(st1, &it);\n> > +\terr = reftable_iterator_seek_ref(&it, \"\");\n> > +\tEXPECT_ERR(err);\n> > +\n> > +\t/* Set up a second stack for the same directory and compact it. */\n> > +\terr = reftable_new_stack(&st2, dir, &opts);\n> > +\tEXPECT_ERR(err);\n> > +\tEXPECT(st2->merged->readers_len == 2);\n> > +\terr = reftable_stack_compact_all(st2, NULL);\n> \n> Shouldn't we verify that `EXPECT(st2->merged->readers_len == 1);` here?\n\nYes, we should.\n\n> > +\tEXPECT_ERR(err);\n> > +\n> > +\t/*\n> > +\t * Verify that we can continue to use the old iterator even after we\n> > +\t * have reloaded its stack.\n> > +\t */\n> > +\terr = reftable_stack_reload(st1);\n> > +\tEXPECT_ERR(err);\n> > +\tEXPECT(st2->merged->readers_len == 1);\n> \n> Oh we do it here, I would've expected it above the `st1` reload. And\n> probably expected `EXPECT(st1->merged->readers_len == 2);` here to\n> confirm that we still have the readers.\n\nAnd yes, this was a typo, it should've been `st1` indeed. Good eyes,\nthanks!\n\nPatrick\n"},{"id":"501597","messageId":"cover.1724420744.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724080006.git.ps@pks.im","subject":"[PATCH v2 00/10] reftable: fix reload with active iterators","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-23T14:12:26Z","receivedAt":"2024-08-23T14:12:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that fixes issues in the\nreftable libary caused by having concurrent readers and writers.\n\nChanges compared to v1:\n\n  - Remove a spurious change that renamed `init_reader()` to\n    `reader_init()`. That function went away in the subsequent commit\n    anyway.\n\n  - Fix a typo in a commit message.\n\n  - Fix some assertions in one of the added tests.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (10):\n  reftable/blocksource: drop malloc block source\n  reftable/stack: inline `stack_compact_range_stats()`\n  reftable/reader: rename `reftable_new_reader()`\n  reftable/reader: inline `init_reader()`\n  reftable/reader: inline `reader_close()`\n  reftable/stack: fix broken refnames in `write_n_ref_tables()`\n  reftable/reader: introduce refcounting\n  reftable/reader: keep readers alive during iteration\n  reftable/stack: reorder swapping in the reloaded stack contents\n  reftable/stack: fix segfault when reload with reused readers fails\n\n reftable/block_test.c            |   3 +-\n reftable/blocksource.c           |  20 -----\n reftable/blocksource.h           |   2 -\n reftable/reader.c                | 149 ++++++++++++++++---------------\n reftable/reader.h                |   5 +-\n reftable/readwrite_test.c        |  85 +++++++++---------\n reftable/reftable-reader.h       |  19 ++--\n reftable/stack.c                 |  90 +++++++++++--------\n reftable/stack_test.c            | 116 +++++++++++++++++++++++-\n t/helper/test-reftable.c         |   4 +-\n t/unit-tests/t-reftable-merged.c |  10 +--\n 11 files changed, 312 insertions(+), 191 deletions(-)\n\nRange-diff against v1:\n 1:  fee3d3523eb =  1:  fee3d3523eb reftable/blocksource: drop malloc block source\n 2:  3c0cf2bf46f =  2:  3c0cf2bf46f reftable/stack: inline `stack_compact_range_stats()`\n 3:  e658b372f04 !  3:  b4cf97bf758 reftable/reader: rename `reftable_new_reader()`\n    @@ reftable/reader.c: int reftable_reader_print_blocks(const char *tablename)\n      \t\tgoto done;\n      \n     \n    - ## reftable/reader.h ##\n    -@@ reftable/reader.h: struct reftable_reader {\n    - \tstruct reftable_reader_offsets log_offsets;\n    - };\n    - \n    --int init_reader(struct reftable_reader *r, struct reftable_block_source *source,\n    -+int reader_init(struct reftable_reader *r, struct reftable_block_source *source,\n    - \t\tconst char *name);\n    - void reader_close(struct reftable_reader *r);\n    - const char *reader_name(struct reftable_reader *r);\n    -\n      ## reftable/readwrite_test.c ##\n     @@ reftable/readwrite_test.c: static void test_write_empty_table(void)\n      \n 4:  f628b7dafb9 !  4:  3b667097501 reftable/reader: inline `init_reader()`\n    @@ reftable/reader.h: struct reftable_reader {\n      \tstruct reftable_reader_offsets log_offsets;\n      };\n      \n    --int reader_init(struct reftable_reader *r, struct reftable_block_source *source,\n    +-int init_reader(struct reftable_reader *r, struct reftable_block_source *source,\n     -\t\tconst char *name);\n      void reader_close(struct reftable_reader *r);\n      const char *reader_name(struct reftable_reader *r);\n 5:  4a9fe150427 =  5:  b129d8a8687 reftable/reader: inline `reader_close()`\n 6:  4965402e7bf =  6:  e3b28709b5f reftable/stack: fix broken refnames in `write_n_ref_tables()`\n 7:  fc0ed68d467 =  7:  6535d1ca9de reftable/reader: introduce refcounting\n 8:  02682056288 !  8:  8d08c3bc515 reftable/reader: keep readers alive during iteration\n    @@ Metadata\n      ## Commit message ##\n         reftable/reader: keep readers alive during iteration\n     \n    -    The lifetime of a table iterator may surive the lifetime of a reader\n    +    The lifetime of a table iterator may survive the lifetime of a reader\n         when the stack gets reloaded. Keep the reader from being released by\n         increasing its refcount while the iterator is still being used.\n     \n    @@ reftable/stack_test.c: static void test_reftable_stack_compaction_concurrent_cle\n     +\tEXPECT(st2->merged->readers_len == 2);\n     +\terr = reftable_stack_compact_all(st2, NULL);\n     +\tEXPECT_ERR(err);\n    ++\tEXPECT(st2->merged->readers_len == 1);\n     +\n     +\t/*\n     +\t * Verify that we can continue to use the old iterator even after we\n    @@ reftable/stack_test.c: static void test_reftable_stack_compaction_concurrent_cle\n     +\t */\n     +\terr = reftable_stack_reload(st1);\n     +\tEXPECT_ERR(err);\n    -+\tEXPECT(st2->merged->readers_len == 1);\n    ++\tEXPECT(st1->merged->readers_len == 1);\n     +\terr = reftable_iterator_next_ref(&it, &rec);\n     +\tEXPECT_ERR(err);\n     +\tEXPECT(!strcmp(rec.refname, \"refs/heads/branch-0000\"));\n 9:  d98316fbf4c =  9:  5aee91de25e reftable/stack: reorder swapping in the reloaded stack contents\n10:  b777818ea99 = 10:  4a8d45cc9b4 reftable/stack: fix segfault when reload with reused readers fails\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501598","messageId":"fee3d3523eb867c6b45f4c133fb3b001be1ddd63.1724420744.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724420744.git.ps@pks.im","subject":"[PATCH v2 01/10] reftable/blocksource: drop malloc block source","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-23T14:12:29Z","receivedAt":"2024-08-23T14:12:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reftable blocksource provides a generic interface to read blocks via\ndifferent sources, e.g. from disk or from memory. One of the block\nsources is the malloc block source, which can in theory read data from\nmemory. We nowadays also have a strbuf block source though, which\nprovides essentially the same functionality with better ergonomics.\n\nAdapt the only remaining user of the malloc block source in our tests\nto use the strbuf block source, instead, and remove the now-unused\nmalloc block source.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/block_test.c  |  3 ++-\n reftable/blocksource.c | 20 --------------------\n reftable/blocksource.h |  2 --\n 3 files changed, 2 insertions(+), 23 deletions(-)\n\ndiff --git a/reftable/block_test.c b/reftable/block_test.c\nindex 90aecd5a7c6..de8f426a429 100644\n--- a/reftable/block_test.c\n+++ b/reftable/block_test.c\n@@ -34,11 +34,12 @@ static void test_block_read_write(void)\n \tstruct block_reader br = { 0 };\n \tstruct block_iter it = BLOCK_ITER_INIT;\n \tint j = 0;\n+\tstruct strbuf block_data = STRBUF_INIT;\n \tstruct strbuf want = STRBUF_INIT;\n \n \tREFTABLE_CALLOC_ARRAY(block.data, block_size);\n \tblock.len = block_size;\n-\tblock.source = malloc_block_source();\n+\tblock_source_from_strbuf(&block.source, &block_data);\n \tblock_writer_init(&bw, BLOCK_TYPE_REF, block.data, block_size,\n \t\t\t  header_off, hash_size(GIT_SHA1_FORMAT_ID));\n \ndiff --git a/reftable/blocksource.c b/reftable/blocksource.c\nindex eeed254ba9c..1774853011d 100644\n--- a/reftable/blocksource.c\n+++ b/reftable/blocksource.c\n@@ -55,26 +55,6 @@ void block_source_from_strbuf(struct reftable_block_source *bs,\n \tbs->arg = buf;\n }\n \n-static void malloc_return_block(void *b, struct reftable_block *dest)\n-{\n-\tif (dest->len)\n-\t\tmemset(dest->data, 0xff, dest->len);\n-\treftable_free(dest->data);\n-}\n-\n-static struct reftable_block_source_vtable malloc_vtable = {\n-\t.return_block = &malloc_return_block,\n-};\n-\n-static struct reftable_block_source malloc_block_source_instance = {\n-\t.ops = &malloc_vtable,\n-};\n-\n-struct reftable_block_source malloc_block_source(void)\n-{\n-\treturn malloc_block_source_instance;\n-}\n-\n struct file_block_source {\n \tuint64_t size;\n \tunsigned char *data;\ndiff --git a/reftable/blocksource.h b/reftable/blocksource.h\nindex 072e2727ad2..659a27b4063 100644\n--- a/reftable/blocksource.h\n+++ b/reftable/blocksource.h\n@@ -17,6 +17,4 @@ struct reftable_block_source;\n void block_source_from_strbuf(struct reftable_block_source *bs,\n \t\t\t      struct strbuf *buf);\n \n-struct reftable_block_source malloc_block_source(void);\n-\n #endif\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501599","messageId":"3c0cf2bf46f4106d7c0b30c863946262639b2351.1724420744.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724420744.git.ps@pks.im","subject":"[PATCH v2 02/10] reftable/stack: inline `stack_compact_range_stats()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-23T14:12:32Z","receivedAt":"2024-08-23T14:12:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The only difference between `stack_compact_range_stats()` and\n`stack_compact_range()` is that the former updates stats on failure,\nwhereas the latter doesn't. There are no callers anymore that do not\nwant their stats updated though, making the indirection unnecessary.\n\nInline the stat updates into `stack_compact_range()`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 16 ++++------------\n 1 file changed, 4 insertions(+), 12 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex d3a95d2f1d7..891ea971b72 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -1328,17 +1328,9 @@ static int stack_compact_range(struct reftable_stack *st,\n \tstrbuf_release(&table_name);\n \tfree_names(names);\n \n-\treturn err;\n-}\n-\n-static int stack_compact_range_stats(struct reftable_stack *st,\n-\t\t\t\t     size_t first, size_t last,\n-\t\t\t\t     struct reftable_log_expiry_config *config,\n-\t\t\t\t     unsigned int flags)\n-{\n-\tint err = stack_compact_range(st, first, last, config, flags);\n \tif (err == REFTABLE_LOCK_ERROR)\n \t\tst->stats.failures++;\n+\n \treturn err;\n }\n \n@@ -1346,7 +1338,7 @@ int reftable_stack_compact_all(struct reftable_stack *st,\n \t\t\t       struct reftable_log_expiry_config *config)\n {\n \tsize_t last = st->merged->readers_len ? st->merged->readers_len - 1 : 0;\n-\treturn stack_compact_range_stats(st, 0, last, config, 0);\n+\treturn stack_compact_range(st, 0, last, config, 0);\n }\n \n static int segment_size(struct segment *s)\n@@ -1452,8 +1444,8 @@ int reftable_stack_auto_compact(struct reftable_stack *st)\n \t\t\t\t\t   st->opts.auto_compaction_factor);\n \treftable_free(sizes);\n \tif (segment_size(&seg) > 0)\n-\t\treturn stack_compact_range_stats(st, seg.start, seg.end - 1,\n-\t\t\t\t\t\t NULL, STACK_COMPACT_RANGE_BEST_EFFORT);\n+\t\treturn stack_compact_range(st, seg.start, seg.end - 1,\n+\t\t\t\t\t   NULL, STACK_COMPACT_RANGE_BEST_EFFORT);\n \n \treturn 0;\n }\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501600","messageId":"b4cf97bf7580fff022bf4a1463071ca9b039510f.1724420744.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724420744.git.ps@pks.im","subject":"[PATCH v2 03/10] reftable/reader: rename `reftable_new_reader()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-23T14:12:34Z","receivedAt":"2024-08-23T14:12:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Rename the `reftable_new_reader()` function to `reftable_reader_new()`\nto match our coding guidelines.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/reader.c                | 4 ++--\n reftable/readwrite_test.c        | 6 +++---\n reftable/reftable-reader.h       | 4 ++--\n reftable/stack.c                 | 4 ++--\n t/helper/test-reftable.c         | 2 +-\n t/unit-tests/t-reftable-merged.c | 6 +++---\n 6 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 082cf00b606..ea4fdea90b6 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -637,7 +637,7 @@ void reader_close(struct reftable_reader *r)\n \tFREE_AND_NULL(r->name);\n }\n \n-int reftable_new_reader(struct reftable_reader **p,\n+int reftable_reader_new(struct reftable_reader **p,\n \t\t\tstruct reftable_block_source *src, char const *name)\n {\n \tstruct reftable_reader *rd = reftable_calloc(1, sizeof(*rd));\n@@ -786,7 +786,7 @@ int reftable_reader_print_blocks(const char *tablename)\n \tif (err < 0)\n \t\tgoto done;\n \n-\terr = reftable_new_reader(&r, &src, tablename);\n+\terr = reftable_reader_new(&r, &src, tablename);\n \tif (err < 0)\n \t\tgoto done;\n \ndiff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c\nindex f411abfe9cb..101cdb5cd66 100644\n--- a/reftable/readwrite_test.c\n+++ b/reftable/readwrite_test.c\n@@ -646,7 +646,7 @@ static void test_write_empty_table(void)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = reftable_new_reader(&rd, &source, \"filename\");\n+\terr = reftable_reader_new(&rd, &source, \"filename\");\n \tEXPECT_ERR(err);\n \n \treftable_reader_init_ref_iterator(rd, &it);\n@@ -850,7 +850,7 @@ static void test_write_multiple_indices(void)\n \tEXPECT(stats->log_stats.index_offset > 0);\n \n \tblock_source_from_strbuf(&source, &writer_buf);\n-\terr = reftable_new_reader(&reader, &source, \"filename\");\n+\terr = reftable_reader_new(&reader, &source, \"filename\");\n \tEXPECT_ERR(err);\n \n \t/*\n@@ -907,7 +907,7 @@ static void test_write_multi_level_index(void)\n \tEXPECT(stats->ref_stats.max_index_level == 2);\n \n \tblock_source_from_strbuf(&source, &writer_buf);\n-\terr = reftable_new_reader(&reader, &source, \"filename\");\n+\terr = reftable_reader_new(&reader, &source, \"filename\");\n \tEXPECT_ERR(err);\n \n \t/*\ndiff --git a/reftable/reftable-reader.h b/reftable/reftable-reader.h\nindex 69621c5b0fc..8a05c846858 100644\n--- a/reftable/reftable-reader.h\n+++ b/reftable/reftable-reader.h\n@@ -23,14 +23,14 @@\n /* The reader struct is a handle to an open reftable file. */\n struct reftable_reader;\n \n-/* reftable_new_reader opens a reftable for reading. If successful,\n+/* reftable_reader_new opens a reftable for reading. If successful,\n  * returns 0 code and sets pp. The name is used for creating a\n  * stack. Typically, it is the basename of the file. The block source\n  * `src` is owned by the reader, and is closed on calling\n  * reftable_reader_destroy(). On error, the block source `src` is\n  * closed as well.\n  */\n-int reftable_new_reader(struct reftable_reader **pp,\n+int reftable_reader_new(struct reftable_reader **pp,\n \t\t\tstruct reftable_block_source *src, const char *name);\n \n /* Initialize a reftable iterator for reading refs. */\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 891ea971b72..c72435b0596 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -258,7 +258,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t\t\tif (err < 0)\n \t\t\t\tgoto done;\n \n-\t\t\terr = reftable_new_reader(&rd, &src, name);\n+\t\t\terr = reftable_reader_new(&rd, &src, name);\n \t\t\tif (err < 0)\n \t\t\t\tgoto done;\n \t\t}\n@@ -1532,7 +1532,7 @@ static void remove_maybe_stale_table(struct reftable_stack *st, uint64_t max,\n \tif (err < 0)\n \t\tgoto done;\n \n-\terr = reftable_new_reader(&rd, &src, name);\n+\terr = reftable_reader_new(&rd, &src, name);\n \tif (err < 0)\n \t\tgoto done;\n \ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex c1942156b50..87c2f42aaed 100644\n--- a/t/helper/test-reftable.c\n+++ b/t/helper/test-reftable.c\n@@ -139,7 +139,7 @@ static int dump_reftable(const char *tablename)\n \tif (err < 0)\n \t\tgoto done;\n \n-\terr = reftable_new_reader(&r, &src, tablename);\n+\terr = reftable_reader_new(&r, &src, tablename);\n \tif (err < 0)\n \t\tgoto done;\n \ndiff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c\nindex 93345c6c8be..8f51aca1b62 100644\n--- a/t/unit-tests/t-reftable-merged.c\n+++ b/t/unit-tests/t-reftable-merged.c\n@@ -102,7 +102,7 @@ merged_table_from_records(struct reftable_ref_record **refs,\n \t\twrite_test_table(&buf[i], refs[i], sizes[i]);\n \t\tblock_source_from_strbuf(&(*source)[i], &buf[i]);\n \n-\t\terr = reftable_new_reader(&(*readers)[i], &(*source)[i],\n+\t\terr = reftable_reader_new(&(*readers)[i], &(*source)[i],\n \t\t\t\t\t  \"name\");\n \t\tcheck(!err);\n \t}\n@@ -277,7 +277,7 @@ merged_table_from_log_records(struct reftable_log_record **logs,\n \t\twrite_test_log_table(&buf[i], logs[i], sizes[i], i + 1);\n \t\tblock_source_from_strbuf(&(*source)[i], &buf[i]);\n \n-\t\terr = reftable_new_reader(&(*readers)[i], &(*source)[i],\n+\t\terr = reftable_reader_new(&(*readers)[i], &(*source)[i],\n \t\t\t\t\t  \"name\");\n \t\tcheck(!err);\n \t}\n@@ -426,7 +426,7 @@ static void t_default_write_opts(void)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = reftable_new_reader(&rd, &source, \"filename\");\n+\terr = reftable_reader_new(&rd, &source, \"filename\");\n \tcheck(!err);\n \n \thash_id = reftable_reader_hash_id(rd);\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501601","messageId":"3b6670975011ea5126c6584be48500b9fc241ab0.1724420744.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724420744.git.ps@pks.im","subject":"[PATCH v2 04/10] reftable/reader: inline `init_reader()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-23T14:12:37Z","receivedAt":"2024-08-23T14:12:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Most users use an allocated version of the `reftable_reader`, except for\nsome tests. We are about to convert the reader to become refcounted\nthough, and providing the ability to keep a reader on the stack makes\nthis conversion harder than necessary.\n\nUpdate the tests to use `reftable_reader_new()` instead to prepare for\nthis change.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/reader.c         | 122 +++++++++++++++++++-------------------\n reftable/reader.h         |   2 -\n reftable/readwrite_test.c |  73 ++++++++++++-----------\n 3 files changed, 98 insertions(+), 99 deletions(-)\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex ea4fdea90b6..9239679ad95 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -162,58 +162,6 @@ static int parse_footer(struct reftable_reader *r, uint8_t *footer,\n \treturn err;\n }\n \n-int init_reader(struct reftable_reader *r, struct reftable_block_source *source,\n-\t\tconst char *name)\n-{\n-\tstruct reftable_block footer = { NULL };\n-\tstruct reftable_block header = { NULL };\n-\tint err = 0;\n-\tuint64_t file_size = block_source_size(source);\n-\n-\t/* Need +1 to read type of first block. */\n-\tuint32_t read_size = header_size(2) + 1; /* read v2 because it's larger.  */\n-\tmemset(r, 0, sizeof(struct reftable_reader));\n-\n-\tif (read_size > file_size) {\n-\t\terr = REFTABLE_FORMAT_ERROR;\n-\t\tgoto done;\n-\t}\n-\n-\terr = block_source_read_block(source, &header, 0, read_size);\n-\tif (err != read_size) {\n-\t\terr = REFTABLE_IO_ERROR;\n-\t\tgoto done;\n-\t}\n-\n-\tif (memcmp(header.data, \"REFT\", 4)) {\n-\t\terr = REFTABLE_FORMAT_ERROR;\n-\t\tgoto done;\n-\t}\n-\tr->version = header.data[4];\n-\tif (r->version != 1 && r->version != 2) {\n-\t\terr = REFTABLE_FORMAT_ERROR;\n-\t\tgoto done;\n-\t}\n-\n-\tr->size = file_size - footer_size(r->version);\n-\tr->source = *source;\n-\tr->name = xstrdup(name);\n-\tr->hash_id = 0;\n-\n-\terr = block_source_read_block(source, &footer, r->size,\n-\t\t\t\t      footer_size(r->version));\n-\tif (err != footer_size(r->version)) {\n-\t\terr = REFTABLE_IO_ERROR;\n-\t\tgoto done;\n-\t}\n-\n-\terr = parse_footer(r, footer.data, header.data);\n-done:\n-\treftable_block_done(&footer);\n-\treftable_block_done(&header);\n-\treturn err;\n-}\n-\n struct table_iter {\n \tstruct reftable_reader *r;\n \tuint8_t typ;\n@@ -637,16 +585,68 @@ void reader_close(struct reftable_reader *r)\n \tFREE_AND_NULL(r->name);\n }\n \n-int reftable_reader_new(struct reftable_reader **p,\n-\t\t\tstruct reftable_block_source *src, char const *name)\n+int reftable_reader_new(struct reftable_reader **out,\n+\t\t\tstruct reftable_block_source *source, char const *name)\n {\n-\tstruct reftable_reader *rd = reftable_calloc(1, sizeof(*rd));\n-\tint err = init_reader(rd, src, name);\n-\tif (err == 0) {\n-\t\t*p = rd;\n-\t} else {\n-\t\tblock_source_close(src);\n-\t\treftable_free(rd);\n+\tstruct reftable_block footer = { 0 };\n+\tstruct reftable_block header = { 0 };\n+\tstruct reftable_reader *r;\n+\tuint64_t file_size = block_source_size(source);\n+\tuint32_t read_size;\n+\tint err;\n+\n+\tREFTABLE_CALLOC_ARRAY(r, 1);\n+\n+\t/*\n+\t * We need one extra byte to read the type of first block. We also\n+\t * pretend to always be reading v2 of the format because it is larger.\n+\t */\n+\tread_size = header_size(2) + 1;\n+\tif (read_size > file_size) {\n+\t\terr = REFTABLE_FORMAT_ERROR;\n+\t\tgoto done;\n+\t}\n+\n+\terr = block_source_read_block(source, &header, 0, read_size);\n+\tif (err != read_size) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n+\t}\n+\n+\tif (memcmp(header.data, \"REFT\", 4)) {\n+\t\terr = REFTABLE_FORMAT_ERROR;\n+\t\tgoto done;\n+\t}\n+\tr->version = header.data[4];\n+\tif (r->version != 1 && r->version != 2) {\n+\t\terr = REFTABLE_FORMAT_ERROR;\n+\t\tgoto done;\n+\t}\n+\n+\tr->size = file_size - footer_size(r->version);\n+\tr->source = *source;\n+\tr->name = xstrdup(name);\n+\tr->hash_id = 0;\n+\n+\terr = block_source_read_block(source, &footer, r->size,\n+\t\t\t\t      footer_size(r->version));\n+\tif (err != footer_size(r->version)) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n+\t}\n+\n+\terr = parse_footer(r, footer.data, header.data);\n+\tif (err)\n+\t\tgoto done;\n+\n+\t*out = r;\n+\n+done:\n+\treftable_block_done(&footer);\n+\treftable_block_done(&header);\n+\tif (err) {\n+\t\treftable_free(r);\n+\t\tblock_source_close(source);\n \t}\n \treturn err;\n }\ndiff --git a/reftable/reader.h b/reftable/reader.h\nindex a2c204d523c..762cd6de667 100644\n--- a/reftable/reader.h\n+++ b/reftable/reader.h\n@@ -52,8 +52,6 @@ struct reftable_reader {\n \tstruct reftable_reader_offsets log_offsets;\n };\n \n-int init_reader(struct reftable_reader *r, struct reftable_block_source *source,\n-\t\tconst char *name);\n void reader_close(struct reftable_reader *r);\n const char *reader_name(struct reftable_reader *r);\n \ndiff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c\nindex 101cdb5cd66..2f2ff787b26 100644\n--- a/reftable/readwrite_test.c\n+++ b/reftable/readwrite_test.c\n@@ -195,7 +195,7 @@ static void test_log_write_read(void)\n \tstruct reftable_log_record log = { NULL };\n \tint n;\n \tstruct reftable_iterator it = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { NULL };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n@@ -236,10 +236,10 @@ static void test_log_write_read(void)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = init_reader(&rd, &source, \"file.log\");\n+\terr = reftable_reader_new(&reader, &source, \"file.log\");\n \tEXPECT_ERR(err);\n \n-\treftable_reader_init_ref_iterator(&rd, &it);\n+\treftable_reader_init_ref_iterator(reader, &it);\n \n \terr = reftable_iterator_seek_ref(&it, names[N - 1]);\n \tEXPECT_ERR(err);\n@@ -254,7 +254,7 @@ static void test_log_write_read(void)\n \treftable_iterator_destroy(&it);\n \treftable_ref_record_release(&ref);\n \n-\treftable_reader_init_log_iterator(&rd, &it);\n+\treftable_reader_init_log_iterator(reader, &it);\n \n \terr = reftable_iterator_seek_log(&it, \"\");\n \tEXPECT_ERR(err);\n@@ -279,7 +279,7 @@ static void test_log_write_read(void)\n \t/* cleanup. */\n \tstrbuf_release(&buf);\n \tfree_names(names);\n-\treader_close(&rd);\n+\treftable_reader_free(reader);\n }\n \n static void test_log_zlib_corruption(void)\n@@ -288,7 +288,7 @@ static void test_log_zlib_corruption(void)\n \t\t.block_size = 256,\n \t};\n \tstruct reftable_iterator it = { 0 };\n-\tstruct reftable_reader rd = { 0 };\n+\tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n@@ -331,18 +331,18 @@ static void test_log_zlib_corruption(void)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = init_reader(&rd, &source, \"file.log\");\n+\terr = reftable_reader_new(&reader, &source, \"file.log\");\n \tEXPECT_ERR(err);\n \n-\treftable_reader_init_log_iterator(&rd, &it);\n+\treftable_reader_init_log_iterator(reader, &it);\n \terr = reftable_iterator_seek_log(&it, \"refname\");\n \tEXPECT(err == REFTABLE_ZLIB_ERROR);\n \n \treftable_iterator_destroy(&it);\n \n \t/* cleanup. */\n+\treftable_reader_free(reader);\n \tstrbuf_release(&buf);\n-\treader_close(&rd);\n }\n \n static void test_table_read_write_sequential(void)\n@@ -352,7 +352,7 @@ static void test_table_read_write_sequential(void)\n \tint N = 50;\n \tstruct reftable_iterator it = { NULL };\n \tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_reader *reader;\n \tint err = 0;\n \tint j = 0;\n \n@@ -360,10 +360,10 @@ static void test_table_read_write_sequential(void)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = init_reader(&rd, &source, \"file.ref\");\n+\terr = reftable_reader_new(&reader, &source, \"file.ref\");\n \tEXPECT_ERR(err);\n \n-\treftable_reader_init_ref_iterator(&rd, &it);\n+\treftable_reader_init_ref_iterator(reader, &it);\n \terr = reftable_iterator_seek_ref(&it, \"\");\n \tEXPECT_ERR(err);\n \n@@ -381,11 +381,11 @@ static void test_table_read_write_sequential(void)\n \t\treftable_ref_record_release(&ref);\n \t}\n \tEXPECT(j == N);\n+\n \treftable_iterator_destroy(&it);\n+\treftable_reader_free(reader);\n \tstrbuf_release(&buf);\n \tfree_names(names);\n-\n-\treader_close(&rd);\n }\n \n static void test_table_write_small_table(void)\n@@ -404,7 +404,7 @@ static void test_table_read_api(void)\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 50;\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { NULL };\n \tint err;\n \tint i;\n@@ -415,10 +415,10 @@ static void test_table_read_api(void)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = init_reader(&rd, &source, \"file.ref\");\n+\terr = reftable_reader_new(&reader, &source, \"file.ref\");\n \tEXPECT_ERR(err);\n \n-\treftable_reader_init_ref_iterator(&rd, &it);\n+\treftable_reader_init_ref_iterator(reader, &it);\n \terr = reftable_iterator_seek_ref(&it, names[0]);\n \tEXPECT_ERR(err);\n \n@@ -431,7 +431,7 @@ static void test_table_read_api(void)\n \t}\n \treftable_iterator_destroy(&it);\n \treftable_free(names);\n-\treader_close(&rd);\n+\treftable_reader_free(reader);\n \tstrbuf_release(&buf);\n }\n \n@@ -440,7 +440,7 @@ static void test_table_read_write_seek(int index, int hash_id)\n \tchar **names;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint N = 50;\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { NULL };\n \tint err;\n \tint i = 0;\n@@ -453,18 +453,18 @@ static void test_table_read_write_seek(int index, int hash_id)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = init_reader(&rd, &source, \"file.ref\");\n+\terr = reftable_reader_new(&reader, &source, \"file.ref\");\n \tEXPECT_ERR(err);\n-\tEXPECT(hash_id == reftable_reader_hash_id(&rd));\n+\tEXPECT(hash_id == reftable_reader_hash_id(reader));\n \n \tif (!index) {\n-\t\trd.ref_offsets.index_offset = 0;\n+\t\treader->ref_offsets.index_offset = 0;\n \t} else {\n-\t\tEXPECT(rd.ref_offsets.index_offset > 0);\n+\t\tEXPECT(reader->ref_offsets.index_offset > 0);\n \t}\n \n \tfor (i = 1; i < N; i++) {\n-\t\treftable_reader_init_ref_iterator(&rd, &it);\n+\t\treftable_reader_init_ref_iterator(reader, &it);\n \t\terr = reftable_iterator_seek_ref(&it, names[i]);\n \t\tEXPECT_ERR(err);\n \t\terr = reftable_iterator_next_ref(&it, &ref);\n@@ -480,7 +480,7 @@ static void test_table_read_write_seek(int index, int hash_id)\n \tstrbuf_addstr(&pastLast, names[N - 1]);\n \tstrbuf_addstr(&pastLast, \"/\");\n \n-\treftable_reader_init_ref_iterator(&rd, &it);\n+\treftable_reader_init_ref_iterator(reader, &it);\n \terr = reftable_iterator_seek_ref(&it, pastLast.buf);\n \tif (err == 0) {\n \t\tstruct reftable_ref_record ref = { NULL };\n@@ -498,7 +498,7 @@ static void test_table_read_write_seek(int index, int hash_id)\n \t\treftable_free(names[i]);\n \t}\n \treftable_free(names);\n-\treader_close(&rd);\n+\treftable_reader_free(reader);\n }\n \n static void test_table_read_write_seek_linear(void)\n@@ -530,7 +530,7 @@ static void test_table_refs_for(int indexed)\n \tint i = 0;\n \tint n;\n \tint err;\n-\tstruct reftable_reader rd;\n+\tstruct reftable_reader *reader;\n \tstruct reftable_block_source source = { NULL };\n \n \tstruct strbuf buf = STRBUF_INIT;\n@@ -579,18 +579,18 @@ static void test_table_refs_for(int indexed)\n \n \tblock_source_from_strbuf(&source, &buf);\n \n-\terr = init_reader(&rd, &source, \"file.ref\");\n+\terr = reftable_reader_new(&reader, &source, \"file.ref\");\n \tEXPECT_ERR(err);\n \tif (!indexed) {\n-\t\trd.obj_offsets.is_present = 0;\n+\t\treader->obj_offsets.is_present = 0;\n \t}\n \n-\treftable_reader_init_ref_iterator(&rd, &it);\n+\treftable_reader_init_ref_iterator(reader, &it);\n \terr = reftable_iterator_seek_ref(&it, \"\");\n \tEXPECT_ERR(err);\n \treftable_iterator_destroy(&it);\n \n-\terr = reftable_reader_refs_for(&rd, &it, want_hash);\n+\terr = reftable_reader_refs_for(reader, &it, want_hash);\n \tEXPECT_ERR(err);\n \n \tj = 0;\n@@ -611,7 +611,7 @@ static void test_table_refs_for(int indexed)\n \tstrbuf_release(&buf);\n \tfree_names(want_names);\n \treftable_iterator_destroy(&it);\n-\treader_close(&rd);\n+\treftable_reader_free(reader);\n }\n \n static void test_table_refs_for_no_index(void)\n@@ -928,11 +928,11 @@ static void test_corrupt_table_empty(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_reader *reader;\n \tint err;\n \n \tblock_source_from_strbuf(&source, &buf);\n-\terr = init_reader(&rd, &source, \"file.log\");\n+\terr = reftable_reader_new(&reader, &source, \"file.log\");\n \tEXPECT(err == REFTABLE_FORMAT_ERROR);\n }\n \n@@ -941,13 +941,14 @@ static void test_corrupt_table(void)\n \tuint8_t zeros[1024] = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader rd = { NULL };\n+\tstruct reftable_reader *reader;\n \tint err;\n \tstrbuf_add(&buf, zeros, sizeof(zeros));\n \n \tblock_source_from_strbuf(&source, &buf);\n-\terr = init_reader(&rd, &source, \"file.log\");\n+\terr = reftable_reader_new(&reader, &source, \"file.log\");\n \tEXPECT(err == REFTABLE_FORMAT_ERROR);\n+\n \tstrbuf_release(&buf);\n }\n \n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501602","messageId":"b129d8a8687975a813f3b165b82ce4599b6e3901.1724420744.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724420744.git.ps@pks.im","subject":"[PATCH v2 05/10] reftable/reader: inline `reader_close()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-23T14:12:43Z","receivedAt":"2024-08-23T14:12:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Same as with the preceding commit, we also provide a `reader_close()`\nfunction that allows the caller to close a reader without freeing it.\nThis is unnecessary now that all users will have an allocated version of\nthe reader.\n\nInline it into `reftable_reader_free()`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/reader.c | 9 ++-------\n reftable/reader.h | 1 -\n reftable/stack.c  | 5 +----\n 3 files changed, 3 insertions(+), 12 deletions(-)\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 9239679ad95..037417fcf63 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -579,12 +579,6 @@ void reftable_reader_init_log_iterator(struct reftable_reader *r,\n \treader_init_iter(r, it, BLOCK_TYPE_LOG);\n }\n \n-void reader_close(struct reftable_reader *r)\n-{\n-\tblock_source_close(&r->source);\n-\tFREE_AND_NULL(r->name);\n-}\n-\n int reftable_reader_new(struct reftable_reader **out,\n \t\t\tstruct reftable_block_source *source, char const *name)\n {\n@@ -655,7 +649,8 @@ void reftable_reader_free(struct reftable_reader *r)\n {\n \tif (!r)\n \t\treturn;\n-\treader_close(r);\n+\tblock_source_close(&r->source);\n+\tFREE_AND_NULL(r->name);\n \treftable_free(r);\n }\n \ndiff --git a/reftable/reader.h b/reftable/reader.h\nindex 762cd6de667..88b4f3b4212 100644\n--- a/reftable/reader.h\n+++ b/reftable/reader.h\n@@ -52,7 +52,6 @@ struct reftable_reader {\n \tstruct reftable_reader_offsets log_offsets;\n };\n \n-void reader_close(struct reftable_reader *r);\n const char *reader_name(struct reftable_reader *r);\n \n void reader_init_iter(struct reftable_reader *r,\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex c72435b0596..0ac9cdf8de1 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -290,7 +290,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t\t\tconst char *name = reader_name(cur[i]);\n \t\t\tstack_filename(&table_path, st, name);\n \n-\t\t\treader_close(cur[i]);\n \t\t\treftable_reader_free(cur[i]);\n \n \t\t\t/* On Windows, can only unlink after closing. */\n@@ -299,10 +298,8 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t}\n \n done:\n-\tfor (i = 0; i < new_readers_len; i++) {\n-\t\treader_close(new_readers[i]);\n+\tfor (i = 0; i < new_readers_len; i++)\n \t\treftable_reader_free(new_readers[i]);\n-\t}\n \treftable_free(new_readers);\n \treftable_free(cur);\n \tstrbuf_release(&table_path);\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501603","messageId":"e3b28709b5fd525b95c6f296e5b2920561d9a8a3.1724420744.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724420744.git.ps@pks.im","subject":"[PATCH v2 06/10] reftable/stack: fix broken refnames in `write_n_ref_tables()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-23T14:12:46Z","receivedAt":"2024-08-23T14:12:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `write_n_ref_tables()` helper function writes N references in\nseparate tables. We never reset the computed name of those references\nthough, leading us to end up with unexpected names.\n\nFix this by resetting the buffer.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack_test.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 42044ed8a3e..de0669b7b8a 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -125,6 +125,7 @@ static void write_n_ref_tables(struct reftable_stack *st,\n \t\t\t.value_type = REFTABLE_REF_VAL1,\n \t\t};\n \n+\t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"refs/heads/branch-%04u\", (unsigned) i);\n \t\tref.refname = buf.buf;\n \t\tset_test_hash(ref.value.val1, i);\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501604","messageId":"6535d1ca9dede63011333a68f885d9782bcb182c.1724420744.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724420744.git.ps@pks.im","subject":"[PATCH v2 07/10] reftable/reader: introduce refcounting","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-23T14:12:48Z","receivedAt":"2024-08-23T14:12:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It was recently reported that concurrent reads and writes may cause the\nreftable backend to segfault. The root cause of this is that we do not\nproperly keep track of reftable readers across reloads.\n\nSuppose that you have a reftable iterator and then decide to reload the\nstack while iterating through the iterator. When the stack has been\nrewritten since we have created the iterator, then we would end up\ndiscarding a subset of readers that may still be in use by the iterator.\nThe consequence is that we now try to reference deallocated memory,\nwhich of course segfaults.\n\nOne way to trigger this is in t5616, where some background maintenance\njobs have been leaking from one test into another. This leads to stack\ntraces like the following one:\n\n  + git -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 --refetch origin\n  AddressSanitizer:DEADLYSIGNAL\n  =================================================================\n  ==657994==ERROR: AddressSanitizer: SEGV on unknown address 0x7fa0f0ec6089 (pc 0x55f23e52ddf9 bp\n0x7ffe7bfa1700 sp 0x7ffe7bfa1700 T0)\n  ==657994==The signal is caused by a READ memory access.\n      #0 0x55f23e52ddf9 in get_var_int reftable/record.c:29\n      #1 0x55f23e53295e in reftable_decode_keylen reftable/record.c:170\n      #2 0x55f23e532cc0 in reftable_decode_key reftable/record.c:194\n      #3 0x55f23e54e72e in block_iter_next reftable/block.c:398\n      #4 0x55f23e5573dc in table_iter_next_in_block reftable/reader.c:240\n      #5 0x55f23e5573dc in table_iter_next reftable/reader.c:355\n      #6 0x55f23e5573dc in table_iter_next reftable/reader.c:339\n      #7 0x55f23e551283 in merged_iter_advance_subiter reftable/merged.c:69\n      #8 0x55f23e55169e in merged_iter_next_entry reftable/merged.c:123\n      #9 0x55f23e55169e in merged_iter_next_void reftable/merged.c:172\n      #10 0x55f23e537625 in reftable_iterator_next_ref reftable/generic.c:175\n      #11 0x55f23e2cf9c6 in reftable_ref_iterator_advance refs/reftable-backend.c:464\n      #12 0x55f23e2d996e in ref_iterator_advance refs/iterator.c:13\n      #13 0x55f23e2d996e in do_for_each_ref_iterator refs/iterator.c:452\n      #14 0x55f23dca6767 in get_ref_map builtin/fetch.c:623\n      #15 0x55f23dca6767 in do_fetch builtin/fetch.c:1659\n      #16 0x55f23dca6767 in fetch_one builtin/fetch.c:2133\n      #17 0x55f23dca6767 in cmd_fetch builtin/fetch.c:2432\n      #18 0x55f23dba7764 in run_builtin git.c:484\n      #19 0x55f23dba7764 in handle_builtin git.c:741\n      #20 0x55f23dbab61e in run_argv git.c:805\n      #21 0x55f23dbab61e in cmd_main git.c:1000\n      #22 0x55f23dba4781 in main common-main.c:64\n      #23 0x7fa0f063fc89 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n      #24 0x7fa0f063fd44 in __libc_start_main_impl ../csu/libc-start.c:360\n      #25 0x55f23dba6ad0 in _start (git+0xadfad0) (BuildId: 803b2b7f59beb03d7849fb8294a8e2145dd4aa27)\n\nWhile it is somewhat awkward that the maintenance processes survive\ntests in the first place, it is totally expected that reftables should\nwork alright with concurrent writers. Seemingly they don't.\n\nThe only underlying resource that we need to care about in this context\nis the reftable reader, which is responsible for reading a single table\nfrom disk. These readers get discarded immediately (unless reused) when\ncalling `reftable_stack_reload()`, which is wrong. We can only close\nthem once we know that there are no iterators using them anymore.\n\nPrepare for a fix by converting the reftable readers to be refcounted.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/reader.c                | 16 ++++++++++++++--\n reftable/reader.h                |  2 ++\n reftable/readwrite_test.c        | 18 +++++++++---------\n reftable/reftable-reader.h       | 15 ++++++++++++---\n reftable/stack.c                 |  8 ++++----\n reftable/stack_test.c            |  6 ++----\n t/helper/test-reftable.c         |  2 +-\n t/unit-tests/t-reftable-merged.c |  4 ++--\n 8 files changed, 46 insertions(+), 25 deletions(-)\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 037417fcf63..64a0953e687 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -621,6 +621,7 @@ int reftable_reader_new(struct reftable_reader **out,\n \tr->source = *source;\n \tr->name = xstrdup(name);\n \tr->hash_id = 0;\n+\tr->refcount = 1;\n \n \terr = block_source_read_block(source, &footer, r->size,\n \t\t\t\t      footer_size(r->version));\n@@ -645,10 +646,21 @@ int reftable_reader_new(struct reftable_reader **out,\n \treturn err;\n }\n \n-void reftable_reader_free(struct reftable_reader *r)\n+void reftable_reader_incref(struct reftable_reader *r)\n+{\n+\tif (!r->refcount)\n+\t\tBUG(\"cannot increment ref counter of dead reader\");\n+\tr->refcount++;\n+}\n+\n+void reftable_reader_decref(struct reftable_reader *r)\n {\n \tif (!r)\n \t\treturn;\n+\tif (!r->refcount)\n+\t\tBUG(\"cannot decrement ref counter of dead reader\");\n+\tif (--r->refcount)\n+\t\treturn;\n \tblock_source_close(&r->source);\n \tFREE_AND_NULL(r->name);\n \treftable_free(r);\n@@ -812,7 +824,7 @@ int reftable_reader_print_blocks(const char *tablename)\n \t}\n \n done:\n-\treftable_reader_free(r);\n+\treftable_reader_decref(r);\n \ttable_iter_close(&ti);\n \treturn err;\n }\ndiff --git a/reftable/reader.h b/reftable/reader.h\nindex 88b4f3b4212..3710ee09b4c 100644\n--- a/reftable/reader.h\n+++ b/reftable/reader.h\n@@ -50,6 +50,8 @@ struct reftable_reader {\n \tstruct reftable_reader_offsets ref_offsets;\n \tstruct reftable_reader_offsets obj_offsets;\n \tstruct reftable_reader_offsets log_offsets;\n+\n+\tuint64_t refcount;\n };\n \n const char *reader_name(struct reftable_reader *r);\ndiff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c\nindex 2f2ff787b26..0494e7955a5 100644\n--- a/reftable/readwrite_test.c\n+++ b/reftable/readwrite_test.c\n@@ -279,7 +279,7 @@ static void test_log_write_read(void)\n \t/* cleanup. */\n \tstrbuf_release(&buf);\n \tfree_names(names);\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n }\n \n static void test_log_zlib_corruption(void)\n@@ -341,7 +341,7 @@ static void test_log_zlib_corruption(void)\n \treftable_iterator_destroy(&it);\n \n \t/* cleanup. */\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n \tstrbuf_release(&buf);\n }\n \n@@ -383,7 +383,7 @@ static void test_table_read_write_sequential(void)\n \tEXPECT(j == N);\n \n \treftable_iterator_destroy(&it);\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n \tstrbuf_release(&buf);\n \tfree_names(names);\n }\n@@ -431,7 +431,7 @@ static void test_table_read_api(void)\n \t}\n \treftable_iterator_destroy(&it);\n \treftable_free(names);\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n \tstrbuf_release(&buf);\n }\n \n@@ -498,7 +498,7 @@ static void test_table_read_write_seek(int index, int hash_id)\n \t\treftable_free(names[i]);\n \t}\n \treftable_free(names);\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n }\n \n static void test_table_read_write_seek_linear(void)\n@@ -611,7 +611,7 @@ static void test_table_refs_for(int indexed)\n \tstrbuf_release(&buf);\n \tfree_names(want_names);\n \treftable_iterator_destroy(&it);\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n }\n \n static void test_table_refs_for_no_index(void)\n@@ -657,7 +657,7 @@ static void test_write_empty_table(void)\n \tEXPECT(err > 0);\n \n \treftable_iterator_destroy(&it);\n-\treftable_reader_free(rd);\n+\treftable_reader_decref(rd);\n \tstrbuf_release(&buf);\n }\n \n@@ -863,7 +863,7 @@ static void test_write_multiple_indices(void)\n \n \treftable_iterator_destroy(&it);\n \treftable_writer_free(writer);\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n \tstrbuf_release(&writer_buf);\n \tstrbuf_release(&buf);\n }\n@@ -919,7 +919,7 @@ static void test_write_multi_level_index(void)\n \n \treftable_iterator_destroy(&it);\n \treftable_writer_free(writer);\n-\treftable_reader_free(reader);\n+\treftable_reader_decref(reader);\n \tstrbuf_release(&writer_buf);\n \tstrbuf_release(&buf);\n }\ndiff --git a/reftable/reftable-reader.h b/reftable/reftable-reader.h\nindex 8a05c846858..a600452b565 100644\n--- a/reftable/reftable-reader.h\n+++ b/reftable/reftable-reader.h\n@@ -33,6 +33,18 @@ struct reftable_reader;\n int reftable_reader_new(struct reftable_reader **pp,\n \t\t\tstruct reftable_block_source *src, const char *name);\n \n+/*\n+ * Manage the reference count of the reftable reader. A newly initialized\n+ * reader starts with a refcount of 1 and will be deleted once the refcount has\n+ * reached 0.\n+ *\n+ * This is required because readers may have longer lifetimes than the stack\n+ * they belong to. The stack may for example be reloaded while the old tables\n+ * are still being accessed by an iterator.\n+ */\n+void reftable_reader_incref(struct reftable_reader *reader);\n+void reftable_reader_decref(struct reftable_reader *reader);\n+\n /* Initialize a reftable iterator for reading refs. */\n void reftable_reader_init_ref_iterator(struct reftable_reader *r,\n \t\t\t\t       struct reftable_iterator *it);\n@@ -44,9 +56,6 @@ void reftable_reader_init_log_iterator(struct reftable_reader *r,\n /* returns the hash ID used in this table. */\n uint32_t reftable_reader_hash_id(struct reftable_reader *r);\n \n-/* closes and deallocates a reader. */\n-void reftable_reader_free(struct reftable_reader *);\n-\n /* return an iterator for the refs pointing to `oid`. */\n int reftable_reader_refs_for(struct reftable_reader *r,\n \t\t\t     struct reftable_iterator *it, uint8_t *oid);\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 0ac9cdf8de1..8e85f8b4d99 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -186,7 +186,7 @@ void reftable_stack_destroy(struct reftable_stack *st)\n \t\t\tif (names && !has_name(names, name)) {\n \t\t\t\tstack_filename(&filename, st, name);\n \t\t\t}\n-\t\t\treftable_reader_free(st->readers[i]);\n+\t\t\treftable_reader_decref(st->readers[i]);\n \n \t\t\tif (filename.len) {\n \t\t\t\t/* On Windows, can only unlink after closing. */\n@@ -290,7 +290,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t\t\tconst char *name = reader_name(cur[i]);\n \t\t\tstack_filename(&table_path, st, name);\n \n-\t\t\treftable_reader_free(cur[i]);\n+\t\t\treftable_reader_decref(cur[i]);\n \n \t\t\t/* On Windows, can only unlink after closing. */\n \t\t\tunlink(table_path.buf);\n@@ -299,7 +299,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \n done:\n \tfor (i = 0; i < new_readers_len; i++)\n-\t\treftable_reader_free(new_readers[i]);\n+\t\treftable_reader_decref(new_readers[i]);\n \treftable_free(new_readers);\n \treftable_free(cur);\n \tstrbuf_release(&table_path);\n@@ -1534,7 +1534,7 @@ static void remove_maybe_stale_table(struct reftable_stack *st, uint64_t max,\n \t\tgoto done;\n \n \tupdate_idx = reftable_reader_max_update_index(rd);\n-\treftable_reader_free(rd);\n+\treftable_reader_decref(rd);\n \n \tif (update_idx <= max) {\n \t\tunlink(table_path.buf);\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex de0669b7b8a..bc3bf772749 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -1036,10 +1036,8 @@ static void test_reftable_stack_compaction_concurrent(void)\n static void unclean_stack_close(struct reftable_stack *st)\n {\n \t/* break abstraction boundary to simulate unclean shutdown. */\n-\tint i = 0;\n-\tfor (; i < st->readers_len; i++) {\n-\t\treftable_reader_free(st->readers[i]);\n-\t}\n+\tfor (size_t i = 0; i < st->readers_len; i++)\n+\t\treftable_reader_decref(st->readers[i]);\n \tst->readers_len = 0;\n \tFREE_AND_NULL(st->readers);\n }\ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex 87c2f42aaed..f6d855a9d7f 100644\n--- a/t/helper/test-reftable.c\n+++ b/t/helper/test-reftable.c\n@@ -152,7 +152,7 @@ static int dump_reftable(const char *tablename)\n \n done:\n \treftable_merged_table_free(mt);\n-\treftable_reader_free(r);\n+\treftable_reader_decref(r);\n \treturn err;\n }\n \ndiff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c\nindex 8f51aca1b62..081d3c8b697 100644\n--- a/t/unit-tests/t-reftable-merged.c\n+++ b/t/unit-tests/t-reftable-merged.c\n@@ -115,7 +115,7 @@ merged_table_from_records(struct reftable_ref_record **refs,\n static void readers_destroy(struct reftable_reader **readers, const size_t n)\n {\n \tfor (size_t i = 0; i < n; i++)\n-\t\treftable_reader_free(readers[i]);\n+\t\treftable_reader_decref(readers[i]);\n \treftable_free(readers);\n }\n \n@@ -437,7 +437,7 @@ static void t_default_write_opts(void)\n \terr = reftable_merged_table_new(&merged, &rd, 1, GIT_SHA1_FORMAT_ID);\n \tcheck(!err);\n \n-\treftable_reader_free(rd);\n+\treftable_reader_decref(rd);\n \treftable_merged_table_free(merged);\n \tstrbuf_release(&buf);\n }\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501605","messageId":"8d08c3bc515db8a3d7fc0a617a2cf1025164f472.1724420744.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724420744.git.ps@pks.im","subject":"[PATCH v2 08/10] reftable/reader: keep readers alive during iteration","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-23T14:12:51Z","receivedAt":"2024-08-23T14:12:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The lifetime of a table iterator may survive the lifetime of a reader\nwhen the stack gets reloaded. Keep the reader from being released by\nincreasing its refcount while the iterator is still being used.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/reader.c     |  2 ++\n reftable/stack_test.c | 50 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+)\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 64a0953e687..f8770990876 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -175,6 +175,7 @@ static int table_iter_init(struct table_iter *ti, struct reftable_reader *r)\n {\n \tstruct block_iter bi = BLOCK_ITER_INIT;\n \tmemset(ti, 0, sizeof(*ti));\n+\treftable_reader_incref(r);\n \tti->r = r;\n \tti->bi = bi;\n \treturn 0;\n@@ -262,6 +263,7 @@ static void table_iter_close(struct table_iter *ti)\n {\n \ttable_iter_block_done(ti);\n \tblock_iter_close(&ti->bi);\n+\treftable_reader_decref(ti->r);\n }\n \n static int table_iter_next_block(struct table_iter *ti)\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex bc3bf772749..7fb5beb7c94 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -1076,6 +1076,55 @@ static void test_reftable_stack_compaction_concurrent_clean(void)\n \tclear_dir(dir);\n }\n \n+static void test_reftable_stack_read_across_reload(void)\n+{\n+\tstruct reftable_write_options opts = { 0 };\n+\tstruct reftable_stack *st1 = NULL, *st2 = NULL;\n+\tstruct reftable_ref_record rec = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tint err;\n+\n+\t/* Create a first stack and set up an iterator for it. */\n+\terr = reftable_new_stack(&st1, dir, &opts);\n+\tEXPECT_ERR(err);\n+\twrite_n_ref_tables(st1, 2);\n+\tEXPECT(st1->merged->readers_len == 2);\n+\treftable_stack_init_ref_iterator(st1, &it);\n+\terr = reftable_iterator_seek_ref(&it, \"\");\n+\tEXPECT_ERR(err);\n+\n+\t/* Set up a second stack for the same directory and compact it. */\n+\terr = reftable_new_stack(&st2, dir, &opts);\n+\tEXPECT_ERR(err);\n+\tEXPECT(st2->merged->readers_len == 2);\n+\terr = reftable_stack_compact_all(st2, NULL);\n+\tEXPECT_ERR(err);\n+\tEXPECT(st2->merged->readers_len == 1);\n+\n+\t/*\n+\t * Verify that we can continue to use the old iterator even after we\n+\t * have reloaded its stack.\n+\t */\n+\terr = reftable_stack_reload(st1);\n+\tEXPECT_ERR(err);\n+\tEXPECT(st1->merged->readers_len == 1);\n+\terr = reftable_iterator_next_ref(&it, &rec);\n+\tEXPECT_ERR(err);\n+\tEXPECT(!strcmp(rec.refname, \"refs/heads/branch-0000\"));\n+\terr = reftable_iterator_next_ref(&it, &rec);\n+\tEXPECT_ERR(err);\n+\tEXPECT(!strcmp(rec.refname, \"refs/heads/branch-0001\"));\n+\terr = reftable_iterator_next_ref(&it, &rec);\n+\tEXPECT(err > 0);\n+\n+\treftable_ref_record_release(&rec);\n+\treftable_iterator_destroy(&it);\n+\treftable_stack_destroy(st1);\n+\treftable_stack_destroy(st2);\n+\tclear_dir(dir);\n+}\n+\n int stack_test_main(int argc, const char *argv[])\n {\n \tRUN_TEST(test_empty_add);\n@@ -1098,6 +1147,7 @@ int stack_test_main(int argc, const char *argv[])\n \tRUN_TEST(test_reftable_stack_auto_compaction_fails_gracefully);\n \tRUN_TEST(test_reftable_stack_update_index_check);\n \tRUN_TEST(test_reftable_stack_uptodate);\n+\tRUN_TEST(test_reftable_stack_read_across_reload);\n \tRUN_TEST(test_suggest_compaction_segment);\n \tRUN_TEST(test_suggest_compaction_segment_nothing);\n \treturn 0;\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501606","messageId":"5aee91de25e5bd5198fef307549b58bb4625161f.1724420744.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724420744.git.ps@pks.im","subject":"[PATCH v2 09/10] reftable/stack: reorder swapping in the reloaded stack contents","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-23T14:12:54Z","receivedAt":"2024-08-23T14:12:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The code flow of how we swap in the reloaded stack contents is somewhat\nconvoluted because we switch back and forth between swapping in\ndifferent parts of the stack.\n\nReorder the code to simplify it. We now first close and unlink the old\ntables which do not get reused before we update the stack to point to\nthe new stack.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 34 +++++++++++++++++++---------------\n 1 file changed, 19 insertions(+), 15 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 8e85f8b4d99..02472222589 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -273,30 +273,34 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \tif (err < 0)\n \t\tgoto done;\n \n-\tst->readers_len = new_readers_len;\n-\tif (st->merged)\n-\t\treftable_merged_table_free(st->merged);\n-\tif (st->readers) {\n-\t\treftable_free(st->readers);\n-\t}\n-\tst->readers = new_readers;\n-\tnew_readers = NULL;\n-\tnew_readers_len = 0;\n-\n-\tnew_merged->suppress_deletions = 1;\n-\tst->merged = new_merged;\n+\t/*\n+\t * Close the old, non-reused readers and proactively try to unlink\n+\t * them. This is done for systems like Windows, where the underlying\n+\t * file of such an open reader wouldn't have been possible to be\n+\t * unlinked by the compacting process.\n+\t */\n \tfor (i = 0; i < cur_len; i++) {\n \t\tif (cur[i]) {\n \t\t\tconst char *name = reader_name(cur[i]);\n \t\t\tstack_filename(&table_path, st, name);\n-\n \t\t\treftable_reader_decref(cur[i]);\n-\n-\t\t\t/* On Windows, can only unlink after closing. */\n \t\t\tunlink(table_path.buf);\n \t\t}\n \t}\n \n+\t/* Update the stack to point to the new tables. */\n+\tif (st->merged)\n+\t\treftable_merged_table_free(st->merged);\n+\tnew_merged->suppress_deletions = 1;\n+\tst->merged = new_merged;\n+\n+\tif (st->readers)\n+\t\treftable_free(st->readers);\n+\tst->readers = new_readers;\n+\tst->readers_len = new_readers_len;\n+\tnew_readers = NULL;\n+\tnew_readers_len = 0;\n+\n done:\n \tfor (i = 0; i < new_readers_len; i++)\n \t\treftable_reader_decref(new_readers[i]);\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501607","messageId":"4a8d45cc9b49c3351012a36728ee2295b002709a.1724420744.git.ps@pks.im","threadId":"61975","inReplyTo":"cover.1724420744.git.ps@pks.im","subject":"[PATCH v2 10/10] reftable/stack: fix segfault when reload with reused readers fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-23T14:12:57Z","receivedAt":"2024-08-23T14:13:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It is expected that reloading the stack fails with concurrent writers,\ne.g. because a table that we just wanted to read just got compacted.\nIn case we decided to reuse readers this will cause a segfault though\nbecause we unconditionally release all new readers, including the reused\nones. As those are still referenced by the current stack, the result is\nthat we will eventually try to dereference those already-freed readers.\n\nFix this bug by incrementing the refcount of reused readers temporarily.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c      | 23 +++++++++++++++++\n reftable/stack_test.c | 59 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 82 insertions(+)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 02472222589..ce0a35216ba 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -226,6 +226,8 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n {\n \tsize_t cur_len = !st->merged ? 0 : st->merged->readers_len;\n \tstruct reftable_reader **cur = stack_copy_readers(st, cur_len);\n+\tstruct reftable_reader **reused = NULL;\n+\tsize_t reused_len = 0, reused_alloc = 0;\n \tsize_t names_len = names_length(names);\n \tstruct reftable_reader **new_readers =\n \t\treftable_calloc(names_len, sizeof(*new_readers));\n@@ -245,6 +247,18 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t\t\tif (cur[i] && 0 == strcmp(cur[i]->name, name)) {\n \t\t\t\trd = cur[i];\n \t\t\t\tcur[i] = NULL;\n+\n+\t\t\t\t/*\n+\t\t\t\t * When reloading the stack fails, we end up\n+\t\t\t\t * releasing all new readers. This also\n+\t\t\t\t * includes the reused readers, even though\n+\t\t\t\t * they are still in used by the old stack. We\n+\t\t\t\t * thus need to keep them alive here, which we\n+\t\t\t\t * do by bumping their refcount.\n+\t\t\t\t */\n+\t\t\t\tREFTABLE_ALLOC_GROW(reused, reused_len + 1, reused_alloc);\n+\t\t\t\treused[reused_len++] = rd;\n+\t\t\t\treftable_reader_incref(rd);\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n@@ -301,10 +315,19 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \tnew_readers = NULL;\n \tnew_readers_len = 0;\n \n+\t/*\n+\t * Decrement the refcount of reused readers again. This only needs to\n+\t * happen on the successful case, because on the unsuccessful one we\n+\t * decrement their refcount via `new_readers`.\n+\t */\n+\tfor (i = 0; i < reused_len; i++)\n+\t\treftable_reader_decref(reused[i]);\n+\n done:\n \tfor (i = 0; i < new_readers_len; i++)\n \t\treftable_reader_decref(new_readers[i]);\n \treftable_free(new_readers);\n+\treftable_free(reused);\n \treftable_free(cur);\n \tstrbuf_release(&table_path);\n \treturn err;\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 7fb5beb7c94..6809bf9d300 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -10,6 +10,7 @@ license that can be found in the LICENSE file or at\n \n #include \"system.h\"\n \n+#include \"copy.h\"\n #include \"reftable-reader.h\"\n #include \"merged.h\"\n #include \"basics.h\"\n@@ -1125,6 +1126,63 @@ static void test_reftable_stack_read_across_reload(void)\n \tclear_dir(dir);\n }\n \n+static void test_reftable_stack_reload_with_missing_table(void)\n+{\n+\tstruct reftable_write_options opts = { 0 };\n+\tstruct reftable_stack *st = NULL;\n+\tstruct reftable_ref_record rec = { 0 };\n+\tstruct reftable_iterator it = { 0 };\n+\tstruct strbuf table_path = STRBUF_INIT, content = STRBUF_INIT;\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tint err;\n+\n+\t/* Create a first stack and set up an iterator for it. */\n+\terr = reftable_new_stack(&st, dir, &opts);\n+\tEXPECT_ERR(err);\n+\twrite_n_ref_tables(st, 2);\n+\tEXPECT(st->merged->readers_len == 2);\n+\treftable_stack_init_ref_iterator(st, &it);\n+\terr = reftable_iterator_seek_ref(&it, \"\");\n+\tEXPECT_ERR(err);\n+\n+\t/*\n+\t * Update the tables.list file with some garbage data, while reusing\n+\t * our old readers. This should trigger a partial reload of the stack,\n+\t * where we try to reuse our old readers.\n+\t*/\n+\tstrbuf_addf(&content, \"%s\\n\", st->readers[0]->name);\n+\tstrbuf_addf(&content, \"%s\\n\", st->readers[1]->name);\n+\tstrbuf_addstr(&content, \"garbage\\n\");\n+\tstrbuf_addf(&table_path, \"%s.lock\", st->list_file);\n+\twrite_file_buf(table_path.buf, content.buf, content.len);\n+\terr = rename(table_path.buf, st->list_file);\n+\tEXPECT_ERR(err);\n+\n+\terr = reftable_stack_reload(st);\n+\tEXPECT(err == -4);\n+\tEXPECT(st->merged->readers_len == 2);\n+\n+\t/*\n+\t * Even though the reload has failed, we should be able to continue\n+\t * using the iterator.\n+\t*/\n+\terr = reftable_iterator_next_ref(&it, &rec);\n+\tEXPECT_ERR(err);\n+\tEXPECT(!strcmp(rec.refname, \"refs/heads/branch-0000\"));\n+\terr = reftable_iterator_next_ref(&it, &rec);\n+\tEXPECT_ERR(err);\n+\tEXPECT(!strcmp(rec.refname, \"refs/heads/branch-0001\"));\n+\terr = reftable_iterator_next_ref(&it, &rec);\n+\tEXPECT(err > 0);\n+\n+\treftable_ref_record_release(&rec);\n+\treftable_iterator_destroy(&it);\n+\treftable_stack_destroy(st);\n+\tstrbuf_release(&table_path);\n+\tstrbuf_release(&content);\n+\tclear_dir(dir);\n+}\n+\n int stack_test_main(int argc, const char *argv[])\n {\n \tRUN_TEST(test_empty_add);\n@@ -1148,6 +1206,7 @@ int stack_test_main(int argc, const char *argv[])\n \tRUN_TEST(test_reftable_stack_update_index_check);\n \tRUN_TEST(test_reftable_stack_uptodate);\n \tRUN_TEST(test_reftable_stack_read_across_reload);\n+\tRUN_TEST(test_reftable_stack_reload_with_missing_table);\n \tRUN_TEST(test_suggest_compaction_segment);\n \tRUN_TEST(test_suggest_compaction_segment_nothing);\n \treturn 0;\n-- \n2.46.0.164.g477ce5ccd6.dirty\n\n"},{"id":"501608","messageId":"xmqqle0np4k9.fsf@gitster.g","threadId":"61975","inReplyTo":"cover.1724420744.git.ps@pks.im","subject":"Re: [PATCH v2 00/10] reftable: fix reload with active iterators","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-23T16:49:42Z","receivedAt":"2024-08-23T16:49:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Changes compared to v1:\n>\n>   - Remove a spurious change that renamed `init_reader()` to\n>     `reader_init()`. That function went away in the subsequent commit\n>     anyway.\n>\n>   - Fix a typo in a commit message.\n>\n>   - Fix some assertions in one of the added tests.\n\nAll changes relative to v1 looked sensible to me.\n\nWill replace.\n"}]}