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.

Good catch, thank you!
Willy

Reply via email to