Le 01/08/2019 à 11:17, Christopher Faulet a écrit :
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 !

Merged now and backported to 2.0 and 1.9.

Christopher Faulet

Reply via email to