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

2020-06-15 Thread Willy Tarreau 
On Sun, Jun 14, 2020 at 05:27:36PM +0200, 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.
(...)

Thank you guys. I was hesitant on this one because I didn't remember
that part at all and had to see how this was used for other actions,
but after analysing the other ones, I agree that this is the correct
way to release memory.

Now applied, thank you!
Willy

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(&rule->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