Hi Tim,
On Tue, Jun 16, 2020 at 12:03:01AM +0200, Tim Duesterhus wrote:
> Willy,
>
> this series fixes up a few more frees. This time I have verified the changes
> a bit more carefully, running configuration check on a real world
> configuration
> of mine within valgrind. It still reports a five leaks (but less than without
> applying these patches!) and does not report any memory unsafety / bogus frees
> / double frees / use-after-frees.
>
> The first two patches fix leaks within helper functions. I triggered them
> during deinit, but they might or might not be accessible during normal
> operation (e.g. via the CLI socket). The last two patches are part of the
> deinit() function and thus cannot happen in normal operation, but cleaning
> them up makes it easier to find actual, important, issues.
>
> Feel free to apply to 'next' only (or after the release of 2.2). No need to
> risk breaking anything shortly before release with something less important
> like this. And of course please carefully review them :-)
Thank you for this work. I appreciate how painful it is to track this
stuff down to the inner code. I'd indeed prefer to postpone its integration
for having been bitten many times. One specific case you need to take care
of is the situation where there is an error in one of the ACL patterns that
prevents the expression from being completed and that can result in some
elements not to be resolved. I typically don't care if we observe memleaks
in such a case since it dies in an error, but we don't want to see double
frees nor dereference of a wrong pointer. I don't remember all the corner
cases but they'll basically look like this :
http-request deny if { src 12.12.12.12 13.13.13.13/33 }
(or anything that manages to cause a parsing error on a pattern after one
was properly resolved). This may also happen in other situations like when
an argument references a server/backend/stick-table that doesn't exist and
as such fails the config check, for example:
http-request deny if { nbsrv(blah) gt 2 }
or possibly if that's one argument of a converter. I suspect that your
patches are OK because very likely release_sample_expr() was created
after the code paths you've changed. But it's indeed a bit tricky and
deep into the error path.
Do not hesitate to ping me after the release if I forget to get back to it.
Cheers,
Willy
