BUG/MEDIUM: filters/htx: Fix data forwarding when payload length is unknown

It is only a problem on the response path because the request payload length
it always known. But when a filter is registered to analyze the response
payload, the filtering may hang if the server closes just after the headers.

The root cause of the bug comes from an attempt to allow the filters to not
immediately forward the headers if necessary. A filter may choose to hold
the headers by not forwarding any bytes of the payload. For a message with
no payload but a known payload length, there is always a EOM block to
forward. Thus holding the EOM block for bodyless messages is a good way to
also hold the headers. However, messages with an unknown payload length,
there is no EOM block finishing the message, but only a SHUTR flag on the
channel to mark the end of the stream. If there is no payload when it
happens, there is no payload at all to forward. In the filters API, it is
wrongly detected as a condition to not forward the headers.

Because it is not the most used feature and not the obvious one, this patch
introduces another way to hold the message headers at the begining of the
forwarding. A filter flag is added to explicitly says the headers should be
hold. A filter may choose to set the STRM_FLT_FL_HOLD_HTTP_HDRS flag and not
forwad anything to hold the headers. This flag is removed at each call, thus
it must always be explicitly set by filters. This flag is only evaluated if
no byte has ever been forwarded because the headers are forwarded with the
first byte of the payload.

reg-tests/filters/random-forwarding.vtc reg-test is updated to also test
responses with unknown payload length (with and without payload).

This patch must be backported as far as 2.0.

diff --git a/include/haproxy/filters-t.h b/include/haproxy/filters-t.h
index d2f8c7386891211f7bae38fec378c0b2f01bb949..73fe574da88ced9a26d8538ba54f6df6b325d0bf 100644 (file)
--- a/include/haproxy/filters-t.h
+++ b/include/haproxy/filters-t.h
@@ -33,6 +33,7 @@
 
 /* Flags set on the stream, common to all filters attached to its stream */
 #define STRM_FLT_FL_HAS_FILTERS          0x0001 /* The stream has at least one filter */
+#define STRM_FLT_FL_HOLD_HTTP_HDRS       0x0002 /* At least one filter on the stream want to hold the message headers */
 
 
 struct http_msg;

diff --git a/reg-tests/filters/random-forwarding.vtc b/reg-tests/filters/random-forwarding.vtc
index fdda4e05a0922e91e1ede4babdbb04e95aa5c0e2..2982c880f3b46898b16c43b4ce824c6cca6edd6c 100644 (file)
--- a/reg-tests/filters/random-forwarding.vtc
+++ b/reg-tests/filters/random-forwarding.vtc
@@ -24,6 +24,19 @@ server s1 {
         recv 36000
         send_n 1000 "0123456789abcdefghijklmnopqrstuvwxyz"
         barrier b1 sync
+
+        accept
+        rxreq
+        expect req.url == "/"
+        txresp -nolen \
+          -hdr "Content-Type: text/plain" \
+          -bodylen 20480
+        close
+
+        accept
+        rxreq
+        expect req.url == "/"
+        txresp -nolen
 } -start
 
 haproxy h1 -conf {
@@ -69,3 +82,27 @@ client c1 -connect ${h1_fe1_sock} {
         recv 36000
         barrier b1 sync
 } -run
+
+client c2 -connect ${h1_fe1_sock} {
+        txreq -url "/" \
+          -hdr "Accept-Encoding: gzip" \
+          -hdr "Content-Type: text/plain"
+        rxresp
+        expect resp.status == 200
+        expect resp.http.content-encoding == "<undef>"
+        expect resp.http.transfer-encoding == "<undef>"
+        expect resp.http.content-length == "<undef>"
+        expect resp.bodylen == 20480
+} -run
+
+client c3 -connect ${h1_fe1_sock} {
+        txreq -url "/" \
+          -hdr "Accept-Encoding: gzip" \
+          -hdr "Content-Type: text/plain"
+        rxresp
+        expect resp.status == 200
+        expect resp.http.content-encoding == "<undef>"
+        expect resp.http.transfer-encoding == "<undef>"
+        expect resp.http.content-length == "<undef>"
+        expect resp.bodylen == 0
+} -run

diff --git a/src/filters.c b/src/filters.c
index fd5c56eab699f0b493614b40ecd3e813279c23fa..69ebf525f6aa02e0a4918aefbe3f1a0aea25bd5f 100644 (file)
--- a/src/filters.c
+++ b/src/filters.c
@@ -632,6 +632,8 @@ flt_http_payload(struct stream *s, struct http_msg *msg, unsigned int len)
        unsigned int out = co_data(msg->chn);
        int ret, data;
 
+       strm_flt(s)->flags &= ~STRM_FLT_FL_HOLD_HTTP_HDRS;
+
        ret = data = len - out;
        DBG_TRACE_ENTER(STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA|STRM_EV_FLT_ANA, s, s->txn, msg);
        list_for_each_entry(filter, &strm_flt(s)->filters, list) {
@@ -656,11 +658,22 @@ flt_http_payload(struct stream *s, struct http_msg *msg, unsigned int len)
                }
        }
 
-       /* Only forward data if the last filter decides to forward something */
-       if (ret > 0) {
-               ret = data;
-               *strm_off += ret;
-       }
+       /* If nothing was forwarded yet, we take care to hold the headers if
+        * following conditions are met :
+        *
+        *  - *strm_off == 0 (nothing forwarded yet)
+        *  - ret == 0       (no data forwarded at all on this turn)
+        *  - STRM_FLT_FL_HOLD_HTTP_HDRS flag set (at least one filter want to hold the headers)
+        *
+        * Be careful, STRM_FLT_FL_HOLD_HTTP_HDRS is removed before each http_payload loop.
+        * Thus, it must explicitly be set when necessary. We must do that to hold the headers
+        * when there is no payload.
+        */
+       if (!ret && !*strm_off && (strm_flt(s)->flags & STRM_FLT_FL_HOLD_HTTP_HDRS))
+               goto end;
+
+       ret = data;
+       *strm_off += ret;
  end:
        DBG_TRACE_LEAVE(STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA|STRM_EV_FLT_ANA, s);
        return ret;