BUG/MEDIUM: connection: fix an infinite loop in the fc_pp_tlv() fetch

fc_pp_tlv() is a multi-value fetch: it reports one TLV per call, sets
SMP_F_NOT_LAST and stores the TLV it returned in smp->ctx.p so that the next
call resumes from there. But it resumes *on* that TLV rather than after it,
and list_for_each_entry_from() starts on the element it is given, so the same
TLV matches again and is returned again, with SMP_F_NOT_LAST set again.

The ACL engine asks for the next occurrence as long as the fetch reports one
(the "fetch_next" retry in acl_exec_cond()), so an ACL whose pattern does not
match a TLV which is present never terminates: the thread spins at 100% and
the request is never answered. A configuration as simple as

    http-request deny if { fc_pp_tlv(0xE0) -m str nomatch }

hangs as soon as a client sends that TLV on any frontend accepting PROXY
protocol v2.

The same mistake also means the fetch never walked the list at all: only the
first matching TLV was ever visible, so a rule matching any later occurrence
of a repeated TLV silently never matched.

Resume from the element following the one returned previously. When it was
the last one, the resulting pointer is the list head and the loop stops right
away, as it should.

The regtest now sends the same TLV type three times and matches on the last
one, and checks that a pattern matching none of the occurrences terminates.
Without this patch that test hangs until the client times out.

This should fix GitHub issue #3482. Thanks to Annika Wickert for reporting
it!

This should be backported to all stable branches where fc_pp_tlv() exists.

diff --git a/reg-tests/sample_fetches/tlvs.vtc b/reg-tests/sample_fetches/tlvs.vtc
index 625f729e42174de39f2f9dd43cd229f8fb07609e..d75473b6d1edc9353fb09d2dfa542cb196f5317a 100644 (file)
--- a/reg-tests/sample_fetches/tlvs.vtc
+++ b/reg-tests/sample_fetches/tlvs.vtc
@@ -22,6 +22,21 @@ haproxy h1 -conf {
         http-after-response set-header echo2 %[var(sess.azure)]
         http-after-response set-header echo3 %[fc_pp_tlv(0xEB)]
         http-after-response set-header echo4 %[fc_pp_tlv(0xEC),length]
+
+        # A pattern that matches none of the occurrences of a TLV which is
+        # present must terminate: the ACL engine keeps asking the fetch for the
+        # next occurrence as long as it reports one, so a fetch that keeps
+        # returning the same TLV loops forever and the request never completes.
+        tcp-request content set-var(sess.nomatch) str(no)
+        tcp-request content set-var(sess.nomatch) str(yes) if { fc_pp_tlv(0xEB) -m str does-not-match }
+        http-after-response set-header echo5 %[var(sess.nomatch)]
+
+        # 0xED is sent three times below: matching on the last value proves the
+        # fetch walks the occurrences instead of repeating the first one.
+        tcp-request content set-var(sess.third) str(no)
+        tcp-request content set-var(sess.third) str(yes) if { fc_pp_tlv(0xED) -m str third }
+        http-after-response set-header echo6 %[var(sess.third)]
+
         http-request return status 200
 } -start
 
@@ -32,8 +47,8 @@ client c1 -connect ${h1_fe1_sock} {
     sendhex "21"
     # TCP4
     sendhex "11"
-    # length of the address (12) + length of the TLVs (14 + 10 + 9 + 131)
-    sendhex "00 B0"
+    # length of the address (12) + length of the TLVs (14 + 10 + 9 + 131 + 3 * 8)
+    sendhex "00 C8"
     # 127.0.0.1 42 127.0.0.1 1337
     sendhex "7F 00 00 01 7F 00 00 01 00 2A 05 39"
 
@@ -52,6 +67,11 @@ client c1 -connect ${h1_fe1_sock} {
     # custom type (0xEC) + length of the value (128, does not fit in pool) + random data
     sendhex "EC 00 80 3A D9 32 9B 11 A7 29 81 14 B2 33 F0 C2 0D 7A 53 D1 97 28 74 4B 78 8A D3 10 C4 B1 88 42 9C 63 8E 8B 8A A0 B4 B0 E7 9D 20 27 0F 1E 53 4D 33 F7 5A D0 91 3F B8 C9 E9 16 C4 61 C5 13 02 92 64 9D D4 22 5C 8E 4E 0B 2D 2D 7D 9F 5D 97 9B 25 C4 12 7D 21 75 C8 15 92 6B 64 F2 5F C0 A9 0F 9A 7D 0A 6D 68 79 F4 56 18 6F 23 45 2A 9B 36 34 3A 47 43 32 29 18 6F 23 45 2A 9B 36 34 3A 47 43 32 29 32 29"
 
+    # custom type (0xED) sent three times: "first", "secnd", "third"
+    sendhex "ED 00 05 66 69 72 73 74"
+    sendhex "ED 00 05 73 65 63 6E 64"
+    sendhex "ED 00 05 74 68 69 72 64"
+
     txreq -url "/"
     rxresp
     expect resp.status == 200
@@ -59,4 +79,6 @@ client c1 -connect ${h1_fe1_sock} {
     expect resp.http.echo2 == "LINKID"
     expect resp.http.echo3 == "custom"
     expect resp.http.echo4 == 128
+    expect resp.http.echo5 == "no"
+    expect resp.http.echo6 == "yes"
 } -run

diff --git a/src/connection.c b/src/connection.c
index 9ebbff286c461cdaf1d7540f6e15b5af1f27f941..4b4340724fbd8b2a0cdba438aaab1cbf4e2f9b7a 100644 (file)
--- a/src/connection.c
+++ b/src/connection.c
@@ -2728,7 +2728,16 @@ int smp_fetch_fc_pp_tlv(const struct arg *args, struct sample *smp, const char *
                return 0;
 
        idx = args[0].data.sint;
-       conn_tlv = smp->ctx.p ? smp->ctx.p : LIST_ELEM(conn->tlv_list.n, struct conn_tlv_list *, list);
+       conn_tlv = smp->ctx.p;
+       if (conn_tlv) {
+               /* resume *after* the TLV returned on the previous call:
+                * list_for_each_entry_from() starts on the element it is given, so
+                * resuming on it would match it again and return it forever
+                */
+               conn_tlv = LIST_NEXT(&conn_tlv->list, struct conn_tlv_list *, list);
+       }
+       else
+               conn_tlv = LIST_ELEM(conn->tlv_list.n, struct conn_tlv_list *, list);
        list_for_each_entry_from(conn_tlv, &conn->tlv_list, list) {
                if (conn_tlv->type == idx) {
                        smp->flags |= SMP_F_NOT_LAST;