BUG/MINOR: logs: fix logsrv leaks on clean exit

Log servers are a real mess because:
  - entries are duplicated using memcpy() without their strings being
    reallocated, which results in these ones not being freeable every
    time.

  - a new field, ring_name, was added in 2.2 by commit 99c453df9
    ("MEDIUM: ring: new section ring to declare custom ring buffers.")
    but it's never initialized during copies, causing the same issue

  - no attempt is made at freeing all that.

Of course, running "haproxy -c" under ASAN quickly notices that and
dumps a core.

This patch adds the missing strdup() and initialization where required,
adds a new free_logsrv() function to cleanly free() such a structure,
calls it from the proxy when iterating over logsrvs instead of silently
leaking their file names and ring names, and adds the same logsrv loop
to the proxy_free_defaults() function so that we don't leak defaults
sections on exit.

It looks a bit entangled, but it comes as a whole because all this stuff
is inter-dependent and was missing.

It's probably preferable not to backport this in the foreseable future
as it may reveal other jokes if some obscure parts continue to memcpy()
the logsrv struct.

diff --git a/include/haproxy/log.h b/include/haproxy/log.h
index 49fbe5bfd43e06656675c67603cf51f732ad4314..ac839c351289a30c7201ae5a064a54a99a8dd55f 100644 (file)
--- a/include/haproxy/log.h
+++ b/include/haproxy/log.h
@@ -86,6 +86,8 @@ int add_to_logformat_list(char *start, char *end, int type, struct list *list_fo
  */
 int parse_logformat_string(const char *str, struct proxy *curproxy, struct list *list_format, int options, int cap, char **err);
 
+void free_logsrv(struct logsrv *logsrv);
+
 /* Parse "log" keyword and update the linked list. */
 int parse_logsrv(char **args, struct list *logsrvs, int do_del, const char *file, int linenum, char **err);
 

diff --git a/src/http_client.c b/src/http_client.c
index 5a3d8a4ee2c57a5d0a6429c8083a1f8e558539e0..f5a29934d3ed223eafe12bba6799270e304bdb47 100644 (file)
--- a/src/http_client.c
+++ b/src/http_client.c
@@ -1075,6 +1075,9 @@ static int httpclient_cfg_postparser()
                memcpy(node, logsrv, sizeof(*node));
                LIST_INIT(&node->list);
                LIST_APPEND(&curproxy->logsrvs, &node->list);
+               node->ring_name = NULL;
+               node->conf.file = NULL;
+               node->conf.line = 0;
        }
        if (curproxy->conf.logformat_string) {
                curproxy->conf.args.ctx = ARGC_LOG;

diff --git a/src/log.c b/src/log.c
index 3e20c1cc0d6c50d2c5ab284b0ac77cba8bb3ad31..9952aaa15609d7c8820f9a298191981bbb1d7d05 100644 (file)
--- a/src/log.c
+++ b/src/log.c
@@ -733,6 +733,21 @@ int smp_log_range_cmp(const void *a, const void *b)
        return 0;
 }
 
+/* frees log server <logsrv> after freeing all of its allocated fields. The
+ * server must not belong to a list anymore. Logsrv may be NULL, which is
+ * silently ignored.
+ */
+void free_logsrv(struct logsrv *logsrv)
+{
+       if (!logsrv)
+               return;
+
+       BUG_ON(LIST_INLIST(&logsrv->list));
+       ha_free(&logsrv->conf.file);
+       ha_free(&logsrv->ring_name);
+       free(logsrv);
+}
+
 /*
  * Parse "log" keyword and update <logsrvs> list accordingly.
  *
@@ -769,8 +784,8 @@ int parse_logsrv(char **args, struct list *logsrvs, int do_del, const char *file
                }
 
                list_for_each_entry_safe(logsrv, back, logsrvs, list) {
-                       LIST_DELETE(&logsrv->list);
-                       free(logsrv);
+                       LIST_DEL_INIT(&logsrv->list);
+                       free_logsrv(logsrv);
                }
                return 1;
        }
@@ -797,6 +812,7 @@ int parse_logsrv(char **args, struct list *logsrvs, int do_del, const char *file
                        node->ref = logsrv;
                        LIST_INIT(&node->list);
                        LIST_APPEND(logsrvs, &node->list);
+                       node->ring_name = logsrv->ring_name ? strdup(logsrv->ring_name) : NULL;
                        node->conf.file = strdup(file);
                        node->conf.line = linenum;
 
@@ -1007,11 +1023,7 @@ int parse_logsrv(char **args, struct list *logsrvs, int do_del, const char *file
 
   error:
        free(smp_rgs);
-       if (logsrv) {
-               free(logsrv->conf.file);
-               free(logsrv->ring_name);
-       }
-       free(logsrv);
+       free_logsrv(logsrv);
        return 0;
 }
 

diff --git a/src/proxy.c b/src/proxy.c
index e45df8c77531bb9384289224330d746dce1a603a..3ff20358e61978246c9e9ce895aa41b876da3ae6 100644 (file)
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -237,8 +237,8 @@ void free_proxy(struct proxy *p)
        }
 
        list_for_each_entry_safe(log, logb, &p->logsrvs, list) {
-               LIST_DELETE(&log->list);
-               free(log);
+               LIST_DEL_INIT(&log->list);
+               free_logsrv(log);
        }
 
        list_for_each_entry_safe(lf, lfb, &p->logformat, list) {
@@ -1424,6 +1424,7 @@ void proxy_preset_defaults(struct proxy *defproxy)
 void proxy_free_defaults(struct proxy *defproxy)
 {
        struct acl *acl, *aclb;
+       struct logsrv *log, *logb;
 
        ha_free(&defproxy->id);
        ha_free(&defproxy->conf.file);
@@ -1467,6 +1468,11 @@ void proxy_free_defaults(struct proxy *defproxy)
        if (defproxy->conf.logformat_sd_string != default_rfc5424_sd_log_format)
                ha_free(&defproxy->conf.logformat_sd_string);
 
+       list_for_each_entry_safe(log, logb, &defproxy->logsrvs, list) {
+               LIST_DEL_INIT(&log->list);
+               free_logsrv(log);
+       }
+
        ha_free(&defproxy->conf.uniqueid_format_string);
        ha_free(&defproxy->conf.error_logformat_string);
        ha_free(&defproxy->conf.lfs_file);
@@ -1773,6 +1779,9 @@ static int proxy_defproxy_cpy(struct proxy *curproxy, const struct proxy *defpro
                node->ref = tmplogsrv->ref;
                LIST_INIT(&node->list);
                LIST_APPEND(&curproxy->logsrvs, &node->list);
+               node->ring_name = tmplogsrv->ring_name ? strdup(tmplogsrv->ring_name) : NULL;
+               node->conf.file = strdup(tmplogsrv->conf.file);
+               node->conf.line = tmplogsrv->conf.line;
        }
 
        curproxy->conf.uniqueid_format_string = defproxy->conf.uniqueid_format_string;