BUG/MEDIUM: ensure that unresolved arguments are freed exactly once

When passing arguments to ACLs and samples, some types are stored as
strings then resolved later after config parsing is done. Upon exit,
the arguments need to be freed only if the string was not resolved
yet. At the moment we can encounter double free during deinit()
because some arguments (eg: userlists) are freed once as their own
type and once as a string.

The solution consists in adding an "unresolved" flag to the args to
say whether the value is still held in the <str> part or is final.

This could be debugged thanks to a useful bug report from Sander Klein.

diff --git a/include/types/arg.h b/include/types/arg.h
index 23697214840b9441fa389a9cc7ba22d780ded01a..2762674ee15d1a6ea6907ad0f70886cab0586895 100644 (file)
--- a/include/types/arg.h
+++ b/include/types/arg.h
@@ -63,8 +63,9 @@ union arg_data {
 };
 
 struct arg {
-       int type;              /* argument type */
-       union arg_data data;   /* argument data */
+       unsigned char type;       /* argument type, ARGT_* */
+       unsigned char unresolved; /* argument contains a string in <str> that must be resolved and freed */
+       union arg_data data;      /* argument data */
 };
 
 

diff --git a/src/acl.c b/src/acl.c
index 8ec046ff02750dfb18a1b338f788632f5d1f10d6..e51dafc9de999624bec372ca4c42e37208cf9b0f 100644 (file)
--- a/src/acl.c
+++ b/src/acl.c
@@ -1227,11 +1227,10 @@ static struct acl_expr *prune_acl_expr(struct acl_expr *expr)
        for (arg = expr->args; arg; arg++) {
                if (arg->type == ARGT_STOP)
                        break;
-               if (arg->type == ARGT_FE || arg->type == ARGT_BE ||
-                   arg->type == ARGT_TAB || arg->type == ARGT_SRV ||
-                   arg->type == ARGT_USR || arg->type == ARGT_STR) {
+               if (arg->type == ARGT_STR || arg->unresolved) {
                        free(arg->data.str.str);
                        arg->data.str.str = NULL;
+                       arg->unresolved = 0;
                }
                arg++;
        }
@@ -2065,6 +2064,8 @@ acl_find_targets(struct proxy *p)
                        for (arg = expr->args; arg; arg++) {
                                if (arg->type == ARGT_STOP)
                                        break;
+                               else if (!arg->unresolved)
+                                       continue;
                                else if (arg->type == ARGT_SRV) {
                                        struct proxy *px;
                                        struct server *srv;
@@ -2107,6 +2108,8 @@ acl_find_targets(struct proxy *p)
                                        }
 
                                        free(expr->args->data.str.str);
+                                       expr->args->data.str.str = NULL;
+                                       arg->unresolved = 0;
                                        expr->args->data.srv = srv;
                                }
                                else if (arg->type == ARGT_FE) {
@@ -2133,6 +2136,8 @@ acl_find_targets(struct proxy *p)
                                        }
 
                                        free(expr->args->data.str.str);
+                                       expr->args->data.str.str = NULL;
+                                       arg->unresolved = 0;
                                        expr->args->data.prx = prx;
                                }
                                else if (arg->type == ARGT_BE) {
@@ -2159,6 +2164,8 @@ acl_find_targets(struct proxy *p)
                                        }
 
                                        free(expr->args->data.str.str);
+                                       expr->args->data.str.str = NULL;
+                                       arg->unresolved = 0;
                                        expr->args->data.prx = prx;
                                }
                                else if (arg->type == ARGT_TAB) {
@@ -2186,6 +2193,8 @@ acl_find_targets(struct proxy *p)
                                        }
 
                                        free(expr->args->data.str.str);
+                                       expr->args->data.str.str = NULL;
+                                       arg->unresolved = 0;
                                        expr->args->data.prx = prx;
                                }
                                else if (arg->type == ARGT_USR) {
@@ -2210,6 +2219,8 @@ acl_find_targets(struct proxy *p)
                                        }
 
                                        free(expr->args->data.str.str);
+                                       expr->args->data.str.str = NULL;
+                                       arg->unresolved = 0;
                                        expr->args->data.usr = ul;
                                }
                        } /* end of args processing */

diff --git a/src/arg.c b/src/arg.c
index 457c5b38dabbe0803d733bce97878de2b5580cdd..9d5fceec2d0a56aa0ffe00456c5d437c35e5e95b 100644 (file)
--- a/src/arg.c
+++ b/src/arg.c
@@ -127,6 +127,11 @@ int make_arg_list(const char *in, int len, unsigned int mask, struct arg **argp,
                case ARGT_TAB:
                case ARGT_SRV:
                case ARGT_USR:
+                       /* These argument types need to be stored as strings during
+                        * parsing then resolved later.
+                        */
+                       arg->unresolved = 1;
+                       /* fall through */
                case ARGT_STR:
                        /* all types that must be resolved are stored as strings
                         * during the parsing. The caller must at one point resolve

diff --git a/src/haproxy.c b/src/haproxy.c
index ff962c2748f88ef3614343ea48a22fd699c053a5..48c343344a53773cc49c92644628b747fe1f2c93 100644 (file)
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -772,11 +772,10 @@ static void deinit_sample_arg(struct arg *p)
                return;
 
        while (p->type != ARGT_STOP) {
-               if (p->type == ARGT_FE || p->type == ARGT_BE ||
-                   p->type == ARGT_TAB || p->type == ARGT_SRV ||
-                   p->type == ARGT_USR || p->type == ARGT_STR) {
+               if (p->type == ARGT_STR || p->unresolved) {
                        free(p->data.str.str);
                        p->data.str.str = NULL;
+                       p->unresolved = 0;
                }
                p++;
        }