BUG/MEDIUM: mux-h1: don't lose error responses once a content-length was parsed

When a request is rejected before its stream is created, an error should be
send by the H1 mux and the embryonic H1 stream is destroyed. But if a
non-null content-length was already parsed, the request was considered as
truncated, discarding the error sending and closing the connection. The
error was logged but not sent.

The regression was introduced by the commit commit 75dd2c18d ("BUG/MEDIUM:
mux-h1: close the connection on a short content-length").

To fix the issue, before destroying an embryonic H1 stream, the current
request and response lengths are reset.

This must be backported wherever commit 75dd2c18d was backported, so as far
as 2.8.

diff --git a/reg-tests/http-messaging/h1_error_after_content_length.vtc b/reg-tests/http-messaging/h1_error_after_content_length.vtc
new file mode 100644 (file)
index 0000000..06f49b2
--- /dev/null
+++ b/reg-tests/http-messaging/h1_error_after_content_length.vtc
@@ -0,0 +1,36 @@
+varnishtest "H1 errors must be sent even when a content-length was parsed"
+
+# A request rejected after a non-null content-length was parsed must still
+# receive its error response instead of a silent close.
+
+feature ignore_unknown_macro
+
+haproxy h1 -conf {
+    defaults
+        mode http
+        timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout client  "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout server  "${HAPROXY_TEST_TIMEOUT-5s}"
+
+    frontend fe1
+        bind "fd@${fe1}"
+        http-request return status 200
+} -start
+
+client c1 -connect ${h1_fe1_sock} {
+       send "POST / HTTP/1.1\r\nHost: a\r\nContent-Length: 5\r\nFoo : x\r\n\r\nhello"
+       rxresp
+       expect resp.status == 400
+} -run
+
+client c2 -connect ${h1_fe1_sock} {
+       send "POST / HTTP/1.1\r\nHost: a\r\nContent-Length: 5\r\nContent-Length: 6\r\n\r\nhello"
+       rxresp
+       expect resp.status == 400
+} -run
+
+client c3 -connect ${h1_fe1_sock} {
+       send "GET / HTTP/1.0\r\nHost: a\r\nContent-Length: 5\r\n\r\nhello"
+       rxresp
+       expect resp.status == 413
+} -run

diff --git a/src/mux_h1.c b/src/mux_h1.c
index 1b2fb6d7e42d62f49a572f7921da89b31a4a1672..18c844856b57814d8a790849fc4c91c79de01c79 100644 (file)
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -3886,6 +3886,13 @@ static int h1_send_error(struct h1c *h1c)
        if (h1c->state == H1_CS_EMBRYONIC) {
                BUG_ON(h1c->h1s == NULL || h1s_sc(h1c->h1s) != NULL);
                TRACE_DEVEL("Abort embryonic H1S", H1_EV_H1C_ERR, h1c->conn, h1c->h1s);
+               /* The request was never forwarded, so a content-length
+                * that may have been parsed must not be considered as a
+                * truncated payload by h1s_destroy(), otherwise the
+                * connection is put in error and the error message is
+                * never sent.
+                */
+               h1c->h1s->req.curr_len = h1c->h1s->res.curr_len = 0;
                h1s_destroy(h1c->h1s);
        }