Re: [PATCH] BUG/MINOR: cli: Fix memory leak
On Thu, Nov 08, 2018 at 12:54:55AM +0100, William Lallemand wrote:
> On Thu, Nov 08, 2018 at 12:47:00AM +0100, Tim Düsterhus wrote:
> > Hi
> >
>
> Hi Tim,
>
> > Am 08.11.18 um 00:32 schrieb Tim Duesterhus:
> > > if (!str2listener(path, global.stats_fe, bind_conf, "master-socket", 0,
> > > &err)) {
> >
> > I just notice that `err` in `mworker_cli_sockpair_new` should probably
> > be freed as well. Valgrind did not report this, because I did not have
> > any errors. Can you make the necessary adjustments, please? There's
> > possibly even more strings that leak.
> >
> > I suggest to run haproxy with valgrind yourself. There's a bit of
> > "possibly lost" memory as well. I used:
> >
> > valgrind --leak-check=full ./haproxy -d -Sa /scratch/haproxy/cli.sock
> > -Ws -f ./haproxy.cfg
> >
> > with an empty configuration file to find the issues my patch fixes.
> >
>
> Thanks for the report, I'm aware of those issues, the mworker is still a WiP
> in
> the master.
Just an advice for such cases in the future, please leave a fixme or XXX
comment close to the area which needs to be re-checked indicating what
remains to be done, otherwise we all forget about this stuff and rediscover
it after the release, ashamed as I was after seeing H2 didn't support
chunked encoding and only had comments about it! The fixme comment will
not fix everything but we do have an opportunity to grep for them before
the final release at least ;-)
On this case I think you'll need to have an error label to jump to at
the end of the function which deals with everything. I'm seeing that
the socket pairs are not cleared either upon error, so you'll need a
full unroll on the error path.
Thanks!
Willy
Re: [PATCH] BUG/MINOR: cli: Fix memory leak
On Thu, Nov 08, 2018 at 12:47:00AM +0100, Tim Düsterhus wrote:
> Hi
>
Hi Tim,
> Am 08.11.18 um 00:32 schrieb Tim Duesterhus:
> > if (!str2listener(path, global.stats_fe, bind_conf, "master-socket", 0,
> > &err)) {
>
> I just notice that `err` in `mworker_cli_sockpair_new` should probably
> be freed as well. Valgrind did not report this, because I did not have
> any errors. Can you make the necessary adjustments, please? There's
> possibly even more strings that leak.
>
> I suggest to run haproxy with valgrind yourself. There's a bit of
> "possibly lost" memory as well. I used:
>
> valgrind --leak-check=full ./haproxy -d -Sa /scratch/haproxy/cli.sock
> -Ws -f ./haproxy.cfg
>
> with an empty configuration file to find the issues my patch fixes.
>
Thanks for the report, I'm aware of those issues, the mworker is still a WiP in
the master.
Cheers,
--
William Lallemand
Re: [PATCH] BUG/MINOR: cli: Fix memory leak
Hi
Am 08.11.18 um 00:32 schrieb Tim Duesterhus:
> if (!str2listener(path, global.stats_fe, bind_conf, "master-socket", 0,
> &err)) {
I just notice that `err` in `mworker_cli_sockpair_new` should probably
be freed as well. Valgrind did not report this, because I did not have
any errors. Can you make the necessary adjustments, please? There's
possibly even more strings that leak.
I suggest to run haproxy with valgrind yourself. There's a bit of
"possibly lost" memory as well. I used:
valgrind --leak-check=full ./haproxy -d -Sa /scratch/haproxy/cli.sock
-Ws -f ./haproxy.cfg
with an empty configuration file to find the issues my patch fixes.
Best regards
Tim Düsterhus
[PATCH] BUG/MINOR: cli: Fix memory leak
Valgrind's memcheck reports memory leaks in cli.c, because
the out parameter of memprintf is not properly freed:
==31035== 11 bytes in 1 blocks are definitely lost in loss record 16 of 101
==31035==at 0x4C2DB8F: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==31035==by 0x4C2FDEF: realloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==31035==by 0x4A3C72: my_realloc2 (standard.h:1364)
==31035==by 0x4A3C72: memvprintf (standard.c:3459)
==31035==by 0x4A3D93: memprintf (standard.c:3482)
==31035==by 0x4AF77E: mworker_cli_sockpair_new (cli.c:2324)
==31035==by 0x48E826: init (haproxy.c:1749)
==31035==by 0x408BBC: main (haproxy.c:2725)
==31035==
==31035== 11 bytes in 1 blocks are definitely lost in loss record 17 of 101
==31035==at 0x4C2DB8F: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==31035==by 0x4C2FDEF: realloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==31035==by 0x4A3C72: my_realloc2 (standard.h:1364)
==31035==by 0x4A3C72: memvprintf (standard.c:3459)
==31035==by 0x4A3D93: memprintf (standard.c:3482)
==31035==by 0x4AF071: mworker_cli_proxy_create (cli.c:2172)
==31035==by 0x48EC89: init (haproxy.c:1760)
==31035==by 0x408BBC: main (haproxy.c:2725)
These leaks were introduced in commits
ce83b4a5dd48c000dec68f9d551945d21e9ac7ac and
8a02257d88276e2f2f10c407d2961995f74a192c
which are specific to haproxy 1.9 dev.
---
src/cli.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/cli.c b/src/cli.c
index e20e0c5b..71537058 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -2170,8 +2170,12 @@ int mworker_cli_proxy_create()
newsrv->conf.line = 0;
memprintf(&msg, "sockpair@%d", child->ipc_fd[0]);
- if ((sk = str2sa_range(msg, &port, &port1, &port2, &errmsg,
NULL, NULL, 0)) == 0)
+ if ((sk = str2sa_range(msg, &port, &port1, &port2, &errmsg,
NULL, NULL, 0)) == 0) {
+ free(msg);
return -1;
+ }
+ free(msg);
+ msg = NULL;
proto = protocol_by_family(sk->ss_family);
if (!proto || !proto->connect) {
@@ -2327,9 +2331,12 @@ int mworker_cli_sockpair_new(struct mworker_proc
*mworker_proc, int proc)
}
if (!str2listener(path, global.stats_fe, bind_conf, "master-socket", 0,
&err)) {
+ free(path);
ha_alert("Cannot create a CLI sockpair listener for process
#%d\n", proc);
return -1;
}
+ free(path);
+ path = NULL;
list_for_each_entry(l, &bind_conf->listeners, by_bind) {
l->maxconn = global.stats_fe->maxconn;
--
2.19.1
