Hi Jean, I finally found it by carefully unrolling the execution based on your core. It's a regression introduced in 1.7 while fixing a problem of missing cookies in logs when doing a tarpit... I could finally reproduce it and fix it with the attached patch.
Thanks a lot for all the details you provided! Willy
>From 6a0bca9e7862984b0edf8fc1e1edc54295a7a5e2 Mon Sep 17 00:00:00 2001 From: Willy Tarreau <[email protected]> Date: Sun, 11 Jun 2017 17:56:27 +0200 Subject: 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. --- src/proto_http.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/proto_http.c b/src/proto_http.c index 357401f..a72f302 100644 --- 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... */ -- 1.7.12.1
