From 88c6e9d31c695c4500376284a4c3b0e17b665781 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 09:03:54 -0400 Subject: [PATCH 01/10] test-lib: --valgrind should not override --verbose-log The --verbose test option cannot be used with test harnesses like "prove". Instead, you must use --verbose-log. Since the --valgrind option implies --verbose, that means that it cannot be used with prove. I.e., this does not work: prove t0000-basic.sh :: --valgrind You'd think it could be fixed by doing: prove t0000-basic.sh :: --valgrind --verbose-log but that doesn't work either, because the implied --verbose takes precedence over --verbose-log. If the user has given us a specific option, we should prefer that. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/test-lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 5fbd8d4a90..62461a6e35 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -274,7 +274,7 @@ then test -z "$verbose" && verbose_only="$valgrind_only" elif test -n "$valgrind" then - verbose=t + test -z "$verbose_log" && verbose=t fi if test -n "$color" From 85b81b35ff90046e5ab73808785706245a778520 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 09:04:04 -0400 Subject: [PATCH 02/10] test-lib: set LSAN_OPTIONS to abort by default We already set ASAN_OPTIONS to abort if it finds any errors. As we start to experiment with LSAN, the leak sanitizer, it's convenient if we give it the same treatment. Note that ASAN is actually a superset of LSAN and can do the leak detection itself. So this only has an effect if you specifically build with "make SANITIZE=leak" (leak detection but not the rest of ASAN). Building with just LSAN results in a build that runs much faster. That makes the build-test-fix cycle more pleasant. In the long run, once we've fixed or suppressed all the leaks, it will probably be worth turning leak-detection on for ASAN and just using that (to check both leaks _and_ memory errors in a single test run). But there's still a lot of work before we get there. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/test-lib.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/t/test-lib.sh b/t/test-lib.sh index 62461a6e35..a738540ef2 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -44,6 +44,11 @@ GIT_BUILD_DIR="$TEST_DIRECTORY"/.. : ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1} export ASAN_OPTIONS +# If LSAN is in effect we _do_ want leak checking, but we still +# want to abort so that we notice the problems. +: ${LSAN_OPTIONS=abort_on_error=1} +export LSAN_OPTIONS + ################################################################ # It appears that people try to run tests without building... "$GIT_BUILD_DIR/git" >/dev/null From fe6a01af8aa913fc23c1486251da6f6f08601816 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 09:04:10 -0400 Subject: [PATCH 03/10] add: free leaked pathspec after add_files_to_cache() After run_diff_files, we throw away the rev_info struct, including the pathspec that we copied into it, leaking the memory. this is probably not a big deal in practice. We usually only run this once per process, and the leak is proportional to the pathspec list we're already holding in memory. But it's still a leak, and it pollutes leak-checker output, making it harder to find important leaks. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/add.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/add.c b/builtin/add.c index c20548e4f5..ef625e3fb8 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -119,6 +119,7 @@ int add_files_to_cache(const char *prefix, rev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG; rev.max_count = 0; /* do not compare unmerged paths with stage #2 */ run_diff_files(&rev, DIFF_RACY_IS_MODIFIED); + clear_pathspec(&rev.prune_data); return !!data.add_errors; } From baddc96b2cbf66fdcde87509392dc8da6a77f452 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 09:04:14 -0400 Subject: [PATCH 04/10] update-index: fix cache entry leak in add_one_file() When we fail to add the cache entry to the index, we end up just leaking the struct. We should follow the pattern of the early-return above and free it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/update-index.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/update-index.c b/builtin/update-index.c index d562f2ec69..d955cd56b3 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -287,8 +287,10 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len } option = allow_add ? ADD_CACHE_OK_TO_ADD : 0; option |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0; - if (add_cache_entry(ce, option)) + if (add_cache_entry(ce, option)) { + free(ce); return error("%s: cannot add to the index - missing --add option?", path); + } return 0; } From 6c6b08d26999405a5c67dbabe6f9f232d658fd26 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 09:04:20 -0400 Subject: [PATCH 05/10] config: plug user_config leak We generate filenames for the user_config ("~/.gitconfig") and the xdg config ("$XDG_CONFIG_HOME/git/config") and then decide which to use by looking at the filesystem. But after selecting one, the unused string is just leaked. This is a tiny leak, but it creates noise in leak-checker output. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/config.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 70ff231e9c..52a4606243 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -518,10 +518,13 @@ int cmd_config(int argc, const char **argv, const char *prefix) die("$HOME not set"); if (access_or_warn(user_config, R_OK, 0) && - xdg_config && !access_or_warn(xdg_config, R_OK, 0)) + xdg_config && !access_or_warn(xdg_config, R_OK, 0)) { given_config_source.file = xdg_config; - else + free(user_config); + } else { given_config_source.file = user_config; + free(xdg_config); + } } else if (use_system_config) given_config_source.file = git_etc_gitconfig(); From e9ce897b9fe5a92719eb991ecc0291dd72aab868 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 09:04:28 -0400 Subject: [PATCH 06/10] reset: make tree counting less confusing Depending on whether we're in --keep mode, git-reset may feed one or two trees to unpack_trees(). We start a counter at "1" and then increment it to "2" only for the two-tree case. But that means we must always subtract one to find the correct array slot to fill with each descriptor. Instead, let's start at "0" and just increment our counter after adding each tree. This skips the extra subtraction, and will make things much easier when we start to actually free our tree buffers. While we're at it, let's make the first allocation use the slot at "desc + nr", too, even though we know "nr" is 0 at that point. It makes the two fill_tree_descriptor() calls consistent (always "desc + nr", followed by always incrementing "nr"). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/reset.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index d72c7d1c96..70230056a8 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -44,7 +44,7 @@ static inline int is_merge(void) static int reset_index(const struct object_id *oid, int reset_type, int quiet) { - int nr = 1; + int nr = 0; struct tree_desc desc[2]; struct tree *tree; struct unpack_trees_options opts; @@ -75,14 +75,16 @@ static int reset_index(const struct object_id *oid, int reset_type, int quiet) struct object_id head_oid; if (get_oid("HEAD", &head_oid)) return error(_("You do not have a valid HEAD.")); - if (!fill_tree_descriptor(desc, &head_oid)) + if (!fill_tree_descriptor(desc + nr, &head_oid)) return error(_("Failed to find tree of HEAD.")); nr++; opts.fn = twoway_merge; } - if (!fill_tree_descriptor(desc + nr - 1, oid)) + if (!fill_tree_descriptor(desc + nr, oid)) return error(_("Failed to find tree of %s."), oid_to_hex(oid)); + nr++; + if (unpack_trees(nr, desc, &opts)) return -1; From afbb8838b7d4d1887da1e1871f8e9ccd01ae1138 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 09:04:51 -0400 Subject: [PATCH 07/10] reset: free allocated tree buffers We read the tree objects with fill_tree_descriptor(), but never actually free the resulting buffers, causing a memory leak. This isn't a huge deal because we call this code at most twice per program invocation. But it does potentially double our heap usage if you have large root trees. Let's free the trees before returning. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/reset.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 70230056a8..0295c961a3 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -44,10 +44,11 @@ static inline int is_merge(void) static int reset_index(const struct object_id *oid, int reset_type, int quiet) { - int nr = 0; + int i, nr = 0; struct tree_desc desc[2]; struct tree *tree; struct unpack_trees_options opts; + int ret = -1; memset(&opts, 0, sizeof(opts)); opts.head_idx = 1; @@ -81,19 +82,26 @@ static int reset_index(const struct object_id *oid, int reset_type, int quiet) opts.fn = twoway_merge; } - if (!fill_tree_descriptor(desc + nr, oid)) - return error(_("Failed to find tree of %s."), oid_to_hex(oid)); + if (!fill_tree_descriptor(desc + nr, oid)) { + error(_("Failed to find tree of %s."), oid_to_hex(oid)); + goto out; + } nr++; if (unpack_trees(nr, desc, &opts)) - return -1; + goto out; if (reset_type == MIXED || reset_type == HARD) { tree = parse_tree_indirect(oid); prime_cache_tree(&the_index, tree); } - return 0; + ret = 0; + +out: + for (i = 0; i < nr; i++) + free((void *)desc[i].buffer); + return ret; } static void print_new_head_line(struct commit *commit) From f9b7573f6b0039d298de826e22c636db79b9c919 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 09:04:57 -0400 Subject: [PATCH 08/10] repository: free fields before overwriting them It's possible that the repository data may be initialized twice (e.g., after doing a chdir() to the top of the worktree we may have to adjust a relative git_dir path). We should free() any existing fields before assigning to them to avoid leaks. This should be safe, as the fields are set based on the environment or on other strings like the gitdir or commondir. That makes it impossible that we are feeding an alias to the just-freed string. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- environment.c | 4 +++- repository.c | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/environment.c b/environment.c index 3fd4b10845..f1f934b6fd 100644 --- a/environment.c +++ b/environment.c @@ -97,7 +97,7 @@ int ignore_untracked_cache_config; /* This is set by setup_git_dir_gently() and/or git_default_config() */ char *git_work_tree_cfg; -static const char *namespace; +static char *namespace; static const char *super_prefix; @@ -152,8 +152,10 @@ void setup_git_env(void) if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT)) check_replace_refs = 0; replace_ref_base = getenv(GIT_REPLACE_REF_BASE_ENVIRONMENT); + free(git_replace_ref_base); git_replace_ref_base = xstrdup(replace_ref_base ? replace_ref_base : "refs/replace/"); + free(namespace); namespace = expand_namespace(getenv(GIT_NAMESPACE_ENVIRONMENT)); shallow_file = getenv(GIT_SHALLOW_FILE_ENVIRONMENT); if (shallow_file) diff --git a/repository.c b/repository.c index f107af7d76..52f1821c6b 100644 --- a/repository.c +++ b/repository.c @@ -40,11 +40,15 @@ static void repo_setup_env(struct repository *repo) repo->different_commondir = find_common_dir(&sb, repo->gitdir, !repo->ignore_env); + free(repo->commondir); repo->commondir = strbuf_detach(&sb, NULL); + free(repo->objectdir); repo->objectdir = git_path_from_env(DB_ENVIRONMENT, repo->commondir, "objects", !repo->ignore_env); + free(repo->graft_file); repo->graft_file = git_path_from_env(GRAFT_ENVIRONMENT, repo->commondir, "info/grafts", !repo->ignore_env); + free(repo->index_file); repo->index_file = git_path_from_env(INDEX_ENVIRONMENT, repo->gitdir, "index", !repo->ignore_env); } From 1fb2b636c672fea06fdc5f50d5c0ed44117ae45a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 09:05:01 -0400 Subject: [PATCH 09/10] set_git_dir: handle feeding gitdir to itself Ideally we'd free the existing gitdir field before assigning the new one, to avoid a memory leak. But we can't do so safely because some callers do the equivalent of: set_git_dir(get_git_dir()); We can detect that case as a noop, but there are even more complicated cases like: set_git_dir(remove_leading_path(worktree, get_git_dir()); where we really do need to do some work, but the original string must remain valid. Rather than put the burden on callers to make a copy of the string (only to free it later, since we'll make a copy of it ourselves), let's solve the problem inside set_git_dir(). We can make a copy of the pointer for the old gitdir, and then avoid freeing it until after we've made our new copy. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- repository.c | 10 +++------- setup.c | 5 ----- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/repository.c b/repository.c index 52f1821c6b..97c732bd48 100644 --- a/repository.c +++ b/repository.c @@ -56,16 +56,12 @@ static void repo_setup_env(struct repository *repo) void repo_set_gitdir(struct repository *repo, const char *path) { const char *gitfile = read_gitfile(path); + char *old_gitdir = repo->gitdir; - /* - * NEEDSWORK: Eventually we want to be able to free gitdir and the rest - * of the environment before reinitializing it again, but we have some - * crazy code paths where we try to set gitdir with the current gitdir - * and we don't want to free gitdir before copying the passed in value. - */ repo->gitdir = xstrdup(gitfile ? gitfile : path); - repo_setup_env(repo); + + free(old_gitdir); } /* diff --git a/setup.c b/setup.c index 23950173fc..6d8380acd2 100644 --- a/setup.c +++ b/setup.c @@ -399,11 +399,6 @@ void setup_work_tree(void) if (getenv(GIT_WORK_TREE_ENVIRONMENT)) setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1); - /* - * NEEDSWORK: this call can essentially be set_git_dir(get_git_dir()) - * which can cause some problems when trying to free the old value of - * gitdir. - */ set_git_dir(remove_leading_path(git_dir, work_tree)); initialized = 1; } From 0e5bba53af7d6f911e99589d736cdf06badad0fe Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 8 Sep 2017 02:38:41 -0400 Subject: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives It's a common pattern in git commands to allocate some memory that should last for the lifetime of the program and then not bother to free it, relying on the OS to throw it away. This keeps the code simple, and it's fast (we don't waste time traversing structures or calling free at the end of the program). But it also triggers warnings from memory-leak checkers like valgrind or LSAN. They know that the memory was still allocated at program exit, but they don't know _when_ the leaked memory stopped being useful. If it was early in the program, then it's probably a real and important leak. But if it was used right up until program exit, it's not an interesting leak and we'd like to suppress it so that we can see the real leaks. This patch introduces an UNLEAK() macro that lets us do so. To understand its design, let's first look at some of the alternatives. Unfortunately the suppression systems offered by leak-checking tools don't quite do what we want. A leak-checker basically knows two things: 1. Which blocks were allocated via malloc, and the callstack during the allocation. 2. Which blocks were left un-freed at the end of the program (and which are unreachable, but more on that later). Their suppressions work by mentioning the function or callstack of a particular allocation, and marking it as OK to leak. So imagine you have code like this: int cmd_foo(...) { /* this allocates some memory */ char *p = some_function(); printf("%s", p); return 0; } You can say "ignore allocations from some_function(), they're not leaks". But that's not right. That function may be called elsewhere, too, and we would potentially want to know about those leaks. So you can say "ignore the callstack when main calls some_function". That works, but your annotations are brittle. In this case it's only two functions, but you can imagine that the actual allocation is much deeper. If any of the intermediate code changes, you have to update the suppression. What we _really_ want to say is that "the value assigned to p at the end of the function is not a real leak". But leak-checkers can't understand that; they don't know about "p" in the first place. However, we can do something a little bit tricky if we make some assumptions about how leak-checkers work. They generally don't just report all un-freed blocks. That would report even globals which are still accessible when the leak-check is run. Instead they take some set of memory (like BSS) as a root and mark it as "reachable". Then they scan the reachable blocks for anything that looks like a pointer to a malloc'd block, and consider that block reachable. And then they scan those blocks, and so on, transitively marking anything reachable from a global as "not leaked" (or at least leaked in a different category). So we can mark the value of "p" as reachable by putting it into a variable with program lifetime. One way to do that is to just mark "p" as static. But that actually affects the run-time behavior if the function is called twice (you aren't likely to call main() twice, but some of our cmd_*() functions are called from other commands). Instead, we can trick the leak-checker by putting the value into _any_ reachable bytes. This patch keeps a global linked-list of bytes copied from "unleaked" variables. That list is reachable even at program exit, which confers recursive reachability on whatever values we unleak. In other words, you can do: int cmd_foo(...) { char *p = some_function(); printf("%s", p); UNLEAK(p); return 0; } to annotate "p" and suppress the leak report. But wait, couldn't we just say "free(p)"? In this toy example, yes. But UNLEAK()'s byte-copying strategy has several advantages over actually freeing the memory: 1. It's recursive across structures. In many cases our "p" is not just a pointer, but a complex struct whose fields may have been allocated by a sub-function. And in some cases (e.g., dir_struct) we don't even have a function which knows how to free all of the struct members. By marking the struct itself as reachable, that confers reachability on any pointers it contains (including those found in embedded structs, or reachable by walking heap blocks recursively. 2. It works on cases where we're not sure if the value is allocated or not. For example: char *p = argc > 1 ? argv[1] : some_function(); It's safe to use UNLEAK(p) here, because it's not freeing any memory. In the case that we're pointing to argv here, the reachability checker will just ignore our bytes. 3. Likewise, it works even if the variable has _already_ been freed. We're just copying the pointer bytes. If the block has been freed, the leak-checker will skip over those bytes as uninteresting. 4. Because it's not actually freeing memory, you can UNLEAK() before we are finished accessing the variable. This is helpful in cases like this: char *p = some_function(); return another_function(p); Writing this with free() requires: int ret; char *p = some_function(); ret = another_function(p); free(p); return ret; But with unleak we can just write: char *p = some_function(); UNLEAK(p); return another_function(p); This patch adds the UNLEAK() macro and enables it automatically when Git is compiled with SANITIZE=leak. In normal builds it's a noop, so we pay no runtime cost. It also adds some UNLEAK() annotations to show off how the feature works. On top of other recent leak fixes, these are enough to get t0000 and t0001 to pass when compiled with LSAN. Note the case in commit.c which actually converts a strbuf_release() into an UNLEAK. This code was already non-leaky, but the free didn't do anything useful, since we're exiting. Converting it to an annotation means that non-leak-checking builds pay no runtime cost. The cost is minimal enough that it's probably not worth going on a crusade to convert these kinds of frees to UNLEAKS. I did it here for consistency with the "sb" leak (though it would have been equally correct to go the other way, and turn them both into strbuf_release() calls). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Makefile | 3 +++ builtin/add.c | 2 ++ builtin/commit.c | 3 ++- builtin/config.c | 4 ++++ builtin/init-db.c | 2 ++ builtin/ls-files.c | 1 + builtin/worktree.c | 2 ++ git-compat-util.h | 20 ++++++++++++++++++++ usage.c | 15 +++++++++++++++ 9 files changed, 51 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index f2bb7f2f63..c052f09bba 100644 --- a/Makefile +++ b/Makefile @@ -1036,6 +1036,9 @@ BASIC_CFLAGS += -fno-omit-frame-pointer ifneq ($(filter undefined,$(SANITIZERS)),) BASIC_CFLAGS += -DNO_UNALIGNED_LOADS endif +ifneq ($(filter leak,$(SANITIZERS)),) +BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS +endif endif ifndef sysconfdir diff --git a/builtin/add.c b/builtin/add.c index ef625e3fb8..a648cf4c56 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -515,5 +515,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) die(_("Unable to write new index file")); } + UNLEAK(pathspec); + UNLEAK(dir); return exit_status; } diff --git a/builtin/commit.c b/builtin/commit.c index b3b04f5dd3..58f9747c2f 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1818,6 +1818,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) if (!quiet) print_summary(prefix, &oid, !current_head); - strbuf_release(&err); + UNLEAK(err); + UNLEAK(sb); return 0; } diff --git a/builtin/config.c b/builtin/config.c index 52a4606243..d13daeeb55 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -631,6 +631,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) check_write(); check_argc(argc, 2, 2); value = normalize_value(argv[0], argv[1]); + UNLEAK(value); ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value); if (ret == CONFIG_NOTHING_SET) error(_("cannot overwrite multiple values with a single value\n" @@ -641,6 +642,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) check_write(); check_argc(argc, 2, 3); value = normalize_value(argv[0], argv[1]); + UNLEAK(value); return git_config_set_multivar_in_file_gently(given_config_source.file, argv[0], value, argv[2], 0); } @@ -648,6 +650,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) check_write(); check_argc(argc, 2, 2); value = normalize_value(argv[0], argv[1]); + UNLEAK(value); return git_config_set_multivar_in_file_gently(given_config_source.file, argv[0], value, CONFIG_REGEX_NONE, 0); @@ -656,6 +659,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) check_write(); check_argc(argc, 2, 3); value = normalize_value(argv[0], argv[1]); + UNLEAK(value); return git_config_set_multivar_in_file_gently(given_config_source.file, argv[0], value, argv[2], 1); } diff --git a/builtin/init-db.c b/builtin/init-db.c index 47823f9aa4..c9b7946bad 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -579,6 +579,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix) set_git_work_tree(work_tree); } + UNLEAK(real_git_dir); + flags |= INIT_DB_EXIST_OK; return init_db(git_dir, real_git_dir, template_dir, flags); } diff --git a/builtin/ls-files.c b/builtin/ls-files.c index e1339e6d17..8c713c47ac 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -673,5 +673,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) return bad ? 1 : 0; } + UNLEAK(dir); return 0; } diff --git a/builtin/worktree.c b/builtin/worktree.c index c98e2ce5f5..de26849f55 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -381,6 +381,8 @@ static int add(int ac, const char **av, const char *prefix) branch = opts.new_branch; } + UNLEAK(path); + UNLEAK(opts); return add_worktree(path, branch, &opts); } diff --git a/git-compat-util.h b/git-compat-util.h index 6678b488cc..003e444c46 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1169,4 +1169,24 @@ static inline int is_missing_file_error(int errno_) extern int cmd_main(int, const char **); +/* + * You can mark a stack variable with UNLEAK(var) to avoid it being + * reported as a leak by tools like LSAN or valgrind. The argument + * should generally be the variable itself (not its address and not what + * it points to). It's safe to use this on pointers which may already + * have been freed, or on pointers which may still be in use. + * + * Use this _only_ for a variable that leaks by going out of scope at + * program exit (so only from cmd_* functions or their direct helpers). + * Normal functions, especially those which may be called multiple + * times, should actually free their memory. This is only meant as + * an annotation, and does nothing in non-leak-checking builds. + */ +#ifdef SUPPRESS_ANNOTATED_LEAKS +extern void unleak_memory(const void *ptr, size_t len); +#define UNLEAK(var) unleak_memory(&(var), sizeof(var)); +#else +#define UNLEAK(var) +#endif + #endif diff --git a/usage.c b/usage.c index 1ea7df9a20..cdd534c9df 100644 --- a/usage.c +++ b/usage.c @@ -241,3 +241,18 @@ NORETURN void BUG(const char *fmt, ...) va_end(ap); } #endif + +#ifdef SUPPRESS_ANNOTATED_LEAKS +void unleak_memory(const void *ptr, size_t len) +{ + static struct suppressed_leak_root { + struct suppressed_leak_root *next; + char data[FLEX_ARRAY]; + } *suppressed_leaks; + struct suppressed_leak_root *root; + + FLEX_ALLOC_MEM(root, data, ptr, len); + root->next = suppressed_leaks; + suppressed_leaks = root; +} +#endif