| Message ID | patch-v3-5.6-9a44204c4c9-20211022T175227Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usage.c: add die_message() & plug memory leaks in refs.c & config.c | expand |
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
diff --git a/config.c b/config.c index c5873f3a706..c36e85c2077 100644 --- a/config.c +++ b/config.c @@ -132,6 +132,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc int ret = 0; struct strbuf buf = STRBUF_INIT; char *expanded; + int exit_with = 0; if (!path) return config_error_nonbool("include.path"); @@ -161,17 +162,21 @@ static int handle_path_include(const char *path, struct config_include_data *inc } if (!access_or_die(path, R_OK, 0)) { - if (++inc->depth > MAX_INCLUDE_DEPTH) - die(_(include_depth_advice), MAX_INCLUDE_DEPTH, path, - !cf ? "<unknown>" : - cf->name ? cf->name : - "the command line"); + if (++inc->depth > MAX_INCLUDE_DEPTH) { + exit_with = die_message(_(include_depth_advice), + MAX_INCLUDE_DEPTH, path, + !cf ? "<unknown>" : cf->name ? + cf->name : "the command line"); + goto cleanup; + } ret = git_config_from_file(git_config_include, path, inc); inc->depth--; } cleanup: strbuf_release(&buf); free(expanded); + if (exit_with) + exit(exit_with); return ret; }
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). 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. Let's work around this issue by freeing the "expanded" memory before we call die(), using a combination of the "goto cleanup" pattern introduced in a preceding commit, and the newly introduced die_message() function. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- config.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)