Le 01/08/2019 à 06:17, Willy Tarreau a écrit :
Hi Richard,

On Wed, Jul 31, 2019 at 02:03:26PM -0700, Richard Russo wrote:
I've attached a patch to fix a bug in http sampling on raw connections. This
fixes crashes experienced with a frontend configuration similar to:

frontend haproxy_v4_http_0
   mode tcp
   bind ipv4@:80
   default_backend chat
   tcp-request inspect-delay 60s
   tcp-request content reject unless { req_len gt 0 }
   acl example_com req.hdr(host) -m str example.com
   acl example_org req.hdr(host) -m str example.org
   acl example_net req.hdr(host) -m str example.net
   acl chat path -m str /chat
   acl post method -m str POST
   use_backend chat if chat post # explicitly send chunked transfer encoded 
chat to chatd
   use_backend cdn if example_com
   use_backend cdn if example_org
   use_backend cdn if example_net

If the host header in inbound requests is not example.com, the check for
example.org will use the wrong trash buffer, and sometimes results in
crashes. I wasn't able to get a clear crash reproduction, but the call to
htx_get_next_blk() in src/http_htx.c:http_find_header would be called with
blk way out of range, and the loop will walk memory until it happens to find
an exit condition, or it reads unmapped memory and segfaults. Most often, it
seems the wrong trash buffer still has the old htx buffer, so it usually
doesn't crash.

I found the blk pointer was out of range in my core dumps, and added logging
in htx_get_next when pos > htx->tail; from there it was pretty easy to figure
out what happened.

I solved this by using a thread local static buffer for the raw buffer to htx
conversion, but there's probably other ways to fix it.

I think your patch is right. I'll let Christopher check it and merge it,
but indeed there's a problem here since buffers allocated using
get_trash_chunk() are just for local use and should never be passed
across functions since they will be reassigned.


It is good to me too. However, I will slightly amend it to use per-thread init/deinit callbacks. This way, valgrind lovers will not be hurt... or at least, not more than before :)

Thanks Richard !
--
Christopher Faulet

Reply via email to