Hi,
thanks for the bug repport. I already encoutered with another function
than redirect. Can you try the join patch ?
Thierry
On Fri, 27 Jan 2017 22:50:00 +0000
Jesse Schulman <je...@dreamtsoft.com> wrote:
> I've found what seems to be a bug when I log from within a Lua sample fetch
> that I am using to determine a redirect URL. It seems that whatever is
> logged from the lua script is written to the log file as expected, but it
> also is replacing the response, making the response invalid and breaking
> the redirection.
>
> Thanks,
> Jesse
>
> Here's what I'm seeing:
>
> *no logging: curl -v http://lab.mysite.com <http://lab.mysite.com>*
> > GET / HTTP/1.1
> > Host: lab.mysite.com
> > User-Agent: curl/7.51.0
> > Accept: */*
> >
> < HTTP/1.1 302 Found
> < Cache-Control: no-cache
> < Content-length: 0
> < Location: https://www.google.com/
> < Connection: close
> <
>
> *issue seen here with logging the string "LOG MSG" from lua script: curl -v
> http://lab.mysite.com/log <http://lab.mysite.com/log>*
> > GET /log HTTP/1.1
> > Host: lab.mysite.com
> > User-Agent: curl/7.51.0
> > Accept: */*
> >
> LOG MSG 302 Found
> Cache-Control: no-cache
> Content-length: 0
> Location: https://www.google.com/log
> Connection: close
>
>
> Here are steps to reproduce and my current setup:
>
> */etc/redhat-release:*
> CentOS Linux release 7.2.1511 (Core)
>
> *uname -rv*
> 3.10.0-327.28.3.el7.x86_64 #1 SMP Thu Aug 18 19:05:49 UTC 2016
>
> *haproxy -vv:*
> HA-Proxy version 1.7.2 2017/01/13
> Copyright 2000-2017 Willy Tarreau <wi...@haproxy.org>
>
> Build options :
> TARGET = linux2628
> CPU = generic
> CC = gcc
> CFLAGS = -O2 -g -fno-strict-aliasing -Wdeclaration-after-statement
> OPTIONS = USE_LINUX_TPROXY=1 USE_ZLIB=1 USE_REGPARM=1 USE_OPENSSL=1
> USE_LUA=1 USE_PCRE=1
>
> Default settings :
> maxconn = 2000, bufsize = 16384, maxrewrite = 1024, maxpollevents = 200
>
> Encrypted password support via crypt(3): yes
> Built with zlib version : 1.2.7
> Running on zlib version : 1.2.7
> Compression algorithms supported : identity("identity"),
> deflate("deflate"), raw-deflate("deflate"), gzip("gzip")
> Built with OpenSSL version : OpenSSL 1.0.1e-fips 11 Feb 2013
> Running on OpenSSL version : OpenSSL 1.0.1e-fips 11 Feb 2013
> OpenSSL library supports TLS extensions : yes
> OpenSSL library supports SNI : yes
> OpenSSL library supports prefer-server-ciphers : yes
> Built with PCRE version : 8.32 2012-11-30
> Running on PCRE version : 8.32 2012-11-30
> PCRE library supports JIT : no (USE_PCRE_JIT not set)
> Built with Lua version : Lua 5.3.3
> Built with transparent proxy support using: IP_TRANSPARENT IPV6_TRANSPARENT
> IP_FREEBIND
>
> Available polling systems :
> epoll : pref=300, test result OK
> poll : pref=200, test result OK
> select : pref=150, test result OK
> Total: 3 (3 usable), will use epoll.
>
> Available filters :
> [COMP] compression
> [TRACE] trace
> [SPOE] spoe
>
> *haproxy.cfg:*
> global
> log 127.0.0.1 local2 debug
> lua-load /etc/haproxy/lua/redirect.lua
> chroot /var/lib/haproxy
> pidfile /var/run/haproxy.pid
> maxconn 256
> tune.ssl.default-dh-param 1024
> stats socket /var/run/haproxy.sock mode 600 level admin
> stats timeout 2m #Wait up to 2 minutes for input
> user haproxy
> group haproxy
> daemon
>
> defaults
> log global
> mode tcp
> option tcplog
> option dontlognull
> timeout connect 10s
> timeout client 60s
> timeout server 60s
> timeout tunnel 600s
>
> frontend http
> bind "${BIND_IP}:80"
> mode http
> option httplog
> option forwardfor
> capture request header Host len 32
> log-format %hr\ %r\ %ST\ %b/%s\ %ci:%cp\ %B\ %Tr
>
> http-request redirect prefix "%[lua.get_redirect()]"
>
> *lua/redirect.lua:*
> core.register_fetches("get_redirect", function(txn)
> local path = txn.sf:path()
> if (path == "/log") then
> core.Info("LOG MSG")
> end
> return "https://www.google.com";
>
> end)
>From 64f2cf8ecc4c19e58fc1d8f8fb04c7b90cc97427 Mon Sep 17 00:00:00 2001
From: Thierry FOURNIER <thierry.fourn...@ozon.io>
Date: Sat, 28 Jan 2017 07:39:53 +0100
Subject: [PATCH] BUG/MEDIUM: http: redirect overwrite a buffer
X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4
see 4b788f7d349ddde3f70f063b7394529eac6ab678
If we use the action "http-request redirect" with a Lua sample-fetch or
converter, and the Lua function calls one of the Lua log function, the
header name is corrupted, it contains an extract of the last loggued data.
This is due to an overwrite of the trash buffer, because his scope is not
respected in the "add-header" function. The scope of the trash buffer must
be limited to the function using it. The build_logline() function can
execute a lot of other function which can use the trash buffer.
This patch fix the usage of the trash buffer. It limits the scope of this
global buffer to the local function, we build first the header value using
build_logline, and after we store the header name.
Thanks Jesse Schulman for the bug repport.
This patch must be backported in 1.7, 1.6 and 1.5 version
---
src/proto_http.c | 103 ++++++++++++++++++++++++++++--------------------------
1 file changed, 53 insertions(+), 50 deletions(-)
diff --git a/src/proto_http.c b/src/proto_http.c
index f7f7545..7506a14 100644
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -4024,6 +4024,9 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s
struct http_msg *res = &txn->rsp;
const char *msg_fmt;
const char *location;
+ struct chunk *chunk;
+
+ chunk = get_trash_chunk();
/* build redirect message */
switch(rule->code) {
@@ -4045,10 +4048,10 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s
break;
}
- if (unlikely(!chunk_strcpy(&trash, msg_fmt)))
+ if (unlikely(!chunk_strcpy(chunk, msg_fmt)))
return 0;
- location = trash.str + trash.len;
+ location = chunk->str + chunk->len;
switch(rule->type) {
case REDIRECT_TYPE_SCHEME: {
@@ -4087,40 +4090,40 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s
if (rule->rdr_str) { /* this is an old "redirect" rule */
/* check if we can add scheme + "://" + host + path */
- if (trash.len + rule->rdr_len + 3 + hostlen + pathlen > trash.size - 4)
+ if (chunk->len + rule->rdr_len + 3 + hostlen + pathlen > chunk->size - 4)
return 0;
/* add scheme */
- memcpy(trash.str + trash.len, rule->rdr_str, rule->rdr_len);
- trash.len += rule->rdr_len;
+ memcpy(chunk->str + chunk->len, rule->rdr_str, rule->rdr_len);
+ chunk->len += rule->rdr_len;
}
else {
/* add scheme with executing log format */
- trash.len += build_logline(s, trash.str + trash.len, trash.size - trash.len, &rule->rdr_fmt);
+ chunk->len += build_logline(s, chunk->str + chunk->len, chunk->size - chunk->len, &rule->rdr_fmt);
/* check if we can add scheme + "://" + host + path */
- if (trash.len + 3 + hostlen + pathlen > trash.size - 4)
+ if (chunk->len + 3 + hostlen + pathlen > chunk->size - 4)
return 0;
}
/* add "://" */
- memcpy(trash.str + trash.len, "://", 3);
- trash.len += 3;
+ memcpy(chunk->str + chunk->len, "://", 3);
+ chunk->len += 3;
/* add host */
- memcpy(trash.str + trash.len, host, hostlen);
- trash.len += hostlen;
+ memcpy(chunk->str + chunk->len, host, hostlen);
+ chunk->len += hostlen;
/* add path */
- memcpy(trash.str + trash.len, path, pathlen);
- trash.len += pathlen;
+ memcpy(chunk->str + chunk->len, path, pathlen);
+ chunk->len += pathlen;
/* append a slash at the end of the location if needed and missing */
- if (trash.len && trash.str[trash.len - 1] != '/' &&
+ if (chunk->len && chunk->str[chunk->len - 1] != '/' &&
(rule->flags & REDIRECT_FLAG_APPEND_SLASH)) {
- if (trash.len > trash.size - 5)
+ if (chunk->len > chunk->size - 5)
return 0;
- trash.str[trash.len] = '/';
- trash.len++;
+ chunk->str[chunk->len] = '/';
+ chunk->len++;
}
break;
@@ -4149,7 +4152,7 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s
}
if (rule->rdr_str) { /* this is an old "redirect" rule */
- if (trash.len + rule->rdr_len + pathlen > trash.size - 4)
+ if (chunk->len + rule->rdr_len + pathlen > chunk->size - 4)
return 0;
/* add prefix. Note that if prefix == "/", we don't want to
@@ -4157,30 +4160,30 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s
* configure a self-redirection.
*/
if (rule->rdr_len != 1 || *rule->rdr_str != '/') {
- memcpy(trash.str + trash.len, rule->rdr_str, rule->rdr_len);
- trash.len += rule->rdr_len;
+ memcpy(chunk->str + chunk->len, rule->rdr_str, rule->rdr_len);
+ chunk->len += rule->rdr_len;
}
}
else {
/* add prefix with executing log format */
- trash.len += build_logline(s, trash.str + trash.len, trash.size - trash.len, &rule->rdr_fmt);
+ chunk->len += build_logline(s, chunk->str + chunk->len, chunk->size - chunk->len, &rule->rdr_fmt);
/* Check length */
- if (trash.len + pathlen > trash.size - 4)
+ if (chunk->len + pathlen > chunk->size - 4)
return 0;
}
/* add path */
- memcpy(trash.str + trash.len, path, pathlen);
- trash.len += pathlen;
+ memcpy(chunk->str + chunk->len, path, pathlen);
+ chunk->len += pathlen;
/* append a slash at the end of the location if needed and missing */
- if (trash.len && trash.str[trash.len - 1] != '/' &&
+ if (chunk->len && chunk->str[chunk->len - 1] != '/' &&
(rule->flags & REDIRECT_FLAG_APPEND_SLASH)) {
- if (trash.len > trash.size - 5)
+ if (chunk->len > chunk->size - 5)
return 0;
- trash.str[trash.len] = '/';
- trash.len++;
+ chunk->str[chunk->len] = '/';
+ chunk->len++;
}
break;
@@ -4188,29 +4191,29 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s
case REDIRECT_TYPE_LOCATION:
default:
if (rule->rdr_str) { /* this is an old "redirect" rule */
- if (trash.len + rule->rdr_len > trash.size - 4)
+ if (chunk->len + rule->rdr_len > chunk->size - 4)
return 0;
/* add location */
- memcpy(trash.str + trash.len, rule->rdr_str, rule->rdr_len);
- trash.len += rule->rdr_len;
+ memcpy(chunk->str + chunk->len, rule->rdr_str, rule->rdr_len);
+ chunk->len += rule->rdr_len;
}
else {
/* add location with executing log format */
- trash.len += build_logline(s, trash.str + trash.len, trash.size - trash.len, &rule->rdr_fmt);
+ chunk->len += build_logline(s, chunk->str + chunk->len, chunk->size - chunk->len, &rule->rdr_fmt);
/* Check left length */
- if (trash.len > trash.size - 4)
+ if (chunk->len > chunk->size - 4)
return 0;
}
break;
}
if (rule->cookie_len) {
- memcpy(trash.str + trash.len, "\r\nSet-Cookie: ", 14);
- trash.len += 14;
- memcpy(trash.str + trash.len, rule->cookie_str, rule->cookie_len);
- trash.len += rule->cookie_len;
+ memcpy(chunk->str + chunk->len, "\r\nSet-Cookie: ", 14);
+ chunk->len += 14;
+ memcpy(chunk->str + chunk->len, rule->cookie_str, rule->cookie_len);
+ chunk->len += rule->cookie_len;
}
/* add end of headers and the keep-alive/close status.
@@ -4230,17 +4233,17 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s
/* keep-alive possible */
if (!(req->flags & HTTP_MSGF_VER_11)) {
if (unlikely(txn->flags & TX_USE_PX_CONN)) {
- memcpy(trash.str + trash.len, "\r\nProxy-Connection: keep-alive", 30);
- trash.len += 30;
+ memcpy(chunk->str + chunk->len, "\r\nProxy-Connection: keep-alive", 30);
+ chunk->len += 30;
} else {
- memcpy(trash.str + trash.len, "\r\nConnection: keep-alive", 24);
- trash.len += 24;
+ memcpy(chunk->str + chunk->len, "\r\nConnection: keep-alive", 24);
+ chunk->len += 24;
}
}
- memcpy(trash.str + trash.len, "\r\n\r\n", 4);
- trash.len += 4;
- FLT_STRM_CB(s, flt_http_reply(s, txn->status, &trash));
- bo_inject(res->chn, trash.str, trash.len);
+ memcpy(chunk->str + chunk->len, "\r\n\r\n", 4);
+ chunk->len += 4;
+ FLT_STRM_CB(s, flt_http_reply(s, txn->status, chunk));
+ bo_inject(res->chn, chunk->str, chunk->len);
/* "eat" the request */
bi_fast_delete(req->chn->buf, req->sov);
req->next -= req->sov;
@@ -4255,13 +4258,13 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s
} else {
/* keep-alive not possible */
if (unlikely(txn->flags & TX_USE_PX_CONN)) {
- memcpy(trash.str + trash.len, "\r\nProxy-Connection: close\r\n\r\n", 29);
- trash.len += 29;
+ memcpy(chunk->str + chunk->len, "\r\nProxy-Connection: close\r\n\r\n", 29);
+ chunk->len += 29;
} else {
- memcpy(trash.str + trash.len, "\r\nConnection: close\r\n\r\n", 23);
- trash.len += 23;
+ memcpy(chunk->str + chunk->len, "\r\nConnection: close\r\n\r\n", 23);
+ chunk->len += 23;
}
- http_reply_and_close(s, txn->status, &trash);
+ http_reply_and_close(s, txn->status, chunk);
req->chn->analysers &= AN_REQ_FLT_END;
}
--
1.7.10.4
