BUG/MAJOR: http: call manage_client_side_cookies() before erasing the buffer

Jean Lubatti reported a crash on haproxy using a config involving cookies
and tarpit rules. It just happens that since 1.7-dev3 with commit 83a2c3d
("BUG/MINOR : allow to log cookie for tarpit and denied request"), function
manage_client_side_cookies() was called after erasing the request buffer in
case of a tarpit action. The problem is that this function must absolutely
not be called with an empty buffer since it moves parts of it. A typical
reproducer consists in sending :

    "GET / HTTP/1.1\r\nCookie: S=1\r\n\r\n"

On such a config :

    listen crash
        bind :8001
        mode http
        reqitarpit .
        cookie S insert indirect
        server s1 127.0.0.1:8000 cookie 1

The fix simply consists in moving the call to the function before the call
to buffer_erase().

Many thanks to Jean for testing instrumented code and providing a usable
core.

This fix must be backported to all stable versions since the fix introducing
this bug was backported as well.

diff --git a/src/proto_http.c b/src/proto_http.c
index 357401f..a72f302 100644 (file)
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -4462,6 +4462,11 @@ int http_process_req_common(struct stream *s, struct channel *req, int an_bit, s
        return 1;
 
  tarpit:
+       /* Allow cookie logging
+        */
+       if (s->be->cookie_name || sess->fe->capture_name)
+               manage_client_side_cookies(s, req);
+
        /* When a connection is tarpitted, we use the tarpit timeout,
         * which may be the same as the connect timeout if unspecified.
         * If unset, then set it to zero because we really want it to
@@ -4474,11 +4479,6 @@ int http_process_req_common(struct stream *s, struct channel *req, int an_bit, s
         */
        channel_dont_connect(req);
 
-       /* Allow cookie logging
-        */
-       if (s->be->cookie_name || sess->fe->capture_name)
-               manage_client_side_cookies(s, req);
-
        txn->status = http_err_codes[deny_status];
 
        req->analysers &= AN_REQ_FLT_END; /* remove switching rules etc... */