On Sun, May 18, 2014 at 12:34:31AM +0200, Cyril Bonté wrote:
> Hi all,
>
> Le 18/05/2014 00:10, Thomas Heil a écrit :
> >Ive tried with this patch
> >-b
> >Index: haproxy-1.5-dev25/src/cfgparse.c
> >===================================================================
> >--- haproxy-1.5-dev25.orig/src/cfgparse.c
> >+++ haproxy-1.5-dev25/src/cfgparse.c
> >@@ -1580,7 +1580,10 @@ static int create_cond_regex_rule(const
> >
> > err:
> > free(errmsg);
> >- free(preg);
> >+ if (preg) {
> >+ regfree(preg);
> >+ free(preg);
> >+ }
> > return err_code;
> > }
>
> One thing is that I don't understand the initial patch provided by Dirkjan :
> http://haproxy.1wt.eu/git?p=haproxy.git;a=commitdiff;h=07fcaaa4cd89002cb100644197752ea050f54bee;hp=e21f84903ef334c02f2783b20a128f381263e676#patch2
>
> Why would we want to free the preg memory when no error occurs ?
> I'm really not sure this fix is valid.
Yeah, that's it! Thank you Cyril! I think I was too much focused on the
error path due to the err label, but what you're saying is absolutely
true : we free what we'll be using next (chain_regex() stores the preg
pointer into exp->preg). Now I can reproduce Thomas' error. We just need
to build a few req* rules and the crash is immediate upon first request
(both regex have the same pointer anyway, probably reused by whatever
appears next in the config) :
reqdeny ^/b
reqdeny ^/c
So we need to revert that part of Dirkjan's patch and to add the
regfree(preg) on the error path, but only if regcomp() succeeds.
The attached patch fixes all cases for me, both the nominal code
path, and the various error paths. I have also renamed the return
code "ret_code" instead of "err_code" in order not to make people
think we're always on an error path (which got me, and which is
probably why Dirkjan got confused first). I've applied it now.
Thanks,
Willy
>From 63af98d0dd83b4b173fe7c6c632f0583b7cec497 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <[email protected]>
Date: Sun, 18 May 2014 08:11:41 +0200
Subject: BUG/MAJOR: config: don't free valid regex memory
MIME-Version: 1.0
Content-Type: text/plain; charset=latin1
Content-Transfer-Encoding: 8bit
Thomas Heil reported that previous commit 07fcaaa ("MINOR: fix a few
memory usage errors") make haproxy crash when req* rules are used. As
diagnosed by Cyril Bonté, this commit introduced a regression which
makes haproxy free the memory areas allocated for regex even when
they're going to be used, resulting in the crashes.
This patch does three things :
- undo the free() on the valid path
- add regfree() on the error path but only when regcomp() succeeds
- rename err_code to ret_code to avoid confusing the valid return
path with an error path.
---
src/cfgparse.c | 36 ++++++++++++++++++++++--------------
1 file changed, 22 insertions(+), 14 deletions(-)
diff --git a/src/cfgparse.c b/src/cfgparse.c
index eb7ec20..5edb773 100644
--- a/src/cfgparse.c
+++ b/src/cfgparse.c
@@ -1505,6 +1505,10 @@ void init_default_instance()
}
+/* This function createss a new req* or rsp* rule to the proxy. It compiles the
+ * regex and may return the ERR_WARN bit, and error bits such as ERR_ALERT and
+ * ERR_FATAL in case of error.
+ */
static int create_cond_regex_rule(const char *file, int line,
struct proxy *px, int dir, int action, int
flags,
const char *cmd, const char *reg, const char
*repl,
@@ -1513,41 +1517,41 @@ static int create_cond_regex_rule(const char *file, int
line,
regex_t *preg = NULL;
char *errmsg = NULL;
const char *err;
- int err_code = 0;
+ int ret_code = 0;
struct acl_cond *cond = NULL;
if (px == &defproxy) {
Alert("parsing [%s:%d] : '%s' not allowed in 'defaults'
section.\n", file, line, cmd);
- err_code |= ERR_ALERT | ERR_FATAL;
+ ret_code |= ERR_ALERT | ERR_FATAL;
goto err;
}
if (*reg == 0) {
Alert("parsing [%s:%d] : '%s' expects <regex> as an
argument.\n", file, line, cmd);
- err_code |= ERR_ALERT | ERR_FATAL;
+ ret_code |= ERR_ALERT | ERR_FATAL;
goto err;
}
if (warnifnotcap(px, PR_CAP_RS, file, line, cmd, NULL))
- err_code |= ERR_WARN;
+ ret_code |= ERR_WARN;
if (cond_start &&
(strcmp(*cond_start, "if") == 0 || strcmp(*cond_start, "unless") ==
0)) {
if ((cond = build_acl_cond(file, line, px, cond_start,
&errmsg)) == NULL) {
Alert("parsing [%s:%d] : error detected while parsing a
'%s' condition : %s.\n",
file, line, cmd, errmsg);
- err_code |= ERR_ALERT | ERR_FATAL;
+ ret_code |= ERR_ALERT | ERR_FATAL;
goto err;
}
}
else if (cond_start && **cond_start) {
Alert("parsing [%s:%d] : '%s' : Expecting nothing, 'if', or
'unless', got '%s'.\n",
file, line, cmd, *cond_start);
- err_code |= ERR_ALERT | ERR_FATAL;
+ ret_code |= ERR_ALERT | ERR_FATAL;
goto err;
}
- err_code |= warnif_cond_conflicts(cond,
+ ret_code |= warnif_cond_conflicts(cond,
(dir == SMP_OPT_DIR_REQ) ?
((px->cap & PR_CAP_FE) ?
SMP_VAL_FE_HRQ_HDR : SMP_VAL_BE_HRQ_HDR) :
((px->cap & PR_CAP_BE) ?
SMP_VAL_BE_HRS_HDR : SMP_VAL_FE_HRS_HDR),
@@ -1556,13 +1560,13 @@ static int create_cond_regex_rule(const char *file, int
line,
preg = calloc(1, sizeof(regex_t));
if (!preg) {
Alert("parsing [%s:%d] : '%s' : not enough memory to build
regex.\n", file, line, cmd);
- err_code = ERR_ALERT | ERR_FATAL;
+ ret_code = ERR_ALERT | ERR_FATAL;
goto err;
}
if (regcomp(preg, reg, REG_EXTENDED | flags) != 0) {
Alert("parsing [%s:%d] : '%s' : bad regular expression
'%s'.\n", file, line, cmd, reg);
- err_code = ERR_ALERT | ERR_FATAL;
+ ret_code = ERR_ALERT | ERR_FATAL;
goto err;
}
@@ -1571,17 +1575,21 @@ static int create_cond_regex_rule(const char *file, int
line,
if (repl && err) {
Alert("parsing [%s:%d] : '%s' : invalid character or
unterminated sequence in replacement string near '%c'.\n",
file, line, cmd, *err);
- err_code |= ERR_ALERT | ERR_FATAL;
- goto err;
+ ret_code |= ERR_ALERT | ERR_FATAL;
+ goto err_free;
}
if (dir == SMP_OPT_DIR_REQ && warnif_misplaced_reqxxx(px, file, line,
cmd))
- err_code |= ERR_WARN;
+ ret_code |= ERR_WARN;
+
+ return ret_code;
+ err_free:
+ regfree(preg);
err:
- free(errmsg);
free(preg);
- return err_code;
+ free(errmsg);
+ return ret_code;
}
/*
--
1.7.12.2.21.g234cd45.dirty
