From 2125e370e0863e607620876f5d10c035349169d5 Mon Sep 17 00:00:00 2001
From: Albert Chu <chu11@llnl.gov>
Date: Wed, 10 Aug 2022 21:42:32 -0700
Subject: [PATCH 1/2] broker: avoid double adding cache entry onto list

Problem: A dirty cache entry has to potential to be added onto the
content cache's flush list twice.  This double addition can lead to list
corruption.  The observed side effect was a list that was shortened and no
longer accurate with respect to the `acct_dirty` counter.  This could lead
to hangs with content flush, missed flushes to the backing store, and
segfault/memory corruption in the worst case.

Solution: Remove the cache entry from the flush list before adding it.
The remove is a no-op if it is not already on a list.

Fixes #4482
---
 src/broker/content-cache.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/broker/content-cache.c b/src/broker/content-cache.c
index d2ef184bcd36..87510606167f 100644
--- a/src/broker/content-cache.c
+++ b/src/broker/content-cache.c
@@ -524,6 +524,18 @@ static void cache_store_continuation (flux_future_t *f, void *arg)
     cache_resume_flush (cache);
 }
 
+/* Issue #4482, there is a small chance a dirty entry could be added
+ * to the flush list twice which can lead to list corruption.  As an
+ * extra measure, perform a delete from the list first.  If the node
+ * is not on the list, the delete is a no-op.
+ */
+static void flush_list_append (struct content_cache *cache,
+                               struct cache_entry *e)
+{
+    list_del (&e->list);
+    list_add_tail (&cache->flush, &e->list);
+}
+
 static int cache_store (struct content_cache *cache, struct cache_entry *e)
 {
     flux_future_t *f;
@@ -535,7 +547,7 @@ static int cache_store (struct content_cache *cache, struct cache_entry *e)
         return 0;
     if (cache->rank == 0) {
         if (cache->flush_batch_count >= cache->flush_batch_limit) {
-            list_add_tail (&cache->flush, &e->list);
+            flush_list_append (cache, e);
             return 0;
         }
         flags = CONTENT_FLAG_CACHE_BYPASS;
@@ -595,7 +607,7 @@ static void content_store_request (flux_t *h, flux_msg_handler_t *mh,
          * during purge or dropcache, so this does not alter
          * behavior */
         if (cache->rank == 0 && !cache->backing)
-            list_add_tail (&cache->flush, &e->list);
+            flush_list_append (cache, e);
     }
     if (flux_respond_raw (h, msg, hash, hash_size) < 0)
         flux_log_error (h, "content store: flux_respond_raw");

From 8c51d6f58e7715bca7c72f9fddae0bd94b68cda0 Mon Sep 17 00:00:00 2001
From: Albert Chu <chu11@llnl.gov>
Date: Wed, 10 Aug 2022 21:43:58 -0700
Subject: [PATCH 2/2] testsuite: add content cache flush regression test

Problem: No test covers duplicate content cache entries
being added to the content cache's flush list.

Solution: Add a regression test.
---
 t/Makefile.am                           |  1 +
 t/issues/t4482-flush-list-corruption.sh | 40 +++++++++++++++++++++++++
 2 files changed, 41 insertions(+)
 create mode 100755 t/issues/t4482-flush-list-corruption.sh

diff --git a/t/Makefile.am b/t/Makefile.am
index 87e073c36a95..e813af7096ac 100644
--- a/t/Makefile.am
+++ b/t/Makefile.am
@@ -307,6 +307,7 @@ dist_check_SCRIPTS = \
 	issues/t4379-dirty-cache-entries-flush.sh \
 	issues/t4413-empty-eventlog.sh \
 	issues/t4465-job-list-use-after-free.sh \
+	issues/t4482-flush-list-corruption.sh \
 	python/__init__.py \
 	python/subflux.py \
 	python/tap \
diff --git a/t/issues/t4482-flush-list-corruption.sh b/t/issues/t4482-flush-list-corruption.sh
new file mode 100755
index 000000000000..94cc758d3a81
--- /dev/null
+++ b/t/issues/t4482-flush-list-corruption.sh
@@ -0,0 +1,40 @@
+#!/bin/sh -e
+
+# How this test works
+#
+# add some unique data to the content cache, we do several stores to
+# build up a decent length internal list of flushable cache entries.
+#
+# write some of the same data again, if error present internal flush
+# list will be messed up and length of flush list < number of dirty
+# entries (acct_dirty).
+#
+# before fix, flux content flush will hang b/c number of dirty entries
+# (acct_dirty) never reaches zero.
+
+cat <<-EOF >t4482.sh
+#!/bin/sh -e
+
+echo "abcde" | flux content store
+echo "fghij" | flux content store
+echo "klmno" | flux content store
+echo "pqrst" | flux content store
+echo "tuvwx" | flux content store
+
+echo "fghij" | flux content store
+echo "klmno" | flux content store
+
+flux module load content-sqlite
+
+flux content flush
+
+flux module remove content-sqlite
+
+EOF
+
+chmod +x t4482.sh
+
+flux start -s 1 \
+    -o,--setattr=broker.rc1_path= \
+    -o,--setattr=broker.rc3_path= \
+    ./t4482.sh