BUG/MEDIUM: cache: Fix crash when deleting secondary entry

When a cache is "cold" and multiple clients simultaneously try to access
the same resource we must forward all the requests to the server. Next,
every "duplicated" response will be processed in http_action_store_cache
and we will try to cache every one of them regardless of whether this
response was already cached. In order to avoid having multiple entries
for a same primary key, the logic is then to first delete any
preexisting entry from the cache tree before storing the current one.
The actual previous response content will not be deleted yet though
because if the corresponding row is detached from the "avail" list it
might still be used by a cache applet if it actually performed a lookup
in the cache tree before the new response could be received.

This all means that we can end up using a valid row that references a
cache_entry that was already removed from the cache tree. This does not
pose any problem in regular caches (no 'vary' mechanism enabled) because
the applet only works on the data and not the 'cache_entry' information,
but in the "vary" context, when calling 'http_cache_applet_release' we
might call 'delete_entry' on the given entry which in turn tries to
iterate over all the secondary entries to find the right one in which
the secondary entry counter can be updated. We would then call
eb32_next_dup on an entry that was not in the tree anymore which ended
up crashing.

This crash was introduced by "48f81ec09 : MAJOR: cache: Delay cache
entry delete in reserve_hot function" which added the call to
"release_entry" in "http_cache_applet_release" that ended up crashing.

This issue was raised in GitHub #2417.
This patch must be backported to branch 2.9.

diff --git a/src/cache.c b/src/cache.c
index 616c8b2d3e45122b9df4106d43acbc181d2503eb..973fd0199cced2e463a78489ad912d41f249ab17 100644 (file)
--- a/src/cache.c
+++ b/src/cache.c
@@ -530,7 +530,9 @@ static void delete_entry(struct cache_entry *del_entry)
        struct cache_entry *entry = NULL;
        struct eb32_node *last = NULL;
 
-       if (del_entry->secondary_key_signature) {
+       /* The entry might have been removed from the cache before. In such a
+        * case calling eb32_next_dup would crash. */
+       if (del_entry->secondary_key_signature && del_entry->eb.key != 0) {
                next = &del_entry->eb;
 
                /* Look for last entry of the duplicates list. */