BUG/MEDIUM: cache: retain the primary or secondary entry only when detaching its row

http_action_req_cache_use() retains the entry returned by the lookup, then
takes the shctx lock to detach its row. Until that detach the row is still
in the avail list, and shctx_row_reserve_hot() recycles a row under the
shctx lock alone, without ever taking the cache lock, so the cache read
lock does not protect a retained entry. In that window another thread can
recycle the row: the entry is queued on the cleanup list and its blocks are
refilled with a response body, after which our detach works from a
block_count and a last_reserved that no longer describe that row, splices
the wrong blocks out of the avail list and takes a reference on blocks
owned by another row. A block then sits both in the avail list and in a
live row, and the next thread reserving it reads a body as a cache_entry.

Retain the entry under the same shctx lock that detaches its row, and
only once it is known to be usable. A row recycled in the meantime is no
longer considered complete, which cache_free_blocks() clears under that
lock, so the existing test already rejects it. The entry is still
readable at that point because recycled blocks are only written once
shctx_row_reserve_hot() has returned, which cannot happen before
cache_reserve_finish() has taken the write lock on the tree our read
lock holds.

The other path then holds no reference, so it has nothing to release.

This should be backported to all stable branches.

diff --git a/src/cache.c b/src/cache.c
index b9fa2d45a91c3641b78440fbac2ff1ced142fccd..4019d5f2f8eda3a20f5b249918470abf929db898 100644 (file)
--- a/src/cache.c
+++ b/src/cache.c
@@ -2601,12 +2601,18 @@ enum act_return http_action_req_cache_use(struct act_rule *rule, struct proxy *p
                struct appctx *appctx;
                int detached = 0;
 
-               retain_entry(res);
-
                entry_block = block_ptr(res);
                shctx_wrlock(shctx);
                if (res->expire > date.tv_sec &&
                    (res->flags & CACHE_EF_COMPLETE) && !(res->flags & CACHE_EF_STRIPPED)) {
+                       /* Retaining the entry and detaching its row must happen
+                        * under the same shctx lock: until the row is detached it
+                        * is still in the avail list, and reserving a row does not
+                        * take the cache lock. A row recycled between the lookup
+                        * and this lock no longer has CACHE_EF_COMPLETE, cleared
+                        * by cache_free_blocks() under this same lock.
+                        */
+                       retain_entry(res);
                        shctx_row_detach(shctx, entry_block);
                        detached = 1;
                } else {
@@ -2649,7 +2655,7 @@ enum act_return http_action_req_cache_use(struct act_rule *rule, struct proxy *p
                                                cache_strip_entry(shctx, res, hint_buf);
                                }
                        }
-                       release_entry(cache_tree, res, 0);
+                       /* Nothing was retained on this path. */
                        res = NULL;
                }
                shctx_wrunlock(shctx);
@@ -2690,11 +2696,15 @@ enum act_return http_action_req_cache_use(struct act_rule *rule, struct proxy *p
                                else if (sec_entry != res) {
                                        /* The wrong row was added to the hot list. */
                                        release_entry(cache_tree, res, 0);
-                                       retain_entry(sec_entry);
                                        shctx_wrlock(shctx);
                                        if (detached)
                                                cache_row_reattach(cache, entry_block);
                                        entry_block = block_ptr(sec_entry);
+                                       /* Same as for the primary entry above: retain
+                                        * this one under the lock that detaches its
+                                        * row.
+                                        */
+                                       retain_entry(sec_entry);
                                        shctx_row_detach(shctx, entry_block);
                                        shctx_wrunlock(shctx);
                                }