Re: [PATCH 3/3] BUG/MINOR: haproxy: Free rule->arg.vars.expr during deinit_act_rules

2020-06-14 Thread Tim Düsterhus
William,

Am 14.06.20 um 16:59 schrieb Tim Düsterhus:
> I can reproduce this with the following config:
> 
> frontend http
> mode http
> bind 127.0.0.1:80
> 
> http-request redirect scheme https if METH_GET
> 
>> $ valgrind ./haproxy -c -f ./crasher.cfg
>> [...]
>> ==6484== Invalid read of size 8
>> ==6484==at 0x4C4747: release_sample_expr (sample.c:1427)
>> ==6484==by 0x525A43: deinit_act_rules (haproxy.c:2558)
>> ==6484==by 0x5271CA: deinit (haproxy.c:2706)
>> ==6484==by 0x528017: deinit_and_exit (haproxy.c:2871)
>> ==6484==by 0x528E90: init (haproxy.c:2205)
>> ==6484==by 0x41F382: main (haproxy.c:3127)
>> ==6484==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
> 
> I would assume that this is related to METH_GET being a default acl.

I spoke too soon. I can even remove the `if METH_GET` and it segfaults
for a simple `http-request redirect scheme https`.

Best regards
Tim Düsterhus



Re: [PATCH 3/3] BUG/MINOR: haproxy: Free rule->arg.vars.expr during deinit_act_rules

2020-06-14 Thread Tim Düsterhus
William,

Am 14.06.20 um 16:28 schrieb William Dauchy:
> After this patch, I'm getting a segfault after firing an USR1 signal
> to trigger the deinit:
> 
> #0  0x55b653aaec34 in release_sample_expr (expr=0x55b654766bc0) at
> src/sample.c:1427
> b1427   list_for_each_entry_safe(conv_expr, conv_exprb,
> >conv_exprs, list)
> (gdb) bt
> #0  0x55b653aaec34 in release_sample_expr (expr=0x55b654766bc0) at
> src/sample.c:1427
> #1  0x55b653b0d884 in deinit_act_rules (rules=0x55b6547644b0) at
> src/haproxy.c:2559
> #2  0x55b653b0f09d in deinit () at src/haproxy.c:2707
> #3  0x55b653b0fef8 in deinit_and_exit (status=0) at src/haproxy.c:2872
> #4  0x55b6539f7256 in main (argc=, argv= out>) at src/haproxy.c:3771
> 

I can reproduce this with the following config:

frontend http
mode http
bind 127.0.0.1:80

http-request redirect scheme https if METH_GET

> $ valgrind ./haproxy -c -f ./crasher.cfg
> [...]
> ==6484== Invalid read of size 8
> ==6484==at 0x4C4747: release_sample_expr (sample.c:1427)
> ==6484==by 0x525A43: deinit_act_rules (haproxy.c:2558)
> ==6484==by 0x5271CA: deinit (haproxy.c:2706)
> ==6484==by 0x528017: deinit_and_exit (haproxy.c:2871)
> ==6484==by 0x528E90: init (haproxy.c:2205)
> ==6484==by 0x41F382: main (haproxy.c:3127)
> ==6484==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

I would assume that this is related to METH_GET being a default acl.

Best regards
Tim Düsterhus



Re: [PATCH 3/3] BUG/MINOR: haproxy: Free rule->arg.vars.expr during deinit_act_rules

2020-06-14 Thread William Dauchy
Hello,

On Sun, Jun 14, 2020 at 7:40 AM Willy Tarreau  wrote:
> On Sun, Jun 14, 2020 at 12:37:43AM +0200, Tim Duesterhus wrote:
> > careful with this one: I don't know whether it's safe to simply free the
> > expression there or whether I need to somehow check whether there actually
> > is some expression.
> >
> > It does not crash with my stupid example configuration showcasing the leak,
> > but of course real world configurations might or might not trigger a bogus
> > free there.
>
> It seems to be OK. I was worried that the pointer could be part of a union
> containing either the expression or its text version during parsing, but
> this doesn't seem to be the case, so apparently it's OK to always free it
> if it appears in a rule.

After this patch, I'm getting a segfault after firing an USR1 signal
to trigger the deinit:

#0  0x55b653aaec34 in release_sample_expr (expr=0x55b654766bc0) at
src/sample.c:1427
b1427   list_for_each_entry_safe(conv_expr, conv_exprb,
>conv_exprs, list)
(gdb) bt
#0  0x55b653aaec34 in release_sample_expr (expr=0x55b654766bc0) at
src/sample.c:1427
#1  0x55b653b0d884 in deinit_act_rules (rules=0x55b6547644b0) at
src/haproxy.c:2559
#2  0x55b653b0f09d in deinit () at src/haproxy.c:2707
#3  0x55b653b0fef8 in deinit_and_exit (status=0) at src/haproxy.c:2872
#4  0x55b6539f7256 in main (argc=, argv=) at src/haproxy.c:3771

Here is my config:

global
log 127.0.0.1 format rfc5424 local0 info
daemon
stats timeout 2m
set-dumpable
nbproc 1
tune.bufsize 33792
ssl-server-verify none
nbthread 16
cpu-map auto:1/1-16 0-15
spread-checks 5
strict-limits
no busy-polling

defaults
mode http
log global
option httplog
option http-keep-alive
option forwardfor except 127.0.0.0/8
option redispatch 1
option http-ignore-probes
retries 3
retry-on conn-failure empty-response response-timeout 0rtt-rejected
timeout http-request 10s
timeout queue 1s
timeout connect 10s
timeout client 180s
timeout server 180s
timeout http-keep-alive 10s
timeout check 5s
balance roundrobin
http-reuse always
default-server inter 5s fastinter 1s fall 3 slowstart 20s observe
layer7 error-limit 5 on-error fail-check pool-purge-delay 10s tfo
http-check expect status 200

listen stats
bind *:8080
stats enable
stats uri /haproxy_stats
http-request use-service prometheus-exporter if { path /metrics }
monitor-uri /

frontend fe_foo
bind 127.0.0.1:80 name http_ip4 process 1/all tfo

acl http  ssl_fc,not
acl https ssl_fc
monitor-uri /delivery/monitor/lb-check
# errorfile 200 /etc/haproxy/errorfiles/200.http
tcp-request content capture fc_rtt len 10
capture request header Host len 64
capture request header user-agent len 128
capture request header cf-ray len 20
log-format "%tr %TR/%Tw/%Ta %ac/%fc/%bc/%sc/%rc %sq/%bq %{+Q}r"
log-format-sd [user@51719\ src_ip=\"%ci\"\ src_port=\"%cp\"\
ftd=\"%f\"\ bkd=\"%b\"\ srv=\"%si:%sp\"\ status=\"%ST\"\
bytes_r=\"%B\"\ tsc=\"%tsc\"\ sslv=\"%sslv\"\ sslc=\"%sslc\"\
h_host=\"%[capture.req.hdr(1)]\"\ meth=\"%HM\"\ version=\"%HV\"\
h_ua=\"%[capture.req.hdr(2)]\"\
cf_dc=\"%[capture.req.hdr(3),word(2,-)]\"\
fc_rtt=\"%[capture.req.hdr(0)]\"\ time_tr=\"%Tr\"\ time_tc=\"%Tc\"\
time_tt=\"%Tt\"]
http-response set-log-level silent if { rand(100) ge 1 }

http-request del-header connection if { req.hdr(connection) close }
http-request set-header x-proto ssl if https
http-request set-header x-forwarded-proto https if https
http-request set-header x-tls-sessionid %[ssl_fc_session_id,hex] if https
http-request add-header x-client-ip %[src]
http-request set-header client-ip %[src]

http-request set-header x-real-host %[req.hdr(host)]
http-request set-header x-server-address %[dst]
acl content_encoding_header_exists req.hdr(content-encoding) -m found
http-request set-header x-original-content-encoding
%[req.hdr(content-encoding)] if content_encoding_header_exists
acl host_header_exists req.hdr(host) -m found
http-request set-header host %[req.hdr(host),field(1,:)] if
host_header_exists
acl ::disabled_http_methods method CONNECT
http-request deny if ::disabled_http_methods
http-request disable-l7-retry if ! METH_GET
http-request redirect scheme https code 302 if !{ ssl_fc } METH_GET
http-request redirect scheme https code 307 if !{ ssl_fc } ! METH_GET

default_backend be_foo

backend be_foo
   server srv0 127.0.0.1:6011 weight 1

-- 
William



Re: [PATCH 3/3] BUG/MINOR: haproxy: Free rule->arg.vars.expr during deinit_act_rules

2020-06-13 Thread Willy Tarreau
Hi Tim,

On Sun, Jun 14, 2020 at 12:37:43AM +0200, Tim Duesterhus wrote:
> Willy,
> 
> careful with this one: I don't know whether it's safe to simply free the
> expression there or whether I need to somehow check whether there actually
> is some expression.
> 
> It does not crash with my stupid example configuration showcasing the leak,
> but of course real world configurations might or might not trigger a bogus
> free there.

It seems to be OK. I was worried that the pointer could be part of a union
containing either the expression or its text version during parsing, but
this doesn't seem to be the case, so apparently it's OK to always free it
if it appears in a rule.

Thanks!
Willy