BUG/MINOR: mworker: Fix memory leak of mworker_proc members

The struct mworker_proc is not uniformly freed everywhere, sometimes leading
to leaks of the `id` string (and possibly the other strings).

Introduce a mworker_free_child function instead of duplicating the freeing
logic everywhere to prevent this kind of issues.

This leak was reported in issue #96.

It looks like the leaks have been introduced in commit 9a1ee7ac31c56fd7d881adf2ef4659f336e50c9f,
which is specific to 2.0-dev. Backporting `mworker_free_child` might be
helpful to ease backporting other fixes, though.

diff --git a/include/proto/mworker.h b/include/proto/mworker.h
index 2386a445ad47ee1329452761a8affbaca7ed7b4f..04187826665d74c6c79e1c810778fd0ec8b97add 100644 (file)
--- a/include/proto/mworker.h
+++ b/include/proto/mworker.h
@@ -36,4 +36,6 @@ int mworker_ext_launch_all();
 
 void mworker_kill_max_reloads(int sig);
 
+void mworker_free_child(struct mworker_proc *);
+
 #endif /* PROTO_MWORKER_H_ */

diff --git a/src/haproxy.c b/src/haproxy.c
index f5d72e6543d2b9c569d2db1f19b21749f580fec7..923b92327c3870afb94c2d3855ba34737838be6e 100644 (file)
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -3035,7 +3035,8 @@ int main(int argc, char **argv)
                                        continue;
                                }
                                LIST_DEL(&child->list);
-                               free(child);
+                               mworker_free_child(child);
+                               child = NULL;
                        }
                }
 

diff --git a/src/mworker-prog.c b/src/mworker-prog.c
index fd8e66384847ee13adbf7cb4bf7b7ff1e5c71f80..467ce9b248a6e5879df60d453c770b26652e5eee 100644 (file)
--- a/src/mworker-prog.c
+++ b/src/mworker-prog.c
@@ -69,24 +69,7 @@ int mworker_ext_launch_all()
 
 
                                LIST_DEL(&child->list);
-                               if (child->command) {
-                                       int i;
-
-                                       for (i = 0; child->command[i]; i++) {
-                                               if (child->command[i]) {
-                                                       free(child->command[i]);
-                                                       child->command[i] = NULL;
-                                               }
-                                       }
-                                       free(child->command);
-                                       child->command = NULL;
-                               }
-                               if (child->id) {
-                                       free(child->id);
-                                       child->id = NULL;
-                               }
-
-                               free(child);
+                               mworker_free_child(child);
                                child = NULL;
 
                                continue;

diff --git a/src/mworker.c b/src/mworker.c
index d5040b75990cbfcd58897269a1db1bc15c466ba3..728a3a149fa19d31e62977e8b6ac55b190bf11ec 100644 (file)
--- a/src/mworker.c
+++ b/src/mworker.c
@@ -185,9 +185,7 @@ void mworker_env_to_proc_list()
 
                        LIST_ADDQ(&proc_list, &child->list);
                } else {
-                       free(child->id);
-                       free(child);
-
+                       mworker_free_child(child);
                }
        }
 
@@ -245,7 +243,6 @@ void mworker_catch_sigchld(struct sig_handler *sh)
 {
        int exitpid = -1;
        int status = 0;
-       struct mworker_proc *child, *it;
        int childfound;
 
 restart_wait:
@@ -254,6 +251,8 @@ restart_wait:
 
        exitpid = waitpid(-1, &status, WNOHANG);
        if (exitpid > 0) {
+               struct mworker_proc *child, *it;
+
                if (WIFEXITED(status))
                        status = WEXITSTATUS(status);
                else if (WIFSIGNALED(status))
@@ -301,7 +300,8 @@ restart_wait:
                                        ha_warning("Former program '%s' (%d) exited with code %d (%s)\n", child->id, exitpid, status, (status >= 128) ? strsignal(status - 128) : "Exit");
                                }
                        }
-                       free(child);
+                       mworker_free_child(child);
+                       child = NULL;
                }
 
                /* do it again to check if it was the last worker */
@@ -561,6 +561,29 @@ out:
        return err_code;
 }
 
+void mworker_free_child(struct mworker_proc *child)
+{
+       if (child == NULL)
+               return;
+
+       if (child->command) {
+               int i;
+
+               for (i = 0; child->command[i]; i++) {
+                       if (child->command[i]) {
+                               free(child->command[i]);
+                               child->command[i] = NULL;
+                       }
+               }
+               free(child->command);
+               child->command = NULL;
+       }
+       if (child->id) {
+               free(child->id);
+               child->id = NULL;
+       }
+       free(child);
+}
 
 static struct cfg_kw_list mworker_kws = {{ }, {
        { CFG_GLOBAL, "mworker-max-reloads", mworker_parse_global_max_reloads },