From ce01f92889a9398b5170bf7e28425c94173cfe06 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:51:52 +0200 Subject: [PATCH 01/22] remote: plug memory leak when aliasing URLs When we have a `url.*.insteadOf` configuration, then we end up aliasing URLs when populating remotes. One place where this happens is in `alias_all_urls()`, where we loop through all remotes and then alias each of their URLs. The actual aliasing logic is then contained in `alias_url()`, which returns an allocated string that contains the new URL. This URL replaces the old URL that we have in the strvec that contains all remote URLs. We replace the remote URLs via `strvec_replace()`, which does not hand over ownership of the new string to the vector. Still, we didn't free the aliased URL and thus have a memory leak here. Fix it by freeing the aliased string. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- remote.c | 2 ++ t/t0210-trace2-normal.sh | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/remote.c b/remote.c index f43cf5e7a4..3b898edd23 100644 --- a/remote.c +++ b/remote.c @@ -499,6 +499,7 @@ static void alias_all_urls(struct remote_state *remote_state) if (alias) strvec_replace(&remote_state->remotes[i]->pushurl, j, alias); + free(alias); } add_pushurl_aliases = remote_state->remotes[i]->pushurl.nr == 0; for (j = 0; j < remote_state->remotes[i]->url.nr; j++) { @@ -512,6 +513,7 @@ static void alias_all_urls(struct remote_state *remote_state) if (alias) strvec_replace(&remote_state->remotes[i]->url, j, alias); + free(alias); } } } diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh index c312657a12..b9adc94aab 100755 --- a/t/t0210-trace2-normal.sh +++ b/t/t0210-trace2-normal.sh @@ -2,7 +2,7 @@ test_description='test trace2 facility (normal target)' -TEST_PASSES_SANITIZE_LEAK=false +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Turn off any inherited trace2 settings for this test. From ce15f9eb9eb9e7ac3d5095b19cd7b8c7c5bfc22b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:51:55 +0200 Subject: [PATCH 02/22] git: fix leaking system paths Git has some flags to make it output system paths as they have been compiled into Git. This is done by calling `system_path()`, which returns an allocated string. This string isn't ever free'd though, creating a memory leak. Plug those leaks. While they are surfaced by t0211, there are more memory leaks looming exposed by that test suite and it thus does not yet pass with the memory leak checker enabled. Helped-by: Taylor Blau Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- git.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/git.c b/git.c index e35af9b0e5..9a618a2740 100644 --- a/git.c +++ b/git.c @@ -143,6 +143,13 @@ void setup_auto_pager(const char *cmd, int def) commit_pager_choice(); } +static void print_system_path(const char *path) +{ + char *s_path = system_path(path); + puts(s_path); + free(s_path); +} + static int handle_options(const char ***argv, int *argc, int *envchanged) { const char **orig_argv = *argv; @@ -173,15 +180,15 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) exit(0); } } else if (!strcmp(cmd, "--html-path")) { - puts(system_path(GIT_HTML_PATH)); + print_system_path(GIT_HTML_PATH); trace2_cmd_name("_query_"); exit(0); } else if (!strcmp(cmd, "--man-path")) { - puts(system_path(GIT_MAN_PATH)); + print_system_path(GIT_MAN_PATH); trace2_cmd_name("_query_"); exit(0); } else if (!strcmp(cmd, "--info-path")) { - puts(system_path(GIT_INFO_PATH)); + print_system_path(GIT_INFO_PATH); trace2_cmd_name("_query_"); exit(0); } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) { From aa9ef614dc281d123e8f18d19c9abecf82295c0e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:51:58 +0200 Subject: [PATCH 03/22] object-file: fix memory leak when reading corrupted headers When reading corrupt object headers in `read_loose_object()`, we bail out immediately. This causes a memory leak though because we would have already initialized the zstream in `unpack_loose_header()`, and it is the callers responsibility to finish the zstream even on error. While this feels weird, other callsites do it correctly already. Fix this leak by ending the zstream even on errors. We may want to revisit this interface in the future such that the callee handles this for us already when there was an error. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- object-file.c | 1 + t/t1450-fsck.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/object-file.c b/object-file.c index 065103be3e..7c65c435cd 100644 --- a/object-file.c +++ b/object-file.c @@ -2954,6 +2954,7 @@ int read_loose_object(const char *path, if (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr), NULL) != ULHR_OK) { error(_("unable to unpack header of %s"), path); + git_inflate_end(&stream); goto out; } diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index 8a456b1142..280cbf3e03 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -6,6 +6,7 @@ test_description='git fsck random collection of tests * (main) A ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' From 9ddd5f755d78dd2a3119b63d5f78ca93e4fd3eba Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:00 +0200 Subject: [PATCH 04/22] object-name: fix leaking symlink paths in object context The object context may be populated with symlink contents when reading a symlink, but the associated strbuf doesn't ever get released when releasing the object context, causing a memory leak. Plug it. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- object-name.c | 1 + t/t1006-cat-file.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/object-name.c b/object-name.c index 240a93e7ce..e39fa50e47 100644 --- a/object-name.c +++ b/object-name.c @@ -1765,6 +1765,7 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name) void object_context_release(struct object_context *ctx) { free(ctx->path); + strbuf_release(&ctx->symlink_path); } /* diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index ff9bf213aa..d36cd7c086 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh @@ -2,6 +2,7 @@ test_description='git cat-file' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_cmdmode_usage () { From c81dcf630cec64d0042f08f7210300671ed5b926 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:03 +0200 Subject: [PATCH 05/22] bulk-checkin: fix leaking state TODO When flushing a bulk-checking to disk we also reset the `struct bulk_checkin_packfile` state. But while we free some of its members, others aren't being free'd, leading to memory leaks: - The temporary packfile name is not getting freed. - The `struct hashfile` only gets freed in case we end up calling `finalize_hashfile()`. There are code paths though where that is not the case, namely when nothing has been written. For this, we need to make `free_hashfile()` public. Fix those leaks. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- bulk-checkin.c | 2 ++ csum-file.c | 2 +- csum-file.h | 10 ++++++++++ t/t1050-large.sh | 1 + 4 files changed, 14 insertions(+), 1 deletion(-) diff --git a/bulk-checkin.c b/bulk-checkin.c index da8673199b..9089c214fa 100644 --- a/bulk-checkin.c +++ b/bulk-checkin.c @@ -61,6 +61,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state) if (state->nr_written == 0) { close(state->f->fd); + free_hashfile(state->f); unlink(state->pack_tmp_name); goto clear_exit; } else if (state->nr_written == 1) { @@ -83,6 +84,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state) free(state->written[i]); clear_exit: + free(state->pack_tmp_name); free(state->written); memset(state, 0, sizeof(*state)); diff --git a/csum-file.c b/csum-file.c index 8abbf01325..7e0ece1305 100644 --- a/csum-file.c +++ b/csum-file.c @@ -56,7 +56,7 @@ void hashflush(struct hashfile *f) } } -static void free_hashfile(struct hashfile *f) +void free_hashfile(struct hashfile *f) { free(f->buffer); free(f->check_buffer); diff --git a/csum-file.h b/csum-file.h index 566e05cbd2..ca553eba17 100644 --- a/csum-file.h +++ b/csum-file.h @@ -46,6 +46,16 @@ int hashfile_truncate(struct hashfile *, struct hashfile_checkpoint *); struct hashfile *hashfd(int fd, const char *name); struct hashfile *hashfd_check(const char *name); struct hashfile *hashfd_throughput(int fd, const char *name, struct progress *tp); + +/* + * Free the hashfile without flushing its contents to disk. This only + * needs to be called when not calling `finalize_hashfile()`. + */ +void free_hashfile(struct hashfile *f); + +/* + * Finalize the hashfile by flushing data to disk and free'ing it. + */ int finalize_hashfile(struct hashfile *, unsigned char *, enum fsync_component, unsigned int); void hashwrite(struct hashfile *, const void *, unsigned int); void hashflush(struct hashfile *f); diff --git a/t/t1050-large.sh b/t/t1050-large.sh index c71932b024..ed638f6644 100755 --- a/t/t1050-large.sh +++ b/t/t1050-large.sh @@ -3,6 +3,7 @@ test_description='adding and checking out large blobs' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'core.bigFileThreshold must be non-negative' ' From d1c53f6703f34eeb3442070016db6e4d1f5676d2 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:06 +0200 Subject: [PATCH 06/22] read-cache: fix leaking hashfile when writing index fails In `do_write_index()`, we use a `struct hashfile` to write the index with a trailer hash. In case the write fails though, we never clean up the allocated `hashfile` state and thus leak memory. Refactor the code to have a common exit path where we can free this and other allocated memory. While at it, refactor our use of `strbuf`s such that we reuse the same buffer to avoid some unneeded allocations. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- read-cache.c | 97 ++++++++++++++++++------------ t/t1601-index-bogus.sh | 2 + t/t2107-update-index-basic.sh | 1 + t/t7008-filter-branch-null-sha1.sh | 1 + 4 files changed, 62 insertions(+), 39 deletions(-) diff --git a/read-cache.c b/read-cache.c index 48bf24f87c..36821fe5b5 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2840,8 +2840,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, int csum_fsync_flag; int ieot_entries = 1; struct index_entry_offset_table *ieot = NULL; - int nr, nr_threads; struct repository *r = istate->repo; + struct strbuf sb = STRBUF_INIT; + int nr, nr_threads, ret; f = hashfd(tempfile->fd, tempfile->filename.buf); @@ -2962,8 +2963,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, strbuf_release(&previous_name_buf); if (err) { - free(ieot); - return err; + ret = err; + goto out; } offset = hashfile_total(f); @@ -2985,20 +2986,20 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, * index. */ if (ieot) { - struct strbuf sb = STRBUF_INIT; + strbuf_reset(&sb); write_ieot_extension(&sb, ieot); err = write_index_ext_header(f, eoie_c, CACHE_EXT_INDEXENTRYOFFSETTABLE, sb.len) < 0; hashwrite(f, sb.buf, sb.len); - strbuf_release(&sb); - free(ieot); - if (err) - return -1; + if (err) { + ret = -1; + goto out; + } } if (write_extensions & WRITE_SPLIT_INDEX_EXTENSION && istate->split_index) { - struct strbuf sb = STRBUF_INIT; + strbuf_reset(&sb); if (istate->sparse_index) die(_("cannot write split index for a sparse index")); @@ -3007,59 +3008,66 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, write_index_ext_header(f, eoie_c, CACHE_EXT_LINK, sb.len) < 0; hashwrite(f, sb.buf, sb.len); - strbuf_release(&sb); - if (err) - return -1; + if (err) { + ret = -1; + goto out; + } } if (write_extensions & WRITE_CACHE_TREE_EXTENSION && !drop_cache_tree && istate->cache_tree) { - struct strbuf sb = STRBUF_INIT; + strbuf_reset(&sb); cache_tree_write(&sb, istate->cache_tree); err = write_index_ext_header(f, eoie_c, CACHE_EXT_TREE, sb.len) < 0; hashwrite(f, sb.buf, sb.len); - strbuf_release(&sb); - if (err) - return -1; + if (err) { + ret = -1; + goto out; + } } if (write_extensions & WRITE_RESOLVE_UNDO_EXTENSION && istate->resolve_undo) { - struct strbuf sb = STRBUF_INIT; + strbuf_reset(&sb); resolve_undo_write(&sb, istate->resolve_undo); err = write_index_ext_header(f, eoie_c, CACHE_EXT_RESOLVE_UNDO, sb.len) < 0; hashwrite(f, sb.buf, sb.len); - strbuf_release(&sb); - if (err) - return -1; + if (err) { + ret = -1; + goto out; + } } if (write_extensions & WRITE_UNTRACKED_CACHE_EXTENSION && istate->untracked) { - struct strbuf sb = STRBUF_INIT; + strbuf_reset(&sb); write_untracked_extension(&sb, istate->untracked); err = write_index_ext_header(f, eoie_c, CACHE_EXT_UNTRACKED, sb.len) < 0; hashwrite(f, sb.buf, sb.len); - strbuf_release(&sb); - if (err) - return -1; + if (err) { + ret = -1; + goto out; + } } if (write_extensions & WRITE_FSMONITOR_EXTENSION && istate->fsmonitor_last_update) { - struct strbuf sb = STRBUF_INIT; + strbuf_reset(&sb); write_fsmonitor_extension(&sb, istate); err = write_index_ext_header(f, eoie_c, CACHE_EXT_FSMONITOR, sb.len) < 0; hashwrite(f, sb.buf, sb.len); - strbuf_release(&sb); - if (err) - return -1; + if (err) { + ret = -1; + goto out; + } } if (istate->sparse_index) { - if (write_index_ext_header(f, eoie_c, CACHE_EXT_SPARSE_DIRECTORIES, 0) < 0) - return -1; + if (write_index_ext_header(f, eoie_c, CACHE_EXT_SPARSE_DIRECTORIES, 0) < 0) { + ret = -1; + goto out; + } } /* @@ -3069,14 +3077,15 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, * when loading the shared index. */ if (eoie_c) { - struct strbuf sb = STRBUF_INIT; + strbuf_reset(&sb); write_eoie_extension(&sb, eoie_c, offset); err = write_index_ext_header(f, NULL, CACHE_EXT_ENDOFINDEXENTRIES, sb.len) < 0; hashwrite(f, sb.buf, sb.len); - strbuf_release(&sb); - if (err) - return -1; + if (err) { + ret = -1; + goto out; + } } csum_fsync_flag = 0; @@ -3085,13 +3094,16 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, finalize_hashfile(f, istate->oid.hash, FSYNC_COMPONENT_INDEX, CSUM_HASH_IN_STREAM | csum_fsync_flag); + f = NULL; if (close_tempfile_gently(tempfile)) { - error(_("could not close '%s'"), get_tempfile_path(tempfile)); - return -1; + ret = error(_("could not close '%s'"), get_tempfile_path(tempfile)); + goto out; + } + if (stat(get_tempfile_path(tempfile), &st)) { + ret = -1; + goto out; } - if (stat(get_tempfile_path(tempfile), &st)) - return -1; istate->timestamp.sec = (unsigned int)st.st_mtime; istate->timestamp.nsec = ST_MTIME_NSEC(st); trace_performance_since(start, "write index, changed mask = %x", istate->cache_changed); @@ -3105,7 +3117,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, trace2_data_intmax("index", the_repository, "write/cache_nr", istate->cache_nr); - return 0; + ret = 0; + +out: + if (f) + free_hashfile(f); + strbuf_release(&sb); + free(ieot); + return ret; } void set_alternate_index_output(const char *name) diff --git a/t/t1601-index-bogus.sh b/t/t1601-index-bogus.sh index 4171f1e141..5dcc101882 100755 --- a/t/t1601-index-bogus.sh +++ b/t/t1601-index-bogus.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='test handling of bogus index entries' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'create tree with null sha1' ' diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh index cc72ead79f..f0eab13f96 100755 --- a/t/t2107-update-index-basic.sh +++ b/t/t2107-update-index-basic.sh @@ -5,6 +5,7 @@ test_description='basic update-index tests Tests for command-line parsing and basic operation. ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'update-index --nonsense fails' ' diff --git a/t/t7008-filter-branch-null-sha1.sh b/t/t7008-filter-branch-null-sha1.sh index 93fbc92b8d..0ce8fd2c89 100755 --- a/t/t7008-filter-branch-null-sha1.sh +++ b/t/t7008-filter-branch-null-sha1.sh @@ -2,6 +2,7 @@ test_description='filter-branch removal of trees with null sha1' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup: base commits' ' From 5f6519b62c636edfff4e1ffd6591917a53616b42 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:12 +0200 Subject: [PATCH 07/22] submodule-config: fix leaking name entry when traversing submodules We traverse through submodules in the tree via `tree_entry()`, passing to it a `struct name_entry` that it is supposed to populate with the tree entry's contents. We unnecessarily allocate this variable instead of passing a variable that is allocated on the stack, and the ultimately don't even free that variable. This is unnecessary and leaks memory. Convert the variable to instead be allocated on the stack to plug the memory leak. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- submodule-config.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/submodule-config.c b/submodule-config.c index 9b0bb0b9f4..c8f2bb2bdd 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -899,27 +899,25 @@ static void traverse_tree_submodules(struct repository *r, { struct tree_desc tree; struct submodule_tree_entry *st_entry; - struct name_entry *name_entry; + struct name_entry name_entry; char *tree_path = NULL; - name_entry = xmalloc(sizeof(*name_entry)); - fill_tree_descriptor(r, &tree, treeish_name); - while (tree_entry(&tree, name_entry)) { + while (tree_entry(&tree, &name_entry)) { if (prefix) tree_path = - mkpathdup("%s/%s", prefix, name_entry->path); + mkpathdup("%s/%s", prefix, name_entry.path); else - tree_path = xstrdup(name_entry->path); + tree_path = xstrdup(name_entry.path); - if (S_ISGITLINK(name_entry->mode) && + if (S_ISGITLINK(name_entry.mode) && is_tree_submodule_active(r, root_tree, tree_path)) { ALLOC_GROW(out->entries, out->entry_nr + 1, out->entry_alloc); st_entry = &out->entries[out->entry_nr++]; st_entry->name_entry = xmalloc(sizeof(*st_entry->name_entry)); - *st_entry->name_entry = *name_entry; + *st_entry->name_entry = name_entry; st_entry->submodule = submodule_from_path(r, root_tree, tree_path); st_entry->repo = xmalloc(sizeof(*st_entry->repo)); @@ -927,9 +925,9 @@ static void traverse_tree_submodules(struct repository *r, root_tree)) FREE_AND_NULL(st_entry->repo); - } else if (S_ISDIR(name_entry->mode)) + } else if (S_ISDIR(name_entry.mode)) traverse_tree_submodules(r, root_tree, tree_path, - &name_entry->oid, out); + &name_entry.oid, out); free(tree_path); } } From 648abbe22d55a6004bf6dafa7c0ed209572c9fe9 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:14 +0200 Subject: [PATCH 08/22] config: fix leaking comment character config When the comment line character has been specified multiple times in the configuration, then `git_default_core_config()` will cause a memory leak because it unconditionally copies the string into `comment_line_str` without free'ing the previous value. In fact, it can't easily free the value in the first place because it may contain a string constant. Refactor the code such that we track allocated comment character strings via a separate non-constant variable `comment_line_str_to_free`. Adapt sites that set `comment_line_str` to set both and free the old value that was stored in `comment_line_str_to_free`. This memory leak is being hit in t3404. As there are still other memory leaks in that file we cannot yet mark it as passing with leak checking enabled. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/commit.c | 7 +++++-- config.c | 3 ++- environment.c | 1 + environment.h | 1 + 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 66427ba82d..b2033c4887 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -684,7 +684,9 @@ static void adjust_comment_line_char(const struct strbuf *sb) const char *p; if (!memchr(sb->buf, candidates[0], sb->len)) { - comment_line_str = xstrfmt("%c", candidates[0]); + free(comment_line_str_to_free); + comment_line_str = comment_line_str_to_free = + xstrfmt("%c", candidates[0]); return; } @@ -705,7 +707,8 @@ static void adjust_comment_line_char(const struct strbuf *sb) if (!*p) die(_("unable to select a comment character that is not used\n" "in the current commit message")); - comment_line_str = xstrfmt("%c", *p); + free(comment_line_str_to_free); + comment_line_str = comment_line_str_to_free = xstrfmt("%c", *p); } static void prepare_amend_commit(struct commit *commit, struct strbuf *sb, diff --git a/config.c b/config.c index 6421894614..205660a8fb 100644 --- a/config.c +++ b/config.c @@ -1596,7 +1596,8 @@ static int git_default_core_config(const char *var, const char *value, else if (value[0]) { if (strchr(value, '\n')) return error(_("%s cannot contain newline"), var); - comment_line_str = xstrdup(value); + comment_line_str = value; + FREE_AND_NULL(comment_line_str_to_free); auto_comment_line_char = 0; } else return error(_("%s must have at least one character"), var); diff --git a/environment.c b/environment.c index 5cea2c9f54..1d6c48b52d 100644 --- a/environment.c +++ b/environment.c @@ -114,6 +114,7 @@ int protect_ntfs = PROTECT_NTFS_DEFAULT; * that is subject to stripspace. */ const char *comment_line_str = "#"; +char *comment_line_str_to_free; int auto_comment_line_char; /* Parallel index stat data preload? */ diff --git a/environment.h b/environment.h index e9f01d4d11..0148738ed6 100644 --- a/environment.h +++ b/environment.h @@ -9,6 +9,7 @@ struct strvec; * that is subject to stripspace. */ extern const char *comment_line_str; +extern char *comment_line_str_to_free; extern int auto_comment_line_char; /* From 1ca57bea4a8a4637c4e7d2a2f46677acc4795d81 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:17 +0200 Subject: [PATCH 09/22] builtin/rebase: fix leaking `commit.gpgsign` value In `get_replay_opts()`, we override the `gpg_sign` field that already got populated by `sequencer_init_config()` in case the user has "commit.gpgsign" set in their config. This creates a memory leak because we overwrite the previously assigned value, which may have already pointed to an allocated string. Let's plug the memory leak by freeing the value before we overwrite it. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/rebase.c | 1 + sequencer.c | 1 + t/t3404-rebase-interactive.sh | 1 + t/t3435-rebase-gpg-sign.sh | 1 + t/t7030-verify-tag.sh | 1 + 5 files changed, 5 insertions(+) diff --git a/builtin/rebase.c b/builtin/rebase.c index e3a8e74cfc..2f01d5d3a6 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -186,6 +186,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts) replay.committer_date_is_author_date = opts->committer_date_is_author_date; replay.ignore_date = opts->ignore_date; + free(replay.gpg_sign); replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt); replay.reflog_action = xstrdup(opts->reflog_action); if (opts->strategy) diff --git a/sequencer.c b/sequencer.c index 0291920f0b..cade9b0ca8 100644 --- a/sequencer.c +++ b/sequencer.c @@ -303,6 +303,7 @@ static int git_sequencer_config(const char *k, const char *v, } if (!strcmp(k, "commit.gpgsign")) { + free(opts->gpg_sign); opts->gpg_sign = git_config_bool(k, v) ? xstrdup("") : NULL; return 0; } diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index f92baad138..f171af3061 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -26,6 +26,7 @@ Initial setup: touch file "conflict". ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-rebase.sh diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh index 6aa2aeb628..6e329fea7c 100755 --- a/t/t3435-rebase-gpg-sign.sh +++ b/t/t3435-rebase-gpg-sign.sh @@ -8,6 +8,7 @@ test_description='test rebase --[no-]gpg-sign' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-rebase.sh" . "$TEST_DIRECTORY/lib-gpg.sh" diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh index 6f526c37c2..effa826744 100755 --- a/t/t7030-verify-tag.sh +++ b/t/t7030-verify-tag.sh @@ -4,6 +4,7 @@ test_description='signed tag tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-gpg.sh" From 187b623eeff215b56d8e45ebab1de899e974383a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:20 +0200 Subject: [PATCH 10/22] builtin/notes: fix leaking `struct notes_tree` when merging notes We allocate a `struct notes_tree` in `merge_commit()` which we then initialize via `init_notes()`. It's not really necessary to allocate the structure though given that we never pass ownership to the caller. Furthermore, the allocation leads to a memory leak because despite its name, `free_notes()` doesn't free the `notes_tree` but only clears it. Fix this issue by converting the code to use an on-stack variable. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/notes.c | 9 ++++----- t/t3310-notes-merge-manual-resolve.sh | 1 + t/t3311-notes-merge-fanout.sh | 1 + 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/builtin/notes.c b/builtin/notes.c index d9c356e354..81cbaeec6b 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -807,7 +807,7 @@ static int merge_commit(struct notes_merge_options *o) { struct strbuf msg = STRBUF_INIT; struct object_id oid, parent_oid; - struct notes_tree *t; + struct notes_tree t = {0}; struct commit *partial; struct pretty_print_context pretty_ctx; void *local_ref_to_free; @@ -830,8 +830,7 @@ static int merge_commit(struct notes_merge_options *o) else oidclr(&parent_oid, the_repository->hash_algo); - CALLOC_ARRAY(t, 1); - init_notes(t, "NOTES_MERGE_PARTIAL", combine_notes_overwrite, 0); + init_notes(&t, "NOTES_MERGE_PARTIAL", combine_notes_overwrite, 0); o->local_ref = local_ref_to_free = refs_resolve_refdup(get_main_ref_store(the_repository), @@ -839,7 +838,7 @@ static int merge_commit(struct notes_merge_options *o) if (!o->local_ref) die(_("failed to resolve NOTES_MERGE_REF")); - if (notes_merge_commit(o, t, partial, &oid)) + if (notes_merge_commit(o, &t, partial, &oid)) die(_("failed to finalize notes merge")); /* Reuse existing commit message in reflog message */ @@ -853,7 +852,7 @@ static int merge_commit(struct notes_merge_options *o) is_null_oid(&parent_oid) ? NULL : &parent_oid, 0, UPDATE_REFS_DIE_ON_ERR); - free_notes(t); + free_notes(&t); strbuf_release(&msg); ret = merge_abort(o); free(local_ref_to_free); diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh index 597df5ebc0..04866b89be 100755 --- a/t/t3310-notes-merge-manual-resolve.sh +++ b/t/t3310-notes-merge-manual-resolve.sh @@ -5,6 +5,7 @@ test_description='Test notes merging with manual conflict resolution' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Set up a notes merge scenario with different kinds of conflicts diff --git a/t/t3311-notes-merge-fanout.sh b/t/t3311-notes-merge-fanout.sh index 5b675417e9..ce4144db0f 100755 --- a/t/t3311-notes-merge-fanout.sh +++ b/t/t3311-notes-merge-fanout.sh @@ -5,6 +5,7 @@ test_description='Test notes merging at various fanout levels' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh verify_notes () { From 0662f0dacb59de2aa56f41c7d8645784c2174ca7 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:23 +0200 Subject: [PATCH 11/22] builtin/fast-import: plug trivial memory leaks Plug some trivial memory leaks in git-fast-import(1). Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/fast-import.c | 8 ++++++-- t/t9300-fast-import.sh | 1 + t/t9304-fast-import-marks.sh | 2 ++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/builtin/fast-import.c b/builtin/fast-import.c index d21c4053a7..6dfeb01665 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -206,8 +206,8 @@ static unsigned int object_entry_alloc = 5000; static struct object_entry_pool *blocks; static struct hashmap object_table; static struct mark_set *marks; -static const char *export_marks_file; -static const char *import_marks_file; +static char *export_marks_file; +static char *import_marks_file; static int import_marks_file_from_stream; static int import_marks_file_ignore_missing; static int import_marks_file_done; @@ -3274,6 +3274,7 @@ static void option_import_marks(const char *marks, read_marks(); } + free(import_marks_file); import_marks_file = make_fast_import_path(marks); import_marks_file_from_stream = from_stream; import_marks_file_ignore_missing = ignore_missing; @@ -3316,6 +3317,7 @@ static void option_active_branches(const char *branches) static void option_export_marks(const char *marks) { + free(export_marks_file); export_marks_file = make_fast_import_path(marks); } @@ -3357,6 +3359,8 @@ static void option_rewrite_submodules(const char *arg, struct string_list *list) free(f); string_list_insert(list, s)->util = ms; + + free(s); } static int parse_one_option(const char *option) diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index 1e68426852..3b3c371740 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -7,6 +7,7 @@ test_description='test git fast-import utility' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh ;# test-lib chdir's into trash diff --git a/t/t9304-fast-import-marks.sh b/t/t9304-fast-import-marks.sh index 410a871c52..1f776a80f3 100755 --- a/t/t9304-fast-import-marks.sh +++ b/t/t9304-fast-import-marks.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='test exotic situations with marks' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup dump of basic history' ' From 8ed4e96b5bcbd98e8d9f4593d53d8729280f47b9 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:26 +0200 Subject: [PATCH 12/22] builtin/fast-export: fix leaking diff options Before calling `handle_commit()` in a loop, we set `diffopt.no_free` such that its contents aren't getting freed inside of `handle_commit()`. We never unset that flag though, which means that the structure's allocated resources will ultimately leak. Fix this by unsetting the flag after the loop such that we release its resources via `release_revisions()`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/fast-export.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 4b6e8c6832..fe92d2436c 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -1278,9 +1278,11 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix) revs.diffopt.format_callback = show_filemodify; revs.diffopt.format_callback_data = &paths_of_changed_objects; revs.diffopt.flags.recursive = 1; + revs.diffopt.no_free = 1; while ((commit = get_revision(&revs))) handle_commit(commit, &revs, &paths_of_changed_objects); + revs.diffopt.no_free = 0; handle_tags_and_duplicates(&extra_refs); handle_tags_and_duplicates(&tag_refs); From a0b82622cbb31de66d7f5f0b1e39f349edaeb009 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:31 +0200 Subject: [PATCH 13/22] builtin/fast-export: plug leaking tag names When resolving revisions in `get_tags_and_duplicates()`, we only partially manage the lifetime of `full_name`. In fact, managing its lifetime properly is almost impossible because we put direct pointers to that variable into multiple lists without duplicating the string. The consequence is that these strings will ultimately leak. Refactor the code to make the lists we put those names into duplicate the memory. This allows us to properly free the string as required and thus plugs the memory leak. While this requires us to allocate more data overall, it shouldn't be all that bad given that the number of allocations corresponds with the number of command line parameters, which typically aren't all that many. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/fast-export.c | 17 ++++++++++++----- t/t9351-fast-export-anonymize.sh | 1 + 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/builtin/fast-export.c b/builtin/fast-export.c index fe92d2436c..f253b79322 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -42,8 +42,8 @@ static int full_tree; static int reference_excluded_commits; static int show_original_ids; static int mark_tags; -static struct string_list extra_refs = STRING_LIST_INIT_NODUP; -static struct string_list tag_refs = STRING_LIST_INIT_NODUP; +static struct string_list extra_refs = STRING_LIST_INIT_DUP; +static struct string_list tag_refs = STRING_LIST_INIT_DUP; static struct refspec refspecs = REFSPEC_INIT_FETCH; static int anonymize; static struct hashmap anonymized_seeds; @@ -901,7 +901,7 @@ static void handle_tag(const char *name, struct tag *tag) free(buf); } -static struct commit *get_commit(struct rev_cmdline_entry *e, char *full_name) +static struct commit *get_commit(struct rev_cmdline_entry *e, const char *full_name) { switch (e->item->type) { case OBJ_COMMIT: @@ -932,14 +932,16 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info) struct rev_cmdline_entry *e = info->rev + i; struct object_id oid; struct commit *commit; - char *full_name; + char *full_name = NULL; if (e->flags & UNINTERESTING) continue; if (repo_dwim_ref(the_repository, e->name, strlen(e->name), - &oid, &full_name, 0) != 1) + &oid, &full_name, 0) != 1) { + free(full_name); continue; + } if (refspecs.nr) { char *private; @@ -955,6 +957,7 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info) warning("%s: Unexpected object of type %s, skipping.", e->name, type_name(e->item->type)); + free(full_name); continue; } @@ -963,10 +966,12 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info) break; case OBJ_BLOB: export_blob(&commit->object.oid); + free(full_name); continue; default: /* OBJ_TAG (nested tags) is already handled */ warning("Tag points to object of unexpected type %s, skipping.", type_name(commit->object.type)); + free(full_name); continue; } @@ -979,6 +984,8 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info) if (!*revision_sources_at(&revision_sources, commit)) *revision_sources_at(&revision_sources, commit) = full_name; + else + free(full_name); } string_list_sort(&extra_refs); diff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh index 156a647484..c0d9d7be75 100755 --- a/t/t9351-fast-export-anonymize.sh +++ b/t/t9351-fast-export-anonymize.sh @@ -4,6 +4,7 @@ test_description='basic tests for fast-export --anonymize' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup simple repo' ' From de54b450a3275e98858bf7e83475ad2465cce073 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:33 +0200 Subject: [PATCH 14/22] merge-ort: unconditionally release attributes index We conditionally release the index used for reading gitattributes in merge-ort based on whether or the index has been populated. This check uses `cache_nr` as a condition. This isn't sufficient though, as the variable may be zero even when some other parts of the index have been populated. This leads to memory leaks when sparse checkouts are in use, as we may not end up releasing the sparse checkout patterns. Fix this issue by unconditionally releasing the index. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- merge-ort.c | 3 +-- t/t3507-cherry-pick-conflict.sh | 1 + t/t6421-merge-partial-clone.sh | 1 + t/t6428-merge-conflicts-sparse.sh | 1 + t/t7817-grep-sparse-checkout.sh | 1 + 5 files changed, 5 insertions(+), 2 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index e9d01ac7f7..3752c7e595 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -689,8 +689,7 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti, */ strmap_clear_func(&opti->conflicted, 0); - if (opti->attr_index.cache_nr) /* true iff opt->renormalize */ - discard_index(&opti->attr_index); + discard_index(&opti->attr_index); /* Free memory used by various renames maps */ for (i = MERGE_SIDE1; i <= MERGE_SIDE2; ++i) { diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh index f3947b400a..10e9c91dbb 100755 --- a/t/t3507-cherry-pick-conflict.sh +++ b/t/t3507-cherry-pick-conflict.sh @@ -13,6 +13,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME TEST_CREATE_REPO_NO_TEMPLATE=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh pristine_detach () { diff --git a/t/t6421-merge-partial-clone.sh b/t/t6421-merge-partial-clone.sh index 711b709e75..020375c805 100755 --- a/t/t6421-merge-partial-clone.sh +++ b/t/t6421-merge-partial-clone.sh @@ -26,6 +26,7 @@ test_description="limiting blob downloads when merging with partial clones" # underscore notation is to differentiate different # files that might be renamed into each other's paths.) +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-merge.sh diff --git a/t/t6428-merge-conflicts-sparse.sh b/t/t6428-merge-conflicts-sparse.sh index 9919c3fa7c..8a79bc2e92 100755 --- a/t/t6428-merge-conflicts-sparse.sh +++ b/t/t6428-merge-conflicts-sparse.sh @@ -22,6 +22,7 @@ test_description="merge cases" # underscore notation is to differentiate different # files that might be renamed into each other's paths.) +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-merge.sh diff --git a/t/t7817-grep-sparse-checkout.sh b/t/t7817-grep-sparse-checkout.sh index eb59564565..0ba7817fb7 100755 --- a/t/t7817-grep-sparse-checkout.sh +++ b/t/t7817-grep-sparse-checkout.sh @@ -33,6 +33,7 @@ should leave the following structure in the working tree: But note that sub2 should have the SKIP_WORKTREE bit set. ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' From 2f07d228c3a1c623526ddf38e609dc45c98fb22e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:36 +0200 Subject: [PATCH 15/22] sequencer: release todo list on error paths We're not releasing the `todo_list` in `sequencer_pick_revisions()` when hitting an error path. Restructure the function to have a common exit path such that we can easily clean up the list and thus plug this memory leak. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- sequencer.c | 66 +++++++++++++++++++++++---------- t/t3510-cherry-pick-sequence.sh | 1 + 2 files changed, 48 insertions(+), 19 deletions(-) diff --git a/sequencer.c b/sequencer.c index cade9b0ca8..ea559c31f1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -5490,8 +5490,10 @@ int sequencer_pick_revisions(struct repository *r, int i, res; assert(opts->revs); - if (read_and_refresh_cache(r, opts)) - return -1; + if (read_and_refresh_cache(r, opts)) { + res = -1; + goto out; + } for (i = 0; i < opts->revs->pending.nr; i++) { struct object_id oid; @@ -5506,11 +5508,14 @@ int sequencer_pick_revisions(struct repository *r, enum object_type type = oid_object_info(r, &oid, NULL); - return error(_("%s: can't cherry-pick a %s"), - name, type_name(type)); + res = error(_("%s: can't cherry-pick a %s"), + name, type_name(type)); + goto out; } - } else - return error(_("%s: bad revision"), name); + } else { + res = error(_("%s: bad revision"), name); + goto out; + } } /* @@ -5525,14 +5530,23 @@ int sequencer_pick_revisions(struct repository *r, opts->revs->no_walk && !opts->revs->cmdline.rev->flags) { struct commit *cmit; - if (prepare_revision_walk(opts->revs)) - return error(_("revision walk setup failed")); + + if (prepare_revision_walk(opts->revs)) { + res = error(_("revision walk setup failed")); + goto out; + } + cmit = get_revision(opts->revs); - if (!cmit) - return error(_("empty commit set passed")); + if (!cmit) { + res = error(_("empty commit set passed")); + goto out; + } + if (get_revision(opts->revs)) BUG("unexpected extra commit from walk"); - return single_pick(r, cmit, opts); + + res = single_pick(r, cmit, opts); + goto out; } /* @@ -5542,16 +5556,30 @@ int sequencer_pick_revisions(struct repository *r, */ if (walk_revs_populate_todo(&todo_list, opts) || - create_seq_dir(r) < 0) - return -1; - if (repo_get_oid(r, "HEAD", &oid) && (opts->action == REPLAY_REVERT)) - return error(_("can't revert as initial commit")); - if (save_head(oid_to_hex(&oid))) - return -1; - if (save_opts(opts)) - return -1; + create_seq_dir(r) < 0) { + res = -1; + goto out; + } + + if (repo_get_oid(r, "HEAD", &oid) && (opts->action == REPLAY_REVERT)) { + res = error(_("can't revert as initial commit")); + goto out; + } + + if (save_head(oid_to_hex(&oid))) { + res = -1; + goto out; + } + + if (save_opts(opts)) { + res = -1; + goto out; + } + update_abort_safety_file(); res = pick_commits(r, &todo_list, opts); + +out: todo_list_release(&todo_list); return res; } diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index 7eb52b12ed..93c725bac3 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -12,6 +12,7 @@ test_description='Test cherry-pick continuation features ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Repeat first match 10 times From 4dfd4f1dfe0d3ddbc22392ad7fc124b4820542dd Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:39 +0200 Subject: [PATCH 16/22] unpack-trees: clear index when not propagating it When provided a pointer to a destination index, then `unpack_trees()` will end up copying its `o->internal.result` index into the provided pointer. In those cases it is thus not necessary to free the index, as we have transferred ownership of it. There are cases though where we do not end up transferring ownership of the memory, but `clear_unpack_trees_porcelain()` will never discard the index in that case and thus cause a memory leak. And right now it cannot do so in the first place because we have no indicator of whether we did or didn't transfer ownership of the index. Adapt the code to zero out the index in case we transfer its ownership. Like this, we can now unconditionally discard the index when being asked to clear the `unpack_trees_options`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t3705-add-sparse-checkout.sh | 1 + unpack-trees.c | 2 ++ 2 files changed, 3 insertions(+) diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh index 2bade9e804..6ae45a788d 100755 --- a/t/t3705-add-sparse-checkout.sh +++ b/t/t3705-add-sparse-checkout.sh @@ -2,6 +2,7 @@ test_description='git add in sparse checked out working trees' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh SPARSE_ENTRY_BLOB="" diff --git a/unpack-trees.c b/unpack-trees.c index 7dc884fafd..9a55cb6204 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -210,6 +210,7 @@ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts) { strvec_clear(&opts->internal.msgs_to_free); memset(opts->internal.msgs, 0, sizeof(opts->internal.msgs)); + discard_index(&opts->internal.result); } static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce, @@ -2082,6 +2083,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options o->internal.result.updated_workdir = 1; discard_index(o->dst_index); *o->dst_index = o->internal.result; + memset(&o->internal.result, 0, sizeof(o->internal.result)); } else { discard_index(&o->internal.result); } From 6b15d9ca7f555b7366ed810e2334d614b96361ad Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:42 +0200 Subject: [PATCH 17/22] diff: fix leak when parsing invalid ignore regex option When parsing invalid ignore regexes passed via the `-I` option we don't free already-allocated memory, leading to a memory leak. Fix this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- diff.c | 6 +++++- t/t4013-diff-various.sh | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/diff.c b/diff.c index ebb7538e04..9251c47b72 100644 --- a/diff.c +++ b/diff.c @@ -5464,9 +5464,13 @@ static int diff_opt_ignore_regex(const struct option *opt, regex_t *regex; BUG_ON_OPT_NEG(unset); + regex = xmalloc(sizeof(*regex)); - if (regcomp(regex, arg, REG_EXTENDED | REG_NEWLINE)) + if (regcomp(regex, arg, REG_EXTENDED | REG_NEWLINE)) { + free(regex); return error(_("invalid regex given to -I: '%s'"), arg); + } + ALLOC_GROW(options->ignore_regex, options->ignore_regex_nr + 1, options->ignore_regex_alloc); options->ignore_regex[options->ignore_regex_nr++] = regex; diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index 3855d68dbc..87d248d034 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -8,6 +8,7 @@ test_description='Various diff formatting options' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh From 1bc158e7509851d9cb007273c9f75a238f295d2e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:44 +0200 Subject: [PATCH 18/22] builtin/format-patch: fix various trivial memory leaks There are various memory leaks hit by git-format-patch(1). Basically all of them are trivial, except that un-setting `diffopt.no_free` requires us to unset the `diffopt.file` because we manually close it already. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/log.c | 13 ++++++++++--- t/t4014-format-patch.sh | 1 + 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index a73a767606..f5cb00c643 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1827,12 +1827,14 @@ static struct commit *get_base_commit(const struct format_config *cfg, if (die_on_failure) { die(_("failed to find exact merge base")); } else { + free_commit_list(merge_base); free(rev); return NULL; } } rev[i] = merge_base->item; + free_commit_list(merge_base); } if (rev_nr % 2) @@ -2023,6 +2025,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) const char *rfc = NULL; int creation_factor = -1; const char *signature = git_version_string; + char *signature_to_free = NULL; char *signature_file_arg = NULL; struct keep_callback_data keep_callback_data = { .cfg = &cfg, @@ -2443,7 +2446,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) if (strbuf_read_file(&buf, signature_file, 128) < 0) die_errno(_("unable to read signature file '%s'"), signature_file); - signature = strbuf_detach(&buf, NULL); + signature = signature_to_free = strbuf_detach(&buf, NULL); } else if (cfg.signature) { signature = cfg.signature; } @@ -2548,12 +2551,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) else print_signature(signature, rev.diffopt.file); } - if (output_directory) + if (output_directory) { fclose(rev.diffopt.file); + rev.diffopt.file = NULL; + } } stop_progress(&progress); free(list); - free(branch_name); if (ignore_if_in_upstream) free_patch_ids(&ids); @@ -2565,11 +2569,14 @@ done: strbuf_release(&rdiff_title); free(description_file); free(signature_file_arg); + free(signature_to_free); + free(branch_name); free(to_free); free(rev.message_id); if (rev.ref_message_ids) string_list_clear(rev.ref_message_ids, 0); free(rev.ref_message_ids); + rev.diffopt.no_free = 0; release_revisions(&rev); format_config_release(&cfg); return 0; diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index 884f83fb8a..1c46e963e4 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -8,6 +8,7 @@ test_description='various format-patch tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-terminal.sh From 38678e5df55b77e98b47e7847faf83451eb161de Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:50 +0200 Subject: [PATCH 19/22] userdiff: fix leaking memory for configured diff drivers The userdiff structures may be initialized either statically on the stack or dynamically via configuration keys. In the latter case we end up leaking memory because we didn't have any infrastructure to discern those strings which have been allocated statically and those which have been allocated dynamically. Refactor the code such that we have two pointers for each of these strings: one that holds the value as accessed by other subsystems, and one that points to the same string in case it has been allocated. Like this, we can safely free the second pointer and thus plug those memory leaks. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- range-diff.c | 6 +++-- t/t4018-diff-funcname.sh | 1 + t/t4042-diff-textconv-caching.sh | 2 ++ t/t4048-diff-combined-binary.sh | 1 + t/t4209-log-pickaxe.sh | 2 ++ userdiff.c | 38 ++++++++++++++++++++++++-------- userdiff.h | 4 ++++ 7 files changed, 43 insertions(+), 11 deletions(-) diff --git a/range-diff.c b/range-diff.c index 5f01605550..bbb0952264 100644 --- a/range-diff.c +++ b/range-diff.c @@ -450,8 +450,10 @@ static void output_pair_header(struct diff_options *diffopt, } static struct userdiff_driver section_headers = { - .funcname = { "^ ## (.*) ##$\n" - "^.?@@ (.*)$", REG_EXTENDED } + .funcname = { + .pattern = "^ ## (.*) ##$\n^.?@@ (.*)$", + .cflags = REG_EXTENDED, + }, }; static struct diff_filespec *get_filespec(const char *name, const char *p) diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh index e026fac1f4..8128c30e7f 100755 --- a/t/t4018-diff-funcname.sh +++ b/t/t4018-diff-funcname.sh @@ -5,6 +5,7 @@ test_description='Test custom diff function name patterns' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh index 8ebfa3c1be..a179205394 100755 --- a/t/t4042-diff-textconv-caching.sh +++ b/t/t4042-diff-textconv-caching.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='test textconv caching' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh cat >helper <<'EOF' diff --git a/t/t4048-diff-combined-binary.sh b/t/t4048-diff-combined-binary.sh index 0260cf64f5..f399484bce 100755 --- a/t/t4048-diff-combined-binary.sh +++ b/t/t4048-diff-combined-binary.sh @@ -4,6 +4,7 @@ test_description='combined and merge diff handle binary files and textconv' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup binary merge conflict' ' diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh index 64e1623733..b42fdc54fc 100755 --- a/t/t4209-log-pickaxe.sh +++ b/t/t4209-log-pickaxe.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='log --grep/--author/--regexp-ignore-case/-S/-G' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_log () { diff --git a/userdiff.c b/userdiff.c index c4ebb9ff73..989629149f 100644 --- a/userdiff.c +++ b/userdiff.c @@ -399,8 +399,11 @@ static struct userdiff_driver *userdiff_find_by_namelen(const char *name, size_t static int parse_funcname(struct userdiff_funcname *f, const char *k, const char *v, int cflags) { - if (git_config_string((char **) &f->pattern, k, v) < 0) + f->pattern = NULL; + FREE_AND_NULL(f->pattern_owned); + if (git_config_string(&f->pattern_owned, k, v) < 0) return -1; + f->pattern = f->pattern_owned; f->cflags = cflags; return 0; } @@ -444,20 +447,37 @@ int userdiff_config(const char *k, const char *v) return parse_funcname(&drv->funcname, k, v, REG_EXTENDED); if (!strcmp(type, "binary")) return parse_tristate(&drv->binary, k, v); - if (!strcmp(type, "command")) - return git_config_string((char **) &drv->external.cmd, k, v); + if (!strcmp(type, "command")) { + FREE_AND_NULL(drv->external.cmd); + return git_config_string(&drv->external.cmd, k, v); + } if (!strcmp(type, "trustexitcode")) { drv->external.trust_exit_code = git_config_bool(k, v); return 0; } - if (!strcmp(type, "textconv")) - return git_config_string((char **) &drv->textconv, k, v); + if (!strcmp(type, "textconv")) { + int ret; + FREE_AND_NULL(drv->textconv_owned); + ret = git_config_string(&drv->textconv_owned, k, v); + drv->textconv = drv->textconv_owned; + return ret; + } if (!strcmp(type, "cachetextconv")) return parse_bool(&drv->textconv_want_cache, k, v); - if (!strcmp(type, "wordregex")) - return git_config_string((char **) &drv->word_regex, k, v); - if (!strcmp(type, "algorithm")) - return git_config_string((char **) &drv->algorithm, k, v); + if (!strcmp(type, "wordregex")) { + int ret; + FREE_AND_NULL(drv->word_regex_owned); + ret = git_config_string(&drv->word_regex_owned, k, v); + drv->word_regex = drv->word_regex_owned; + return ret; + } + if (!strcmp(type, "algorithm")) { + int ret; + FREE_AND_NULL(drv->algorithm_owned); + ret = git_config_string(&drv->algorithm_owned, k, v); + drv->algorithm = drv->algorithm_owned; + return ret; + } return 0; } diff --git a/userdiff.h b/userdiff.h index 7565930337..827361b0bc 100644 --- a/userdiff.h +++ b/userdiff.h @@ -8,6 +8,7 @@ struct repository; struct userdiff_funcname { const char *pattern; + char *pattern_owned; int cflags; }; @@ -20,11 +21,14 @@ struct userdiff_driver { const char *name; struct external_diff external; const char *algorithm; + char *algorithm_owned; int binary; struct userdiff_funcname funcname; const char *word_regex; + char *word_regex_owned; const char *word_regex_multi_byte; const char *textconv; + char *textconv_owned; struct notes_cache *textconv_cache; int textconv_want_cache; }; From 0aaca0ec099139f12df6d66cfa09a27ec85106cc Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:53 +0200 Subject: [PATCH 20/22] builtin/log: fix leak when showing converted blob contents In `show_blob_object()`, we proactively call `textconv_object()`. In case we have a textconv driver for this blob we will end up showing the converted contents, otherwise we'll show the un-converted contents of it instead. When the object has been converted we never free the buffer containing the converted contents. Fix this to plug this memory leak. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/log.c | 1 + t/t4030-diff-textconv.sh | 2 ++ 2 files changed, 3 insertions(+) diff --git a/builtin/log.c b/builtin/log.c index f5cb00c643..36769bab3b 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -707,6 +707,7 @@ static int show_blob_object(const struct object_id *oid, struct rev_info *rev, c write_or_die(1, buf, size); object_context_release(&obj_context); + free(buf); return 0; } diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh index a39a626664..29f6d610c2 100755 --- a/t/t4030-diff-textconv.sh +++ b/t/t4030-diff-textconv.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='diff.*.textconv tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh find_diff() { From 36f971f86182a25fa2fa4af680bf79f522a27f60 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:55 +0200 Subject: [PATCH 21/22] diff: free state populated via options The `objfind` and `anchors` members of `struct diff_options` are populated via option parsing, but are never freed in `diff_free()`. Fix this to plug those memory leaks. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- diff.c | 10 ++++++++++ t/t4064-diff-oidfind.sh | 2 ++ t/t4065-diff-anchored.sh | 1 + t/t4069-remerge-diff.sh | 1 + 4 files changed, 14 insertions(+) diff --git a/diff.c b/diff.c index 9251c47b72..4035a9374d 100644 --- a/diff.c +++ b/diff.c @@ -6717,6 +6717,16 @@ void diff_free(struct diff_options *options) if (options->no_free) return; + if (options->objfind) { + oidset_clear(options->objfind); + FREE_AND_NULL(options->objfind); + } + + for (size_t i = 0; i < options->anchors_nr; i++) + free(options->anchors[i]); + FREE_AND_NULL(options->anchors); + options->anchors_nr = options->anchors_alloc = 0; + diff_free_file(options); diff_free_ignore_regex(options); clear_pathspec(&options->pathspec); diff --git a/t/t4064-diff-oidfind.sh b/t/t4064-diff-oidfind.sh index 6d8c8986fc..846f285f77 100755 --- a/t/t4064-diff-oidfind.sh +++ b/t/t4064-diff-oidfind.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='test finding specific blobs in the revision walking' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup ' ' diff --git a/t/t4065-diff-anchored.sh b/t/t4065-diff-anchored.sh index b3f510f040..647537c12e 100755 --- a/t/t4065-diff-anchored.sh +++ b/t/t4065-diff-anchored.sh @@ -2,6 +2,7 @@ test_description='anchored diff algorithm' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success '--anchored' ' diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh index 07323ebafe..888714bbd3 100755 --- a/t/t4069-remerge-diff.sh +++ b/t/t4069-remerge-diff.sh @@ -2,6 +2,7 @@ test_description='remerge-diff handling' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # This test is ort-specific From 77d4b3dd73c44b2c617345a6d9686d2f7f5b8a68 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Aug 2024 08:52:58 +0200 Subject: [PATCH 22/22] builtin/diff: free symmetric diff members We populate a `struct symdiff` in case the user has requested a symmetric diff. Part of this is to populate a `skip` bitmap that indicates which commits shall be ignored in the diff. But while this bitmap is dynamically allocated, we never free it. Fix this by introducing and calling a new `symdiff_release()` function that does this for us. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/diff.c | 6 ++++++ t/t4068-diff-symmetric-merge-base.sh | 1 + t/t4108-apply-threeway.sh | 1 + 3 files changed, 8 insertions(+) diff --git a/builtin/diff.c b/builtin/diff.c index 9b6cdabe15..6eac445579 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -388,6 +388,11 @@ static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym) sym->skip = map; } +static void symdiff_release(struct symdiff *sdiff) +{ + bitmap_free(sdiff->skip); +} + int cmd_diff(int argc, const char **argv, const char *prefix) { int i; @@ -619,6 +624,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) refresh_index_quietly(); release_revisions(&rev); object_array_clear(&ent); + symdiff_release(&sdiff); UNLEAK(blob); return result; } diff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh index eff63c16b0..4d6565e728 100755 --- a/t/t4068-diff-symmetric-merge-base.sh +++ b/t/t4068-diff-symmetric-merge-base.sh @@ -5,6 +5,7 @@ test_description='behavior of diff with symmetric-diff setups and --merge-base' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # build these situations: diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh index c558282bc0..3211e1e65f 100755 --- a/t/t4108-apply-threeway.sh +++ b/t/t4108-apply-threeway.sh @@ -5,6 +5,7 @@ test_description='git apply --3way' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh print_sanitized_conflicted_diff () {