In response to the feedback on v1 on this (in particular the
use-after-free, thanks Martin!) here's a v2 which I think is a good
thing to do with our without that particular GCC behavior I ran into.
As noted in 3/3 I think this is a known caveat of those SANITIZE=
modes, e.g. valgrind reports a memory leak regardless of optimization
level.
The only pure workaround for the issue is now 3/3, which I think is a
worthwhile to carry to avoid developer potentially wasting time on it.
Ævar Arnfjörð Bjarmason (3):
refs.c: make "repo_default_branch_name" static, remove xstrfmt()
config.c: don't leak memory in handle_path_include()
config.c: free(expanded) before die(), work around GCC oddity
config.c | 22 ++++++++++++++--------
refs.c | 8 +++-----
refs.h | 1 -
t/t1305-config-include.sh | 1 +
4 files changed, 18 insertions(+), 14 deletions(-)
Range-diff against v1:
-: ----------- > 1: 4f8554bb02e refs.c: make "repo_default_branch_name" static, remove xstrfmt()
-: ----------- > 2: d6d04da1d9d config.c: don't leak memory in handle_path_include()
1: 5a47bf2e9c9 ! 3: d812358e331 leak tests: free() before die for two API functions
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
## Commit message ##
- leak tests: free() before die for two API functions
+ config.c: free(expanded) before die(), work around GCC oddity
- Call free() just before die() in two API functions whose tests are
- asserted under SANITIZE=leak. Normally this would not be needed due to
- how SANITIZE=leak works, but in these cases my GCC version (10.2.1-6)
- will fail tests t0001 and t0017 under SANITIZE=leak depending on the
- optimization level.
+ 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).
- See 956d2e4639b (tests: add a test mode for SANITIZE=leak, run it in
- CI, 2021-09-23) for the commit that marked t0017 for testing with
- SANITIZE=leak, and c150064dbe2 (leak tests: run various built-in tests
- in t00*.sh SANITIZE=leak, 2021-10-12) for t0001 (currently in "next").
+ 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, and
+ 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 the "goto cleanup" pattern introduced in the
+ preceding commit.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
## config.c ##
+@@ config.c: static int handle_path_include(const char *path, struct config_include_data *inc
+ int ret = 0;
+ struct strbuf buf = STRBUF_INIT;
+ char *expanded;
++ int die_depth = 0;
+
+ if (!path)
+ return config_error_nonbool("include.path");
@@ config.c: 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) {
-+ free(expanded);
- die(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,
- !cf ? "<unknown>" :
- cf->name ? cf->name :
- "the command line");
++ die_depth = 1;
++ goto cleanup;
+ }
ret = git_config_from_file(git_config_include, path, inc);
inc->depth--;
}
-
- ## refs.c ##
-@@ refs.c: char *repo_default_branch_name(struct repository *r, int quiet)
- }
-
- full_ref = xstrfmt("refs/heads/%s", ret);
-- if (check_refname_format(full_ref, 0))
-+ if (check_refname_format(full_ref, 0)) {
-+ free(ret);
-+ free(full_ref);
- die(_("invalid branch name: %s = %s"), config_display_key, ret);
-+ }
- free(full_ref);
+ cleanup:
+ strbuf_release(&buf);
+ free(expanded);
+- return ret;
++ if (!die_depth)
++ return ret;
++ die(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,
++ !cf ? "<unknown>" : cf->name ? cf->name : "the command line");
+ }
- return ret;
+ static void add_trailing_starstar_for_dir(struct strbuf *pat)
--
2.33.1.1494.g88b39a443e1
