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
