Re: [PATCH v3 5/6] config.c: free(expanded) before die(), work around GCC oddity

On Fri, Oct 22, 2021 at 08:19:38PM +0200, Ævar Arnfjörð Bjarmason wrote:

On my GCC version (10.2.1-6), but not the clang I have available t0017
will fail under SANITIZE=leak on optimization levels higher than -O0,
which is annoying when combined with the change in 956d2e4639b (tests:
add a test mode for SANITIZE=leak, run it in CI, 2021-09-23).

This one really makes me sad. The resulting code is more complicated,
and what guarantee do we have that we won't run into similar problems
with other die() calls?

If we're getting false positives, I'd rather see us work around them
with annotations, or a better compiler (I couldn't reproduce with gcc
10.3.0 or 11.2.0 from Debian, so I doubt there is even much point in
reporting it upstream).

We really do have a memory leak here in either case, as e.g. running
the pre-image under valgrind(1) will reveal. It's documented
SANITIZE=leak (and "address", which exhibits the same behavior) might
interact with compiler optimization in this way in some cases. Since
this function is called recursively it's going to be especially
interesting as an optimization target.

I don't see how we have a leak. If we hit this die code-path then we
never exit the function. I can't reproduce the problem, but it sounds
like -O2 is reusing the stack space of "expanded" to prepare for the
die() call? IMHO that is not an actual leak. It is still in scope from
the perspective of C, and anyway we are about to exit from within the
die().

If we were to do anything in the code itself, I'd much prefer to hit it
with an UNLEAK().

-Peff