Re: [PATCH v2 1/1] BUG/MINOR: mworker: Fix memory leak of mworker_proc members

2019-05-22 Thread Илья Шипицин
more findings

https://travis-ci.org/chipitsine/haproxy-1/jobs/535709949#L1938-L1948

ср, 22 мая 2019 г. в 15:12, Илья Шипицин :

> here's build with patch applied
>
> https://travis-ci.org/chipitsine/haproxy-1/jobs/535709947#L604-L608
>
> ср, 22 мая 2019 г. в 14:43, William Lallemand :
>
>> Hi Tim,
>>
>> Sorry for the delay.
>>
>> I added 2 new lines in your patch:
>>
>> On Thu, May 16, 2019 at 08:23:22PM +0200, Tim Duesterhus wrote:
>> > +void mworker_free_child(struct mworker_proc *child) {
>> > + if (child == NULL) return;
>> > +
>>
>> Which became:
>>
>> > +void mworker_free_child(struct mworker_proc *child)
>> > +{
>> > +   if (child == NULL)
>> > +   return;
>>
>>
>> Thanks, merged.
>>
>> --
>> William Lallemand
>>
>>


Re: [PATCH v2 1/1] BUG/MINOR: mworker: Fix memory leak of mworker_proc members

2019-05-22 Thread Илья Шипицин
here's build with patch applied

https://travis-ci.org/chipitsine/haproxy-1/jobs/535709947#L604-L608

ср, 22 мая 2019 г. в 14:43, William Lallemand :

> Hi Tim,
>
> Sorry for the delay.
>
> I added 2 new lines in your patch:
>
> On Thu, May 16, 2019 at 08:23:22PM +0200, Tim Duesterhus wrote:
> > +void mworker_free_child(struct mworker_proc *child) {
> > + if (child == NULL) return;
> > +
>
> Which became:
>
> > +void mworker_free_child(struct mworker_proc *child)
> > +{
> > +   if (child == NULL)
> > +   return;
>
>
> Thanks, merged.
>
> --
> William Lallemand
>
>


Re: [PATCH v2 1/1] BUG/MINOR: mworker: Fix memory leak of mworker_proc members

2019-05-22 Thread William Lallemand
Hi Tim,

Sorry for the delay.

I added 2 new lines in your patch:

On Thu, May 16, 2019 at 08:23:22PM +0200, Tim Duesterhus wrote:
> +void mworker_free_child(struct mworker_proc *child) {
> + if (child == NULL) return;
> +
 
Which became:

> +void mworker_free_child(struct mworker_proc *child)
> +{
> +   if (child == NULL)
> +   return;


Thanks, merged.

-- 
William Lallemand



[PATCH v2 1/1] BUG/MINOR: mworker: Fix memory leak of mworker_proc members

2019-05-16 Thread Tim Duesterhus
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.
---
 include/proto/mworker.h |  2 ++
 src/haproxy.c   |  3 ++-
 src/mworker-prog.c  | 19 +--
 src/mworker.c   | 31 ++-
 4 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/include/proto/mworker.h b/include/proto/mworker.h
index 86f09049f..05fe82af6 100644
--- a/include/proto/mworker.h
+++ b/include/proto/mworker.h
@@ -37,4 +37,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 a47b7dd32..b688375e2 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -3002,7 +3002,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 fd8e66384..467ce9b24 100644
--- 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 cac74102c..9052d14ad 100644
--- 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 */
@@ -554,6 +554,27 @@ 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(chi