BUG/MEDIUM: cache: do not create a second applet when two cache-use rules hit

A configuration with two "http-request cache-use" rules that may both find
the requested object, for example two caches both fed by unconditional
"cache-store" rules, or a cache-use rule in a defaults section that is
repeated in the proxy, leaks memory and cache space on every request that
hits both of them. Each such request leaks one appctx and its task (about
1kB on x86_64) and leaves the row of the first cache's entry retained
forever. The memory grows with the number of requests until the process is
restarted, which affects the system owner, and the rows retained this way
can never be evicted anymore: once their entry expires, their blocks are
lost and the cache progressively shrinks, until it cannot store anything.
Any client may trigger this by simply requesting cached objects. With
2000 requests, "show pools" reports 2016 appctx used and "show cache"
reports a refcount of 2000 on the entry, which stays like this after all
the streams are gone.

The cache-use action is not a final one. On a hit, it sets s->target to
the cache applet and creates the applet on the back stream connector with
sc_applet_create(), after retaining the entry and detaching its row. The
next cache-use rule is still evaluated and, when it also hits, it calls
sc_applet_create() again on the same stream connector. The new appctx
takes the same sedesc, so the first one is not referenced anymore: it was
woken up once, found no request in its input buffer and waits for one
forever. Its release callback is never called, so the entry it retained is
never released and its row never reattached to the avail list.

Let's simply ignore the cache-use rule when the stream already has a
target: the response will be produced by the applet that is already
there. This is consistent with the stats applet that is only set up when
s->target is NULL. An alternative would be to make the action final on a
hit, but this would change the evaluation of the following rules for all
users. Note that "use-service" has the same weakness, it creates its
applet without checking whether one is already attached, so a cache hit
followed by a matching use-service rule could leak the cache applet the
same way. This one is left as is since use-service is a final action and
mixing it with a cache hit is not a sensible configuration.

This patch should be backported to all supported versions. In older versions,
http_action_req_cache_use() may register the applet with a different
function (si_register_handler(), stream_int_register_handler()) but the
fix remains the same: return ACT_RET_CONT at the beginning of the function
when s->target is set.

diff --git a/reg-tests/cache/multiple_cache_use.vtc b/reg-tests/cache/multiple_cache_use.vtc
new file mode 100644 (file)
index 0000000..a9e48a3
--- /dev/null
+++ b/reg-tests/cache/multiple_cache_use.vtc
@@ -0,0 +1,69 @@
+varnishtest "Two cache-use rules hitting the same request"
+
+# A hit on a first cache-use rule attaches the cache applet to the stream. A
+# hit on a second cache-use rule must not create another applet, which would
+# replace the first one and leak it with its retained cache row: the row's
+# refcount would never drop back to zero.
+
+feature ignore_unknown_macro
+
+server s1 {
+    rxreq
+    txresp -hdr "Cache-Control: max-age=60" -bodylen 3000
+} -start
+
+haproxy h1 -conf {
+    global
+    .if feature(THREAD)
+        thread-groups 1
+    .endif
+
+    defaults
+        mode http
+        timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout client  "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout server  "${HAPROXY_TEST_TIMEOUT-5s}"
+
+    frontend fe
+        bind "fd@${fe}"
+        http-request cache-use c1
+        http-request cache-use c2
+        http-response cache-store c1
+        http-response cache-store c2
+        http-response set-header X-Cache-Hit %[res.cache_hit]
+        default_backend be
+
+    backend be
+        server s1 ${s1_addr}:${s1_port}
+
+    cache c1
+        total-max-size 4
+
+    cache c2
+        total-max-size 4
+} -start
+
+client c1 -connect ${h1_fe_sock} {
+    txreq -url "/a"
+    rxresp
+    expect resp.status == 200
+    expect resp.bodylen == 3000
+    expect resp.http.X-Cache-Hit == 0
+} -run
+
+# both caches hold the object now, every request hits both rules
+client c2 -connect ${h1_fe_sock} -repeat 3 {
+    txreq -url "/a"
+    rxresp
+    expect resp.status == 200
+    expect resp.bodylen == 3000
+    expect resp.http.X-Cache-Hit == 1
+} -run
+
+delay 0.2
+
+# the applets were all released, so the row of c1 is not retained anymore
+haproxy h1 -cli {
+    send "show cache"
+    expect ~ "c1 .*\n.*type:full size:[0-9]+ \\([0-9]+ blocks\\), refcount:0,"
+}

diff --git a/src/cache.c b/src/cache.c
index 3e3a2119ee41432b20bddbf78d0240d3eb90bf94..e11b13af895f0a6dfef3af73d65d8881e863b2ed 100644 (file)
--- a/src/cache.c
+++ b/src/cache.c
@@ -2661,6 +2661,13 @@ enum act_return http_action_req_cache_use(struct act_rule *rule, struct proxy *p
 
        struct cache_tree *cache_tree = NULL;
 
+       /* The response is already produced by an applet, e.g. after a hit on
+        * a previous cache-use rule. Creating another one would replace it on
+        * the back stream connector, leaking it with its retained cache row.
+        */
+       if (s->target)
+               return ACT_RET_CONT;
+
        /* Ignore cache for HTTP/1.0 requests and for requests other than GET
         * and HEAD */
        if (!(txn->req.flags & HTTP_MSGF_VER_11) ||