reftable/stack: fix segfault when reload with reused readers fails

It is expected that reloading the stack fails with concurrent writers,
e.g. because a table that we just wanted to read just got compacted.
In case we decided to reuse readers this will cause a segfault though
because we unconditionally release all new readers, including the reused
ones. As those are still referenced by the current stack, the result is
that we will eventually try to dereference those already-freed readers.

Fix this bug by incrementing the refcount of reused readers temporarily.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/reftable/stack.c b/reftable/stack.c
index 02472222589ffec4da1c1721d804aa8859b8bbae..ce0a35216bad6d0171120e286f8a43076818d3f8 100644 (file)
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -226,6 +226,8 @@ static int reftable_stack_reload_once(struct reftable_stack *st,
 {
        size_t cur_len = !st->merged ? 0 : st->merged->readers_len;
        struct reftable_reader **cur = stack_copy_readers(st, cur_len);
+       struct reftable_reader **reused = NULL;
+       size_t reused_len = 0, reused_alloc = 0;
        size_t names_len = names_length(names);
        struct reftable_reader **new_readers =
                reftable_calloc(names_len, sizeof(*new_readers));
@@ -245,6 +247,18 @@ static int reftable_stack_reload_once(struct reftable_stack *st,
                        if (cur[i] && 0 == strcmp(cur[i]->name, name)) {
                                rd = cur[i];
                                cur[i] = NULL;
+
+                               /*
+                                * When reloading the stack fails, we end up
+                                * releasing all new readers. This also
+                                * includes the reused readers, even though
+                                * they are still in used by the old stack. We
+                                * thus need to keep them alive here, which we
+                                * do by bumping their refcount.
+                                */
+                               REFTABLE_ALLOC_GROW(reused, reused_len + 1, reused_alloc);
+                               reused[reused_len++] = rd;
+                               reftable_reader_incref(rd);
                                break;
                        }
                }
@@ -301,10 +315,19 @@ static int reftable_stack_reload_once(struct reftable_stack *st,
        new_readers = NULL;
        new_readers_len = 0;
 
+       /*
+        * Decrement the refcount of reused readers again. This only needs to
+        * happen on the successful case, because on the unsuccessful one we
+        * decrement their refcount via `new_readers`.
+        */
+       for (i = 0; i < reused_len; i++)
+               reftable_reader_decref(reused[i]);
+
 done:
        for (i = 0; i < new_readers_len; i++)
                reftable_reader_decref(new_readers[i]);
        reftable_free(new_readers);
+       reftable_free(reused);
        reftable_free(cur);
        strbuf_release(&table_path);
        return err;

diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 7fb5beb7c944b6951c1875eab378736ca45449ba..6809bf9d3002ac4b41f2d2458cdb85d465cee581 100644 (file)
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -10,6 +10,7 @@ https://developers.google.com/open-source/licenses/bsd
 
 #include "system.h"
 
+#include "copy.h"
 #include "reftable-reader.h"
 #include "merged.h"
 #include "basics.h"
@@ -1125,6 +1126,63 @@ static void test_reftable_stack_read_across_reload(void)
        clear_dir(dir);
 }
 
+static void test_reftable_stack_reload_with_missing_table(void)
+{
+       struct reftable_write_options opts = { 0 };
+       struct reftable_stack *st = NULL;
+       struct reftable_ref_record rec = { 0 };
+       struct reftable_iterator it = { 0 };
+       struct strbuf table_path = STRBUF_INIT, content = STRBUF_INIT;
+       char *dir = get_tmp_dir(__LINE__);
+       int err;
+
+       /* Create a first stack and set up an iterator for it. */
+       err = reftable_new_stack(&st, dir, &opts);
+       EXPECT_ERR(err);
+       write_n_ref_tables(st, 2);
+       EXPECT(st->merged->readers_len == 2);
+       reftable_stack_init_ref_iterator(st, &it);
+       err = reftable_iterator_seek_ref(&it, "");
+       EXPECT_ERR(err);
+
+       /*
+        * Update the tables.list file with some garbage data, while reusing
+        * our old readers. This should trigger a partial reload of the stack,
+        * where we try to reuse our old readers.
+       */
+       strbuf_addf(&content, "%s\n", st->readers[0]->name);
+       strbuf_addf(&content, "%s\n", st->readers[1]->name);
+       strbuf_addstr(&content, "garbage\n");
+       strbuf_addf(&table_path, "%s.lock", st->list_file);
+       write_file_buf(table_path.buf, content.buf, content.len);
+       err = rename(table_path.buf, st->list_file);
+       EXPECT_ERR(err);
+
+       err = reftable_stack_reload(st);
+       EXPECT(err == -4);
+       EXPECT(st->merged->readers_len == 2);
+
+       /*
+        * Even though the reload has failed, we should be able to continue
+        * using the iterator.
+       */
+       err = reftable_iterator_next_ref(&it, &rec);
+       EXPECT_ERR(err);
+       EXPECT(!strcmp(rec.refname, "refs/heads/branch-0000"));
+       err = reftable_iterator_next_ref(&it, &rec);
+       EXPECT_ERR(err);
+       EXPECT(!strcmp(rec.refname, "refs/heads/branch-0001"));
+       err = reftable_iterator_next_ref(&it, &rec);
+       EXPECT(err > 0);
+
+       reftable_ref_record_release(&rec);
+       reftable_iterator_destroy(&it);
+       reftable_stack_destroy(st);
+       strbuf_release(&table_path);
+       strbuf_release(&content);
+       clear_dir(dir);
+}
+
 int stack_test_main(int argc, const char *argv[])
 {
        RUN_TEST(test_empty_add);
@@ -1148,6 +1206,7 @@ int stack_test_main(int argc, const char *argv[])
        RUN_TEST(test_reftable_stack_update_index_check);
        RUN_TEST(test_reftable_stack_uptodate);
        RUN_TEST(test_reftable_stack_read_across_reload);
+       RUN_TEST(test_reftable_stack_reload_with_missing_table);
        RUN_TEST(test_suggest_compaction_segment);
        RUN_TEST(test_suggest_compaction_segment_nothing);
        return 0;