store: Track the actual mmap size separately

After a trim, the snip file is truncated and the header's nr_snips_alloc
is updated, but mremap is not called because shrinking the mapping is
unnecessary. local_nr_snips_alloc is also updated to match. When the
store subsequently grows, cs_file_resize and cs_ref both derive the old
mapping size from cs_file_size(local_nr_snips_alloc), which by that
point reflects the post-trim allocation rather than the extent of the
live mapping.

Introduce mapped_file_size to record the actual size of the current
mmap. Update it in cs_remap and cs_init, and use it wherever the true
mapped extent is required.

Author: cdown

src/store.c

@@ -94,6 +94,7 @@ static int _must_use_ _nonnull_ cs_remap(struct clip_store *cs, size_t old_size,
     }
     cs->header = new_header;
     cs->snips = (struct cs_snip *)(cs->header + 1);
+    cs->mapped_file_size = new_size;
     return 0;
 }
 
@@ -162,7 +163,7 @@ struct ref_guard cs_ref(struct clip_store *cs) {
 
         // If we shrank, no need to remap, since we'll just use the new bounds.
         if (cs->local_nr_snips_alloc < cs->header->nr_snips_alloc) {
-            int remap_ret = cs_remap(cs, cs_file_size(cs->local_nr_snips_alloc),
+            int remap_ret = cs_remap(cs, cs->mapped_file_size,
                                      cs_file_size(cs->header->nr_snips_alloc));
             if (remap_ret < 0) {
                 guard.status = remap_ret;
@@ -194,9 +195,7 @@ void drop_cs_unref(struct ref_guard *guard) {
  */
 int cs_destroy(struct clip_store *cs) {
     cs->ready = false;
-    // Don't use the value from the header: if it's out of date, we haven't
-    // done mremap() with the new size yet
-    if (munmap(cs->header, cs_file_size(cs->local_nr_snips_alloc))) {
+    if (munmap(cs->header, cs->mapped_file_size)) {
         return negative_errno();
     }
     return 0;
@@ -258,6 +257,7 @@ int cs_init(struct clip_store *cs, int snip_fd, int content_dir_fd) {
     cs->snips = (struct cs_snip *)(cs->header + 1);
     cs->local_nr_snips = cs->header->nr_snips;
     cs->local_nr_snips_alloc = cs->header->nr_snips_alloc;
+    cs->mapped_file_size = file_size;
     cs->ready = true;
 
     (void)guard; // Old clang will complain guard is unused, despite cleanup
@@ -308,10 +308,10 @@ static int _must_use_ _nonnull_ cs_file_resize(struct clip_store *cs,
     }
 
     if (grow) {
-        size_t old_size = cs_file_size(cs->header->nr_snips_alloc);
-        int ret = cs_remap(cs, old_size, new_size);
+        size_t old_file_size = cs_file_size(cs->header->nr_snips_alloc);
+        int ret = cs_remap(cs, cs->mapped_file_size, new_size);
         if (ret < 0) {
-            ftruncate(cs->snip_fd, (off_t)old_size);
+            expect(ftruncate(cs->snip_fd, (off_t)old_file_size) == 0);
             return ret;
         }
     }

src/store.h

@@ -63,6 +63,7 @@ static_assert(sizeof(struct cs_snip) == sizeof(struct cs_header),
  * @refcount: The reference count for the fd flock
  * @local_nr_snips: Our last known header->nr_snips
  * @local_nr_snips_alloc: Our last known header->nr_snips_alloc
+ * @mapped_file_size: The actual size of our current mmap() in bytes
  */
 struct clip_store {
     /* FDs */
@@ -77,6 +78,7 @@ struct clip_store {
     size_t refcount;
     size_t local_nr_snips;
     size_t local_nr_snips_alloc;
+    size_t mapped_file_size;
     bool ready;
 };
 

tests/test_store.c

@@ -112,6 +112,25 @@ static int create_test_content_dir_fd(void) {
     return dir_fd;
 }
 
+static int count_test_snip_maps(void) {
+#ifdef __linux__
+    _drop_(fclose) FILE *maps = fopen("/proc/self/maps", "r");
+    assert(maps != NULL);
+
+    char line[4096];
+    int count = 0;
+    while (fgets(line, sizeof(line), maps) != NULL) {
+        if (strstr(line, TEST_SNIP_FILE) != NULL) {
+            count++;
+        }
+    }
+
+    return count;
+#else
+    return -1;
+#endif
+}
+
 /* Test callables */
 static enum cs_remove_action remove_if_five(uint64_t hash, const char *line,
                                             void *private) {
@@ -646,6 +665,46 @@ static bool test__synchronisation(void) {
     return true;
 }
 
+static bool test__cs_trim__regrow_does_not_leak_mapping(void) {
+    struct clip_store cs = setup_test();
+
+    bool added = true;
+    for (int i = 0; i < 1500; i++) {
+        char buf[32];
+        snprintf(buf, sizeof(buf), "clip-%d", i);
+        if (cs_add(&cs, buf, NULL, CS_DUPE_KEEP_ALL) != 0) {
+            added = false;
+            break;
+        }
+    }
+    t_assert(added);
+
+    int maps_after_growth = count_test_snip_maps();
+    if (maps_after_growth < 0) {
+        drop_teardown_test(&cs);
+        return true;
+    }
+    t_assert(maps_after_growth == 1);
+
+    t_assert(cs_trim(&cs, CS_ITER_NEWEST_FIRST, 0) == 0);
+    t_assert(count_test_snip_maps() == 1);
+
+    t_assert(cs_add(&cs, "regrow", NULL, CS_DUPE_KEEP_ALL) == 0);
+    t_assert(count_test_snip_maps() == 1);
+
+    int snip_fd = cs.snip_fd;
+    int content_dir_fd = cs.content_dir_fd;
+    t_assert(cs_destroy(&cs) == 0);
+    t_assert(count_test_snip_maps() == 0);
+
+    close(snip_fd);
+    shm_unlink(TEST_SNIP_FILE);
+    close(content_dir_fd);
+    remove_test_content_dir(TEST_CONTENT_DIR);
+
+    return true;
+}
+
 static bool test__cs_add__dupe_keep_all(void) {
     _drop_(teardown_test) struct clip_store cs = setup_test();
 
@@ -771,6 +830,7 @@ int main(void) {
     t_run(test__cs_replace__out_of_bounds);
     t_run(test__cs_replace__add_fail_keeps_old_entry);
     t_run(test__synchronisation);
+    t_run(test__cs_trim__regrow_does_not_leak_mapping);
     t_run(test__cs_trim__no_remove_when_still_referenced);
     t_run(test__cs_snip__correct_nr_lines);
     t_run(test__first_line__empty);