Re: [PATCH 3/3] BUG/MINOR: haproxy: Free rule->arg.vars.expr during deinit_act_rules
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
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
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
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
[PATCH 3/3] BUG/MINOR: haproxy: Free rule->arg.vars.expr during deinit_act_rules
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.
Best regards
Tim Düsterhus
Apply with `git am --scissors` to automatically cut the commit message.
-- >8 --
Given the following example configuration:
frontend foo
bind *:8080
mode http
http-request set-var(txn.foo) str(bar)
Running a configuration check within valgrind reports:
==23665== Memcheck, a memory error detector
==23665== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==23665== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==23665== Command: ./haproxy -c -f ./crasher.cfg
==23665==
[WARNING] 165/002941 (23665) : config : missing timeouts for frontend 'foo'.
| While not properly invalid, you will certainly encounter various
problems
| with such a configuration. To fix this, please ensure that all
following
| timeouts are set to a non-zero value: 'client', 'connect', 'server'.
Warnings were found.
Configuration file is valid
==23665==
==23665== HEAP SUMMARY:
==23665== in use at exit: 314,008 bytes in 87 blocks
==23665== total heap usage: 160 allocs, 73 frees, 1,448,074 bytes
allocated
==23665==
==23665== 132 (48 direct, 84 indirect) bytes in 1 blocks are definitely
lost in loss record 15 of 28
==23665==at 0x4C2FB55: calloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==23665==by 0x4A2612: sample_parse_expr (sample.c:876)
==23665==by 0x54DF84: parse_store (vars.c:766)
==23665==by 0x528BDF: parse_http_req_cond (http_rules.c:95)
==23665==by 0x469F36: cfg_parse_listen (cfgparse-listen.c:1339)
==23665==by 0x459E33: readcfgfile (cfgparse.c:2167)
==23665==by 0x5074FD: init (haproxy.c:2021)
==23665==by 0x418262: main (haproxy.c:3126)
==23665==
==23665== LEAK SUMMARY:
==23665==definitely lost: 48 bytes in 1 blocks
==23665==indirectly lost: 84 bytes in 2 blocks
==23665== possibly lost: 0 bytes in 0 blocks
==23665==still reachable: 313,876 bytes in 84 blocks
==23665== suppressed: 0 bytes in 0 blocks
==23665== Reachable blocks (those to which a pointer was found) are not
shown.
==23665== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==23665==
==23665== For counts of detected and suppressed errors, rerun with: -v
==23665== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
After this patch is applied the leak is gone as expected.
This is a very minor leak that can only be observed if deinit() is called,
shortly before the OS will free all memory of the process anyway. No
backport needed.
---
src/haproxy.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/haproxy.c b/src/haproxy.c
index 6548db6b5..245ac3b60 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2555,6 +2555,7 @@ static void deinit_act_rules(struct list *rules)
list_for_each_entry_safe(rule, ruleb, rules, list) {
LIST_DEL(>list);
deinit_acl_cond(rule->cond);
+ release_sample_expr(rule->arg.vars.expr);
if (rule->release_ptr)
rule->release_ptr(rule);
free(rule);
--
2.27.0
