Re: VTest does not test deinit

2020-06-14 Thread Илья Шипицин
Lots of changes before 2.2 release :)

On Sun, Jun 14, 2020, 9:24 PM Tim Düsterhus  wrote:

> Hi List,
> Willy,
> Ilya,
>
> I noticed that the reg-tests were unable find the issue reported by
> William here:
> https://www.mail-archive.com/haproxy@formilux.org/msg37637.html
>
> This is because VTest never performs a "soft" shutdown of HAProxy,
> instead it uses SIGINT -> SIGTERM -> SIGKILL. Thus the deinit() will
> never trigger.
>
> Fixing this will require a change to VTest:
>
> https://github.com/vtest/VTest/blob/b9e9e03fdeebd494783ae1dd8e6008f5c1e3a4bc/src/vtc_haproxy.c#L786
> needs to be SIGUSR1 (and the switch below adjusted).
>
> Making the change in my local repository and running the tests that
> don't need any additional USE_XXX falgs causes 3 tests to fail with 2 of
> them triggering an assertion within VTest.
>
> It would have caught the bug reported by William, though.
>
> Concluding: It probably makes sense to adjust VTest to use SIGUSR1
> instead of SIGINT, the latter is handled like SIGTERM in HAProxy anyway,
> so there is no need for this distinction. I did not yet look into the
> details of the failing tests, though.
>
> Best regards
> Tim Düsterhus
>


VTest does not test deinit

2020-06-14 Thread Tim Düsterhus
Hi List,
Willy,
Ilya,

I noticed that the reg-tests were unable find the issue reported by
William here:
https://www.mail-archive.com/haproxy@formilux.org/msg37637.html

This is because VTest never performs a "soft" shutdown of HAProxy,
instead it uses SIGINT -> SIGTERM -> SIGKILL. Thus the deinit() will
never trigger.

Fixing this will require a change to VTest:
https://github.com/vtest/VTest/blob/b9e9e03fdeebd494783ae1dd8e6008f5c1e3a4bc/src/vtc_haproxy.c#L786
needs to be SIGUSR1 (and the switch below adjusted).

Making the change in my local repository and running the tests that
don't need any additional USE_XXX falgs causes 3 tests to fail with 2 of
them triggering an assertion within VTest.

It would have caught the bug reported by William, though.

Concluding: It probably makes sense to adjust VTest to use SIGUSR1
instead of SIGINT, the latter is handled like SIGTERM in HAProxy anyway,
so there is no need for this distinction. I did not yet look into the
details of the failing tests, though.

Best regards
Tim Düsterhus



Re: [PATCH] BUG/MAJOR: Fix bogus free() during deinit() for http-request rules

2020-06-14 Thread William Dauchy
Hi Tim,

Thanks for the quick fix. I've also tested on my side, it fixes the
issue I reported earlier.

On Sun, Jun 14, 2020 at 5:27 PM Tim Duesterhus  wrote:
> We cannot simply `release_sample_expr(rule->arg.vars.expr)` for a
> `struct act_rule`, because `rule->arg` is a union that might not
> contain valid `vars`. This leads to a crash on a configuration using
> `http-request redirect` and possibly others:
>
> frontend http
> mode http
> bind 127.0.0.1:80
> http-request redirect scheme https
>
> Instead a `struct act_rule` has a `release_ptr` that must be used
> to properly free any additional storage allocated.
>
> This patch fixes a regression in commit 
> ff78fcdd7f15c8626c7e70add7a935221ee2920c.
> It must be backported to whereever that patch is backported.
>
> It has be verified that the configuration above no longer crashes.
> It has also been verified that the configuration in 
> ff78fcdd7f15c8626c7e70add7a935221ee2920c
> does not leak.

Reported-by: William Dauchy 

> ---
>  src/haproxy.c | 1 -
>  src/vars.c| 7 +++
>  2 files changed, 7 insertions(+), 1 deletion(-)

Best,
-- 
William



[PATCH] BUG/MAJOR: Fix bogus free() during deinit() for http-request rules

2020-06-14 Thread Tim Duesterhus
We cannot simply `release_sample_expr(rule->arg.vars.expr)` for a
`struct act_rule`, because `rule->arg` is a union that might not
contain valid `vars`. This leads to a crash on a configuration using
`http-request redirect` and possibly others:

frontend http
mode http
bind 127.0.0.1:80
http-request redirect scheme https

Instead a `struct act_rule` has a `release_ptr` that must be used
to properly free any additional storage allocated.

This patch fixes a regression in commit 
ff78fcdd7f15c8626c7e70add7a935221ee2920c.
It must be backported to whereever that patch is backported.

It has be verified that the configuration above no longer crashes.
It has also been verified that the configuration in 
ff78fcdd7f15c8626c7e70add7a935221ee2920c
does not leak.
---
 src/haproxy.c | 1 -
 src/vars.c| 7 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/haproxy.c b/src/haproxy.c
index 245ac3b60..6548db6b5 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2555,7 +2555,6 @@ 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);
diff --git a/src/vars.c b/src/vars.c
index b154c529d..fd95eed5d 100644
--- a/src/vars.c
+++ b/src/vars.c
@@ -689,6 +689,11 @@ static enum act_return action_clear(struct act_rule *rule, 
struct proxy *px,
return ACT_RET_CONT;
 }
 
+static void release_store_rule(struct act_rule *rule)
+{
+   release_sample_expr(rule->arg.vars.expr);
+}
+
 /* This two function checks the variable name and replace the
  * configuration string name by the global string name. its
  * the same string, but the global pointer can be easy to
@@ -758,6 +763,7 @@ static enum act_parse_ret parse_store(const char **args, 
int *arg, struct proxy
if (!set_var) {
rule->action = ACT_CUSTOM;
rule->action_ptr = action_clear;
+   rule->release_ptr = release_store_rule;
return ACT_RET_PRS_OK;
}
 
@@ -791,6 +797,7 @@ static enum act_parse_ret parse_store(const char **args, 
int *arg, struct proxy
 
rule->action = ACT_CUSTOM;
rule->action_ptr = action_store;
+   rule->release_ptr = release_store_rule;
return ACT_RET_PRS_OK;
 }
 
-- 
2.27.0




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