Memory leak fixes (pt.7)
cover Memory leak fixes (pt.7)
Hi,
as usual, whenever the previous part of memory leak fixes has landed, here's the next one. This series brings us down to 80 leaking test suites. The 8th part is almost ready, too, bringing us down to 48 leaking suites. So the end is near, and I hope that our test suites are leak free after the 10th part has landed.
The patch series is built on top of ed155187b4 (Sync with Git 2.46.1, 2024-09-13) with ps/leakfixes-part-6 at 46f6ca2a68 (builtin/repack: fix leaking keep-pack list, 2024-09-05) merged into it.
Thanks!
Patrick
Patrick Steinhardt (23):
builtin/help: fix dangling reference to html_path builtin/help: fix leaking html_path when reading config multiple
times
git: fix leaking argv when handling builtins
submodule: fix leaking update strategy builtin/submodule--helper: clear child process when not running it builtin/submodule--helper: fix leaking error buffer t/helper: fix leaking subrepo in nested submodule config helper builtin/submodule--helper: fix leaking remote ref on errors
dir: fix off by one errors for ignored and untracked entries builtin/pull: fix leaking "ff" option
diff: fix leaking orderfile option
parse-options: free previous value of OPTION_FILENAME
diffcore-order: fix leaking buffer when parsing orderfiles builtin/repack: fix leaking configuration builtin/difftool: plug several trivial memory leaks
trace2: destroy context stored in thread-local storage
submodule: fix leaking submodule ODB paths
grep: fix leaking grep pattern
promisor-remote: fix leaking partial clone filter builtin/maintenance: fix leaking config string builtin/maintenance: fix leak in get_schedule_cmd()
revision: fix leaking parents when simplifying commits
diffcore-break: fix leaking filespecs when merging broken pairs
builtin/difftool.c | 6 + builtin/gc.c | 131 +++++++++++------- builtin/help.c | 16 ++- builtin/pull.c | 11 +- builtin/repack.c | 57 ++++++-- builtin/submodule--helper.c | 26 +++- combine-diff.c | 3 +- diff.c | 7 +- diff.h | 2 +- diffcore-break.c | 4 +- diffcore-order.c | 19 +-- dir.c | 6 +- git.c | 22 ++- grep.c | 2 +- parse-options.c | 22 +-- promisor-remote.c | 2 + revision.c | 5 + submodule-config.c | 2 +- submodule.c | 9 +- submodule.h | 6 +- t/helper/test-submodule-nested-repo-config.c | 2 +- t/t0012-help.sh | 1 + t/t1414-reflog-walk.sh | 1 + ...common-prefixes-and-directory-traversal.sh | 1 + t/t4008-diff-break-rewrite.sh | 2 + t/t4022-diff-rewrite.sh | 1 + t/t4023-diff-rename-typechange.sh | 1 + t/t4031-diff-rewrite-binary.sh | 1 + t/t4056-diff-order.sh | 1 + t/t4204-patch-id.sh | 1 + t/t5310-pack-bitmaps.sh | 1 + t/t5326-multi-pack-bitmaps.sh | 2 + t/t5329-pack-objects-cruft.sh | 2 + t/t6004-rev-list-path-optim.sh | 1 + t/t6019-rev-list-ancestry-path.sh | 1 + t/t6111-rev-list-treesame.sh | 1 + t/t7061-wtstatus-ignore.sh | 1 + t/t7406-submodule-update.sh | 1 + t/t7407-submodule-foreach.sh | 1 + t/t7408-submodule-reference.sh | 2 + t/t7411-submodule-config.sh | 1 + t/t7420-submodule-set-url.sh | 1 + t/t7521-ignored-mode.sh | 1 + t/t7524-commit-summary.sh | 2 + t/t7601-merge-pull-config.sh | 1 + t/t7700-repack.sh | 1 + t/t7800-difftool.sh | 1 + t/t7814-grep-recurse-submodules.sh | 1 + t/t7900-maintenance.sh | 1 + trace2/tr2_tls.c | 10 +- 50 files changed, 279 insertions(+), 124 deletions(-)
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 1/23 builtin/help: fix dangling reference to `html_path`
In get_html_page_path() we may end up assigning the return value of system_path() to the global html_path variable. But as we also assign the returned value to to_free, we will deallocate its memory upon returning from the function. Consequently, html_path will now point to deallocated memory.
Fix this issue by instead assigning the value to a separate local variable.
--- builtin/help.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
builtin/help.c +7 -6
@@ -513,23 +513,24 @@ static void show_info_page(const char *page)static void get_html_page_path (struct strbuf *page_path,const char *page) {struct stat st; +const char *path = html_path;char *to_free =NULL ; -if (!html_path) - html_path = to_free =system_path (GIT_HTML_PATH ); +if (!path) + path = to_free =system_path (GIT_HTML_PATH );/* * Check that the page we're looking for exists. */ -if (!strstr (html_path,"://" )) { -if (stat (mkpath ("%s/%s.html" , html_path, page), &st) +if (!strstr (path,"://" )) { +if (stat (mkpath ("%s/%s.html" , path, page), &st) || !S_ISREG (st.st_mode))die ("'%s/%s.html': documentation file not found." , - html_path, page); + path, page); }strbuf_init (page_path,0 ); -strbuf_addf (page_path,"%s/%s.html" , html_path, page); +strbuf_addf (page_path,"%s/%s.html" , path, page);free (to_free); }
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
Re: [PATCH 01/23] builtin/help: fix dangling reference to `html_path`
Quoted 19 lines · Patrick Steinhardt
In
get_html_page_path()we may end up assigning the return value ofsystem_path()to the globalhtml_pathvariable. But as we also assign the returned value toto_free, we will deallocate its memory upon returning from the function. Consequently,html_pathwill now point to deallocated memory.Fix this issue by instead assigning the value to a separate local variable.
Signed-off-by Patrick Steinhardt <[email protected]>--- builtin/help.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)builtin/help.c +0
-0
builtin/help.c static void show_info_page(const char *page) · line 520
Quoted 10 lines
builtin/help.c +3
-2@@ -513,23 +513,24 @@ static void show_info_page(const char *page)static void get_html_page_path (struct strbuf *page_path,const char *page) {struct stat st; +const char *path = html_path;char *to_free =NULL ; -if (!html_path) - html_path = to_free =system_path (GIT_HTML_PATH ); +if (!path) + path = to_free =system_path (GIT_HTML_PATH );
Previously, html_path was being assigned the return value of system_path() and consequently escaping this scope even though the value was being freed. There is no reason to modify the global value to begin with so we instead assign the value to a local variable. Makes sense.
Quoted 24 lines
builtin/help.c +4
-4/* * Check that the page we're looking for exists. */ -if (!strstr (html_path,"://" )) { -if (stat (mkpath ("%s/%s.html" , html_path, page), &st) +if (!strstr (path,"://" )) { +if (stat (mkpath ("%s/%s.html" , path, page), &st) || !S_ISREG (st.st_mode))die ("'%s/%s.html': documentation file not found." , - html_path, page); + path, page); }strbuf_init (page_path,0 ); -strbuf_addf (page_path,"%s/%s.html" , html_path, page); +strbuf_addf (page_path,"%s/%s.html" , path, page);free (to_free); }Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 2/23 builtin/help: fix leaking `html_path` when reading config multiple times
In reply to Patrick SteinhardtThe html_path variable gets populated via git_help_config(), which puts an allocated string into it if its value has been configured. We do not clear the old value though, which causes a memory leak in case the config exists multiple times.
Plug this leak. The leak is exposed by t0012, but plugging it alone is not sufficient to make the test suite pass.
--- builtin/help.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
builtin/help.c +2 -1
@@ -52,7 +52,7 @@ static enum help_action {HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION , } cmd_mode; -static const char *html_path; +static char *html_path;static int verbose =1 ;static enum help_format help_format =HELP_FORMAT_NONE ;static int exclude_guides;
@@ -409,6 +409,7 @@ static int git_help_config(const char *var, const char *value,if (!strcmp (var,"help.htmlpath" )) {if (!value)return config_error_nonbool (var); +free (html_path); html_path =xstrdup (value);return 0 ; }
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 3/23 git: fix leaking argv when handling builtins
In reply to Patrick SteinhardtIn handle_builtin() we may end up creating an ad-hoc argv array in case we see that the command line contains the "--help" parameter. In this case we observe two memory leaks though:
- We leak the struct strvec itself because we directly exit after calling run_builtin(), without bothering about any cleanups.
- Even if we free'd that vector we'd end up leaking some of its strings because run_builtin() will modify the array.
Plug both of these leaks.
--- git.c | 22 +++++++++++++++++++--- t/t0012-help.sh | 1 + 2 files changed, 20 insertions(+), 3 deletions(-)
git.c +19 -3
@@ -711,6 +711,7 @@ static void strip_extension(const char **argv)static void handle_builtin (int argc,const char **argv) {struct strvec args =STRVEC_INIT ; +const char **argv_copy =NULL ;const char *cmd;struct cmd_struct *builtin;
@@ -731,13 +732,28 @@ static void handle_builtin(int argc, const char **argv) } argc++; - argv = args.v; + +/* +* `run_builtin()` will modify the argv array, so we need to +* create a shallow copy such that we can free all of its +* strings. +*/ +CALLOC_ARRAY (argv_copy, argc +1 ); +COPY_ARRAY (argv_copy, args.v, argc); + + argv = argv_copy; } builtin =get_builtin (cmd); -if (builtin) -exit (run_builtin (builtin, argc, argv)); +if (builtin) { +int ret =run_builtin (builtin, argc, argv); +strvec_clear (&args); +free (argv_copy); +exit (ret); + } +strvec_clear (&args); +free (argv_copy); }static void execv_dashed_external (const char **argv)
t/t0012-help.sh +1 -0
@@ -2,6 +2,7 @@ test_description='help' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh configure_help () {
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 4/23 submodule: fix leaking update strategy
In reply to Patrick SteinhardtWe're not freeing the submodule update strategy command. Provide a helper function that does this for us and call it in update_data_release().
--- builtin/submodule--helper.c | 1 + submodule-config.c | 2 +- submodule.c | 5 +++++ submodule.h | 6 ++++-- t/t7406-submodule-update.sh | 1 + 5 files changed, 12 insertions(+), 3 deletions(-)
builtin/submodule--helper.c +1 -0
@@ -2021,6 +2021,7 @@ struct update_data {static void update_data_release (struct update_data *ud) {free (ud->displaypath); +submodule_update_strategy_release (&ud->update_strategy);module_list_release (&ud->list); }
submodule-config.c +1 -1
@@ -95,7 +95,7 @@ static void free_one_config(struct submodule_entry *entry)free ((void *) entry->config->branch);free ((void *) entry->config->url);free ((void *) entry->config->ignore); -free ((void *) entry->config->update_strategy.command); +submodule_update_strategy_release (&entry->config->update_strategy);free (entry->config); }
submodule.c +5 -0
@@ -424,6 +424,11 @@ int parse_submodule_update_strategy(const char *value,return 0 ; } +void submodule_update_strategy_release (struct submodule_update_strategy *strategy) +{ +free ((char *) strategy->command); +} +const char *submodule_update_type_to_string (enum submodule_update_type type) {switch (type) {
submodule.h +4 -2
@@ -41,6 +41,10 @@ struct submodule_update_strategy { .type = SM_UPDATE_UNSPECIFIED, \ } +int parse_submodule_update_strategy (const char *value, +struct submodule_update_strategy *dst); +void submodule_update_strategy_release (struct submodule_update_strategy *strategy); +int is_gitmodules_unmerged (struct index_state *istate);int is_writing_gitmodules_ok (void );int is_staging_gitmodules_ok (struct index_state *istate);
@@ -70,8 +74,6 @@ void die_in_unpopulated_submodule(struct index_state *istate,void die_path_inside_submodule (struct index_state *istate,const struct pathspec *ps);enum submodule_update_type parse_submodule_update_type (const char *value); -int parse_submodule_update_strategy (const char *value, -struct submodule_update_strategy *dst);const char *submodule_update_type_to_string (enum submodule_update_type type);void handle_ignore_submodules_arg (struct diff_options *,const char *); void show_submodule_diff_summary(struct diff_options *o, const char *path,
t/t7406-submodule-update.sh +1 -0
@@ -12,6 +12,7 @@ submodule and "git submodule update --rebase/--merge" does not detach the HEAD. GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 5/23 builtin/submodule--helper: clear child process when not running it
In reply to Patrick SteinhardtIn runcommand_in_submodule_cb() we may end up not executing the child command when argv is empty. But we still populate the command with environment variables and other things, which needs cleanup. This leads to a memory leak because we do not call finish_command().
Fix this by clearing the child process when we don't execute it.
--- builtin/submodule--helper.c | 10 +++++++--- t/t7407-submodule-foreach.sh | 1 + 2 files changed, 8 insertions(+), 3 deletions(-)
builtin/submodule--helper.c +7 -3
@@ -363,9 +363,13 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,if (!info->quiet)printf (_ ("Entering '%s'\n" ), displaypath); -if (info->argv[0 ] &&run_command (&cp)) -die (_ ("run_command returned non-zero status for %s\n." ), - displaypath); +if (info->argv[0 ]) { +if (run_command (&cp)) +die (_ ("run_command returned non-zero status for %s\n." ), + displaypath); + }else { +child_process_clear (&cp); + }if (info->recursive) {struct child_process cpr =CHILD_PROCESS_INIT ;
t/t7407-submodule-foreach.sh +1 -0
@@ -12,6 +12,7 @@ that are currently checked out. GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 6/23 builtin/submodule--helper: fix leaking error buffer
In reply to Patrick SteinhardtFix leaking error buffer when compute_alternate_path() fails.
--- builtin/submodule--helper.c | 2 ++ t/t7408-submodule-reference.sh | 2 ++ 2 files changed, 4 insertions(+)
builtin/submodule--helper.c +2 -0
@@ -1621,6 +1621,8 @@ static int add_possible_reference_from_superproject( ;/* nothing */ } } + +strbuf_release (&err);strbuf_release (&sb); }
t/t7408-submodule-reference.sh +2 -0
@@ -4,6 +4,8 @@ # test_description='test clone --reference' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh base_dir=$(pwd)
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 7/23 t/helper: fix leaking subrepo in nested submodule config helper
In reply to Patrick SteinhardtIn the "submodule-nested-repo-config" helper we create a submodule repository and print its configuration. We do not clear the repo, causing a memory leak. Plug it.
--- t/helper/test-submodule-nested-repo-config.c | 2 +- t/t7411-submodule-config.sh | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)
t/helper/test-submodule-nested-repo-config.c +1 -1
@@ -29,6 +29,6 @@ int cmd__submodule_nested_repo_config(int argc, const char **argv)print_config_from_gitmodules (&subrepo, argv[2 ]);submodule_free (the_repository); - +repo_clear (&subrepo);return 0 ; }
t/t7411-submodule-config.sh +1 -0
@@ -10,6 +10,7 @@ from the database and from the worktree works. ' TEST_NO_CREATE_REPO=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 8/23 builtin/submodule--helper: fix leaking remote ref on errors
In reply to Patrick SteinhardtWhen update_submodule() fails we return with die_message(). Curiously enough, this causes a memory leak because we use the run_process_parallel() interfaces here, which swap out the die routine.
Fix the leak by freeing the remote ref.
--- builtin/submodule--helper.c | 13 +++++++++---- t/t7420-submodule-set-url.sh | 1 + 2 files changed, 10 insertions(+), 4 deletions(-)
builtin/submodule--helper.c +9 -4
@@ -2648,15 +2648,20 @@ static int update_submodule(struct update_data *update_data)if (!update_data->nofetch) {if (fetch_in_submodule (update_data->sm_path, update_data->depth, -0 ,NULL )) +0 ,NULL )) { +free (remote_ref);return die_message (_ ("Unable to fetch in submodule path '%s'" ), update_data->sm_path); + } }if (repo_resolve_gitlink_ref (the_repository, update_data->sm_path, - remote_ref, &update_data->oid)) -return die_message (_ ("Unable to find %s revision in submodule path '%s'" ), - remote_ref, update_data->sm_path); + remote_ref, &update_data->oid)) { + ret =die_message (_ ("Unable to find %s revision in submodule path '%s'" ), + remote_ref, update_data->sm_path); +free (remote_ref); +return ret; + }free (remote_ref); }
t/t7420-submodule-set-url.sh +1 -0
@@ -10,6 +10,7 @@ as expected. ' TEST_NO_CREATE_REPO=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
Re: [PATCH 08/23] builtin/submodule--helper: fix leaking remote ref on errors
Quoted 4 lines · Patrick Steinhardt
When
update_submodule()fails we return withdie_message(). Curiously enough, this causes a memory leak because we use therun_process_parallel()interfaces here, which swap out the die routine.
Naive question, is update_submodule() itself being run in parallel here? Is that why the die routine gets swapped out so a child process dying is handled differently? Also is it correct to say leaks are not considered when we "die" normally?
Quoted 3 lines
Fix the leak by freeing the remote ref.
Signed-off-by Patrick Steinhardt <[email protected]>
Re: [PATCH 08/23] builtin/submodule--helper: fix leaking remote ref on errors
Quoted 10 lines · Justin Tobler
Quoted 4 lines · Patrick Steinhardt
When
update_submodule()fails we return withdie_message(). Curiously enough, this causes a memory leak because we use therun_process_parallel()interfaces here, which swap out the die routine.Naive question, is
update_submodule()itself being run in parallel here? Is that why the die routine gets swapped out so a child process dying is handled differently? Also is it correct to say leaks are not considered when we "die" normally?
Hm. Revisiting this patch: my analysis was wrong. It's not the parallel subsystem that swaps out die(), but it's the fact that we call die_message(), which actually doesn't die. It really only prints the message you would see when we call die(), nothing more.
I'll amend the commit message and send out the amended version once there is more feedback to address.
Patrick
Re: [PATCH 08/23] builtin/submodule--helper: fix leaking remote ref on errors
Quoted 12 lines · Patrick Steinhardt
Quoted 10 lines · Justin Tobler
Quoted 4 lines · Patrick Steinhardt
When
update_submodule()fails we return withdie_message(). Curiously enough, this causes a memory leak because we use therun_process_parallel()interfaces here, which swap out the die routine.Naive question, is
update_submodule()itself being run in parallel here? Is that why the die routine gets swapped out so a child process dying is handled differently? Also is it correct to say leaks are not considered when we "die" normally?
Quoted 7 lines
Hm. Revisiting this patch: my analysis was wrong. It's not the parallel subsystem that swaps out
die(), but it's the fact that we calldie_message(), which actually doesn't die. It really only prints the message you would see when we calldie(), nothing more.I'll amend the commit message and send out the amended version once there is more feedback to address.
So it has been a week and half since the series was posted and it seems that this is the only thing you might want to touch up.
What's next? Just have an updated patch [08/23] and nothing else and be done with it? A v2 round of 23-patch series hopefully will see somebody other than Justin and I lend an extra set of eyes to double check before we merge it to 'next'?
Thanks.
Re: [PATCH 08/23] builtin/submodule--helper: fix leaking remote ref on errors
Quoted 22 lines · Junio C Hamano
Quoted 12 lines · Patrick Steinhardt
Quoted 10 lines · Justin Tobler
Quoted 4 lines · Patrick Steinhardt
When
update_submodule()fails we return withdie_message(). Curiously enough, this causes a memory leak because we use therun_process_parallel()interfaces here, which swap out the die routine.Naive question, is
update_submodule()itself being run in parallel here? Is that why the die routine gets swapped out so a child process dying is handled differently? Also is it correct to say leaks are not considered when we "die" normally?Quoted 7 lines
Hm. Revisiting this patch: my analysis was wrong. It's not the parallel subsystem that swaps out
die(), but it's the fact that we calldie_message(), which actually doesn't die. It really only prints the message you would see when we calldie(), nothing more.I'll amend the commit message and send out the amended version once there is more feedback to address.
Quoted 7 lines
So it has been a week and half since the series was posted and it seems that this is the only thing you might want to touch up.
What's next? Just have an updated patch [08/23] and nothing else and be done with it? A v2 round of 23-patch series hopefully will see somebody other than Justin and I lend an extra set of eyes to double check before we merge it to 'next'?
Makes sense, let's do it this way! I've sent a v2 a couple minutes ago.
Patrick
PATCH 9/23 dir: fix off by one errors for ignored and untracked entries
In reply to Patrick SteinhardtIn treat_directory() we perform some logic to handle ignored and untracked entries. When populating a directory with entries we first save the current number of ignored/untracked entries and then populate new entries at the end of our arrays that keep track of those entries. When we figure out that all entries have been ignored/are untracked we then remove this tail of entries from those vectors again. But there is an off by one error in both paths that causes us to not free the first ignored and untracked entries, respectively.
Fix these off-by-one errors to plug the resulting leak. While at it, massage the code a bit to match our modern code style.
--- dir.c | 6 ++---- t/t3011-common-prefixes-and-directory-traversal.sh | 1 + t/t7061-wtstatus-ignore.sh | 1 + t/t7521-ignored-mode.sh | 1 + 4 files changed, 5 insertions(+), 4 deletions(-)
dir.c +2 -4
@@ -2135,8 +2135,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,*/ state = path_none; }else { -int i; -for (i = old_ignored_nr +1 ; i<dir->ignored_nr; ++i) +for (int i = old_ignored_nr; i < dir->ignored_nr; i++)FREE_AND_NULL (dir->ignored[i]); dir->ignored_nr = old_ignored_nr; }
@@ -2148,8 +2147,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,*/ if ((dir->flags &DIR_SHOW_IGNORED_TOO ) && !(dir->flags &DIR_KEEP_UNTRACKED_CONTENTS )) { -int i; -for (i = old_untracked_nr +1 ; i<dir->nr; ++i) +for (int i = old_untracked_nr; i < dir->nr; i++)FREE_AND_NULL (dir->entries[i]); dir->nr = old_untracked_nr; }
t/t3011-common-prefixes-and-directory-traversal.sh +1 -0
@@ -2,6 +2,7 @@ test_description='directory traversal handling, especially with common prefixes' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
t/t7061-wtstatus-ignore.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git-status ignored files' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh cat >expected <<\EOF
t/t7521-ignored-mode.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git status ignored modes' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup initial commit and ignore file' '
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
Re: [PATCH 09/23] dir: fix off by one errors for ignored and untracked entries
Quoted 25 lines · Patrick Steinhardt
In
treat_directory()we perform some logic to handle ignored and untracked entries. When populating a directory with entries we first save the current number of ignored/untracked entries and then populate new entries at the end of our arrays that keep track of those entries. When we figure out that all entries have been ignored/are untracked we then remove this tail of entries from those vectors again. But there is an off by one error in both paths that causes us to not free the first ignored and untracked entries, respectively.Fix these off-by-one errors to plug the resulting leak. While at it, massage the code a bit to match our modern code style.
Signed-off-by Patrick Steinhardt <[email protected]>--- dir.c | 6 ++---- t/t3011-common-prefixes-and-directory-traversal.sh | 1 + t/t7061-wtstatus-ignore.sh | 1 + t/t7521-ignored-mode.sh | 1 + 4 files changed, 5 insertions(+), 4 deletions(-)dir.c +0
-0
dir.c static enum path_treatment treat_directory(struct dir_struct *dir, · line 2140
Quoted 8 lines
dir.c +1
-2@@ -2135,8 +2135,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,*/ state = path_none; }else { -int i; -for (i = old_ignored_nr +1 ; i<dir->ignored_nr; ++i) +for (int i = old_ignored_nr; i < dir->ignored_nr; i++)FREE_AND_NULL (dir->ignored[i]); dir->ignored_nr = old_ignored_nr;
So the only effect this bug had was that the entry in the array at the old_ignored_nr did not get freed, even though the code correctly updated that the length of the dir->ignored[] after truncation. I speculated, before looking at the code change but only from what was in the proposed log message, that the resulting array ended up to have an extra entry, or to lose an entry, due to miscounting, but that is not what is going on. If the bug's nature was that we screwed up the length of the updated array, somebody would certainly have noticed a bug in the end-user observable behaviour that a path that should be ignored is not getting ignored or the other way around. Interesting.
I would imagine this is something hard to spot with code inspection alone, yet fairly easy for the leak checker to notice. I am happy to see that it does really help to have good code coverage in the test suite.
Thanks.
PATCH 10/23 builtin/pull: fix leaking "ff" option
In reply to Patrick SteinhardtThe opt_ff field gets populated either via OPT_PASSTHRU via config_get_ff() or when --rebase is passed. So we sometimes end up overriding the value in opt_ff with another value, but we do not free the old value, causing a memory leak.
Adapt the type of the variable to be char * and consistently assign allocated strings to it such that we can easily free it when it is being overridden.
--- builtin/pull.c | 11 +++++++---- t/t7601-merge-pull-config.sh | 1 + 2 files changed, 8 insertions(+), 4 deletions(-)
builtin/pull.c +7 -4
@@ -84,7 +84,7 @@ static const char *opt_squash;static const char *opt_commit;static const char *opt_edit;static const char *cleanup_arg; -static const char *opt_ff; +static char *opt_ff;static const char *opt_verify_signatures;static const char *opt_verify;static int opt_autostash =-1 ;
@@ -1024,8 +1024,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)* "--rebase" can override a config setting of * pull.ff=only. */ -if (opt_rebase >=0 && opt_ff && !strcmp (opt_ff,"--ff-only" )) - opt_ff ="--ff" ; +if (opt_rebase >=0 && opt_ff && !strcmp (opt_ff,"--ff-only" )) { +free (opt_ff); + opt_ff =xstrdup ("--ff" ); + } }if (opt_rebase <0 )
@@ -1135,7 +1137,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)if (can_ff) {/* we can fast-forward this without invoking rebase */ - opt_ff ="--ff-only" ; +free (opt_ff); + opt_ff =xstrdup ("--ff-only" ); ret =run_merge (); }else { ret =run_rebase (&newbase, &upstream);
t/t7601-merge-pull-config.sh +1 -0
@@ -4,6 +4,7 @@ test_description='git merge Testing pull.* configuration parsing and other things.' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
Re: [PATCH 10/23] builtin/pull: fix leaking "ff" option
Quoted 4 lines · Patrick Steinhardt
The
opt_fffield gets populated either viaOPT_PASSTHRUviaconfig_get_ff()or when--rebaseis passed. So we sometimes end up overriding the value inopt_ffwith another value, but we do not free the old value, causing a memory leak.
Good eyes.
I have to wonder if it will come back and bite us again that OPT_PASSTHRU does not pass through but creates a new copy, while OPT_STRING borrows from the argv[]. I guess that we could check in parse_options_check() if the address of the same variable is passed to both OPT_PASSTHRU (giving it a piece of memory we are responsible for freeing) and OPT_STRING (giving it a piece of memory borrowed from argv[], and would trigger a failure if given to free()).
In an extremely longer run, I wonder if it becomes necessary for us to also make OPT_STRING to make a copy (which means strings that come from the configuration and from parsing the command line arguments are all allocated ones that we are responsible to free).
Thanks.
Re: [PATCH 10/23] builtin/pull: fix leaking "ff" option
Quoted 17 lines · Junio C Hamano
Quoted 4 lines · Patrick Steinhardt
The
opt_fffield gets populated either viaOPT_PASSTHRUviaconfig_get_ff()or when--rebaseis passed. So we sometimes end up overriding the value inopt_ffwith another value, but we do not free the old value, causing a memory leak.Good eyes.
I have to wonder if it will come back and bite us again that
OPT_PASSTHRUdoes not pass through but creates a new copy, whileOPT_STRINGborrows from the argv[]. I guess that we could check inparse_options_check()if the address of the same variable is passed to bothOPT_PASSTHRU(giving it a piece of memory we are responsible for freeing) andOPT_STRING(giving it a piece of memory borrowed from argv[], and would trigger a failure if given tofree()).
Quoted 4 lines
In an extremely longer run, I wonder if it becomes necessary for us to also make
OPT_STRINGto make a copy (which means strings that come from the configuration and from parsing the command line arguments are all allocated ones that we are responsible to free).
For now it seems like we are fine with OPT_STRING not copying the value, and I haven't yet found a situation where we cannot work around the alloc/don't alloc split. But it certainly does lead to an awkward calling convention in many places that makes the resulting code way harder to read than necessary if it did allocate.
I also doubt that avoiding an allocation for these gives us a noticeable speedup. After all we're bounded by the number of arguments passed by the user, so it's not like we would end up dupliciating thousands of strings in the general case.
So yes, I think that it would simplify things quite a bit when all three of them (OPT_PASSTHRU, OPT_STRING, config parsing) were duplicating the values.
Patrick
PATCH 11/23 diff: fix leaking orderfile option
In reply to Patrick SteinhardtThe orderfile diff option is being assigned via OPT_FILENAME(), which assigns an allocated string to the variable. We never free it though, causing a memory leak.
Change the type of the string to char * and free it to plug the leak. This also requires us to use xstrdup() to assign the global config to it in case it is set.
This leak is being hit in t7621, but plugging it alone does not make the test suite pass.
--- combine-diff.c | 3 +-- diff.c | 7 +++++-- diff.h | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-)
combine-diff.c +1 -2
@@ -1393,9 +1393,8 @@ static struct combine_diff_path *find_paths_generic(const struct object_id *oid, {struct combine_diff_path *paths =NULL ;int i, num_parent = parents->nr; -int output_format = opt->output_format; -const char *orderfile = opt->orderfile; +char *orderfile = opt->orderfile; opt->output_format =DIFF_FORMAT_NO_OUTPUT ;/* tell diff_tree to emit paths in sorted (=tree) order */
diff.c +5 -2
@@ -441,8 +441,10 @@ int git_diff_ui_config(const char *var, const char *value, }if (!strcmp (var,"diff.wordregex" ))return git_config_string (&diff_word_regex_cfg, var, value); -if (!strcmp (var,"diff.orderfile" )) +if (!strcmp (var,"diff.orderfile" )) { +FREE_AND_NULL (diff_order_file_cfg);return git_config_pathname (&diff_order_file_cfg, var, value); + }if (!strcmp (var,"diff.ignoresubmodules" )) {if (!value)
@@ -4775,7 +4777,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)if (diff_indent_heuristic)DIFF_XDL_SET (options,INDENT_HEURISTIC ); - options->orderfile = diff_order_file_cfg; + options->orderfile =xstrdup_or_null (diff_order_file_cfg);if (!options->flags.ignore_submodule_set) options->flags.ignore_untracked_in_submodules =1 ;
@@ -6727,6 +6729,7 @@ void diff_free(struct diff_options *options)FREE_AND_NULL (options->objfind); } +FREE_AND_NULL (options->orderfile);for (size_t i =0 ; i < options->anchors_nr; i++)free (options->anchors[i]);FREE_AND_NULL (options->anchors);
diff.h +1 -1
@@ -235,7 +235,7 @@ enum diff_submodule_format {* diffcore library with. */ struct diff_options { -const char *orderfile; +char *orderfile;/* * "--rotate-to=<file>" would start showing at <file> and when
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 12/23 parse-options: free previous value of `OPTION_FILENAME`
In reply to Patrick SteinhardtThe OPTION_FILENAME option always assigns either an allocated string or NULL to the value. In case it is passed multiple times it does not know to free the previous value though, which causes a memory leak.
Refactor the function to always free the previous value. None of the sites where this option is used pass a string constant, so this change is safe.
While at it, fix the argument of fix_filename() to be a string constant. The only reason why it's not is because we use it as an in-out-parameter, where the input is a constant and the output is not. This is weird and unnecessary, as we can just return the result instead of using the parameter for this.
This leak is being hit in t7621, but plugging it alone does not make the test suite pass.
--- parse-options.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
parse-options.c +14 -8
@@ -60,12 +60,12 @@ static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,return 0 ; } -static void fix_filename (const char *prefix,char **file) +static char *fix_filename (const char *prefix,const char *file) {if (!file || !*file) - ;/* leave as NULL */ +return NULL ;else - *file =prefix_filename_except_for_dash (prefix, *file); +return prefix_filename_except_for_dash (prefix, file); } static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
@@ -129,18 +129,24 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,return 0 ;case OPTION_FILENAME : + { +const char *value; + +FREE_AND_NULL (*(char **)opt->value); + err =0 ; +if (unset) - *(const char **)opt->value =NULL ; + value =NULL ;else if (opt->flags &PARSE_OPT_OPTARG && !p->opt) - *(const char **)opt->value = (const char *)opt->defval; + value = (const char *) opt->defval;else - err =get_arg (p, opt, flags, (const char **)opt->value); + err =get_arg (p, opt, flags, &value);if (!err) -fix_filename (p->prefix, (char **)opt->value); + *(char **)opt->value =fix_filename (p->prefix, value);return err; - + }case OPTION_CALLBACK : {const char *p_arg =NULL ;
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
Re: [PATCH 12/23] parse-options: free previous value of `OPTION_FILENAME`
Quoted 3 lines · Patrick Steinhardt
The
OPTION_FILENAMEoption always assigns either an allocated string orNULLto the value. In case it is passed multiple times it does not know to free the previous value though, which causes a memory leak.
Our earlier attitude was "the user gave --filename twice from the command line, fully knowing that the last one wins rule will make the earlier one have no effect; not worth worrying about".
But defining the memory ownership rules and make sure the library-ish part of the codebase honors these rules is absolutely the right thing to do.
We saw earlier that OPT_PASSTHRU correctly frees the original value before assigning the new value, and this makes OPT_FILENAME do the same thing. Nice.
PATCH 13/23 diffcore-order: fix leaking buffer when parsing orderfiles
In reply to Patrick SteinhardtIn prepare_order() we parse an orderfile and assign it to a global array. In order to save on some allocations, we replace newlines with NUL characters and then assign pointers into the allocated buffer to that array. This can cause the buffer to be completely unreferenced though in some cases, e.g. because the order file is empty or because we had to use xmemdupz() to copy the lines instead of NUL-terminating them.
Refactor the code to always xmemdupz() the strings. This is a bit simpler, and it is rather unlikely that saving a handful of allocations really matters. This allows us to release the string buffer and thus plug the memory leak.
--- diffcore-order.c | 19 +++++++------------ t/t4056-diff-order.sh | 1 + t/t4204-patch-id.sh | 1 + 3 files changed, 9 insertions(+), 12 deletions(-)
diffcore-order.c +7 -12
@@ -14,8 +14,7 @@ static void prepare_order(const char *orderfile) {int cnt, pass;struct strbuf sb =STRBUF_INIT ; -void *map; -char *cp, *endp; +const char *cp, *endp;ssize_t sz;if (order)
@@ -24,14 +23,13 @@ static void prepare_order(const char *orderfile) sz =strbuf_read_file (&sb, orderfile,0 );if (sz <0 )die_errno (_ ("failed to read orderfile '%s'" ), orderfile); - map =strbuf_detach (&sb,NULL ); - endp = (char *) map + sz; + endp = sb.buf + sz;for (pass =0 ; pass <2 ; pass++) { cnt =0 ; - cp = map; + cp = sb.buf;while (cp < endp) { -char *ep; +const char *ep;for (ep = cp; ep < endp && *ep !='\n' ; ep++) ;/* cp to ep has one line */
@@ -40,12 +38,7 @@ static void prepare_order(const char *orderfile)else if (pass ==0 ) cnt++;else { -if (*ep =='\n' ) { - *ep =0 ; - order[cnt] = cp; - }else { - order[cnt] =xmemdupz (cp, ep - cp); - } + order[cnt] =xmemdupz (cp, ep - cp); cnt++; }if (ep < endp)
@@ -57,6 +50,8 @@ static void prepare_order(const char *orderfile)ALLOC_ARRAY (order, cnt); } } + +strbuf_release (&sb); }static int match_order (const char *path)
t/t4056-diff-order.sh +1 -0
@@ -5,6 +5,7 @@ test_description='diff order & rotate' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh create_files () {
t/t4204-patch-id.sh +1 -0
@@ -5,6 +5,7 @@ test_description='git patch-id' 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' '
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 14/23 builtin/repack: fix leaking configuration
In reply to Patrick SteinhardtWhen repacking, we assemble git-pack-objects(1) arguments both for the "normal" pack and for the cruft pack. This configuration gets populated with a bunch of OPT_PASSTHRU options that we end up passing to the child process. These options are allocated, but never free'd.
Create a new pack_objects_args_release() function that releases the memory for us and call it for both sets of options.
--- builtin/repack.c | 57 ++++++++++++++++++++++++++--------- t/t5329-pack-objects-cruft.sh | 2 ++ t/t7700-repack.sh | 1 + 3 files changed, 45 insertions(+), 15 deletions(-)
builtin/repack.c +42 -15
@@ -85,17 +85,34 @@ static int repack_config(const char *var, const char *value, run_update_server_info =git_config_bool (var, value);return 0 ; } -if (!strcmp (var,"repack.cruftwindow" )) +if (!strcmp (var,"repack.cruftwindow" )) { +free (cruft_po_args->window);return git_config_string (&cruft_po_args->window, var, value); -if (!strcmp (var,"repack.cruftwindowmemory" )) + } +if (!strcmp (var,"repack.cruftwindowmemory" )) { +free (cruft_po_args->window_memory);return git_config_string (&cruft_po_args->window_memory, var, value); -if (!strcmp (var,"repack.cruftdepth" )) + } +if (!strcmp (var,"repack.cruftdepth" )) { +free (cruft_po_args->depth);return git_config_string (&cruft_po_args->depth, var, value); -if (!strcmp (var,"repack.cruftthreads" )) + } +if (!strcmp (var,"repack.cruftthreads" )) { +free (cruft_po_args->threads);return git_config_string (&cruft_po_args->threads, var, value); + }return git_default_config (var, value, ctx, cb); } +static void pack_objects_args_release (struct pack_objects_args *args) +{ +free (args->window); +free (args->window_memory); +free (args->depth); +free (args->threads); +list_objects_filter_release (&args->filter_options); +} +struct existing_packs {struct string_list kept_packs;struct string_list non_kept_packs;
@@ -1152,12 +1169,16 @@ int cmd_repack(int argc, const char **argv, const char *prefix)const char *unpack_unreachable =NULL ;int keep_unreachable =0 ;struct string_list keep_pack_list =STRING_LIST_INIT_NODUP ; -struct pack_objects_args po_args = {NULL }; -struct pack_objects_args cruft_po_args = {NULL }; +struct pack_objects_args po_args = {0 }; +struct pack_objects_args cruft_po_args = {0 };int write_midx =0 ;const char *cruft_expiration =NULL ;const char *expire_to =NULL ;const char *filter_to =NULL ; +const char *opt_window =NULL ; +const char *opt_window_memory =NULL ; +const char *opt_depth =NULL ; +const char *opt_threads =NULL ;struct option builtin_repack_options[] = { OPT_BIT('a', NULL, &pack_everything,
@@ -1191,13 +1212,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix) N_("with -A, do not loosen objects older than this")),OPT_BOOL ('k' ,"keep-unreachable" , &keep_unreachable,N_ ("with -a, repack unreachable objects" )), -OPT_STRING (0 ,"window" , &po_args.window,N_ ("n" ), +OPT_STRING (0 ,"window" , &opt_window,N_ ("n" ),N_ ("size of the window used for delta compression" )), -OPT_STRING (0 ,"window-memory" , &po_args.window_memory,N_ ("bytes" ), +OPT_STRING (0 ,"window-memory" , &opt_window_memory,N_ ("bytes" ),N_ ("same as the above, but limit memory size instead of entries count" )), -OPT_STRING (0 ,"depth" , &po_args.depth,N_ ("n" ), +OPT_STRING (0 ,"depth" , &opt_depth,N_ ("n" ),N_ ("limits the maximum delta depth" )), -OPT_STRING (0 ,"threads" , &po_args.threads,N_ ("n" ), +OPT_STRING (0 ,"threads" , &opt_threads,N_ ("n" ),N_ ("limits the maximum number of threads" )),OPT_MAGNITUDE (0 ,"max-pack-size" , &po_args.max_pack_size, N_("maximum size of each packfile")),
@@ -1224,6 +1245,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix) argc =parse_options (argc, argv, prefix, builtin_repack_options, git_repack_usage,0 ); + po_args.window =xstrdup_or_null (opt_window); + po_args.window_memory =xstrdup_or_null (opt_window_memory); + po_args.depth =xstrdup_or_null (opt_depth); + po_args.threads =xstrdup_or_null (opt_threads); +if (delete_redundant && repository_format_precious_objects)die (_ ("cannot delete packs in a precious-objects repo" ));
@@ -1389,13 +1415,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)const char *pack_prefix =find_pack_prefix (packdir, packtmp);if (!cruft_po_args.window) - cruft_po_args.window = po_args.window; + cruft_po_args.window =xstrdup_or_null (po_args.window);if (!cruft_po_args.window_memory) - cruft_po_args.window_memory = po_args.window_memory; + cruft_po_args.window_memory =xstrdup_or_null (po_args.window_memory);if (!cruft_po_args.depth) - cruft_po_args.depth = po_args.depth; + cruft_po_args.depth =xstrdup_or_null (po_args.depth);if (!cruft_po_args.threads) - cruft_po_args.threads = po_args.threads; + cruft_po_args.threads =xstrdup_or_null (po_args.threads);if (!cruft_po_args.max_pack_size) cruft_po_args.max_pack_size = po_args.max_pack_size;
@@ -1547,7 +1573,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)string_list_clear (&names,1 );existing_packs_release (&existing);free_pack_geometry (&geometry); -list_objects_filter_release (&po_args.filter_options); +pack_objects_args_release (&po_args); +pack_objects_args_release (&cruft_po_args);return ret; }
t/t5329-pack-objects-cruft.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='cruft pack related pack-objects tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh objdir=.git/objects
t/t7700-repack.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git repack works correctly' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "${TEST_DIRECTORY}/lib-bitmap.sh" . "${TEST_DIRECTORY}/lib-midx.sh"
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
Re: [PATCH 14/23] builtin/repack: fix leaking configuration
Quoted 20 lines · Patrick Steinhardt
When repacking, we assemble git-pack-objects(1) arguments both for the "normal" pack and for the cruft pack. This configuration gets populated with a bunch of
OPT_PASSTHRUoptions that we end up passing to the child process. These options are allocated, but never free'd.Create a new
pack_objects_args_release()function that releases the memory for us and call it for both sets of options.Signed-off-by Patrick Steinhardt <[email protected]>--- builtin/repack.c | 57 ++++++++++++++++++++++++++--------- t/t5329-pack-objects-cruft.sh | 2 ++ t/t7700-repack.sh | 1 + 3 files changed, 45 insertions(+), 15 deletions(-)builtin/repack.c +0
-0
builtin/repack.c static int repack_config(const char *var, const char *value, · line 90
Quoted 7 lines
builtin/repack.c +2
-1@@ -85,17 +85,34 @@ static int repack_config(const char *var, const char *value, run_update_server_info =git_config_bool (var, value);return 0 ; } -if (!strcmp (var,"repack.cruftwindow" )) +if (!strcmp (var,"repack.cruftwindow" )) { +free (cruft_po_args->window);return git_config_string (&cruft_po_args->window, var, value);
Surely, as git_config_string() gives an allocated string, the ownership rules should be that the receiving variable/struct member is responsible for properly freeing it, which means that the current value must be discarded before the new value is taken. Looking good.
I wonder if any of these targets (like the cruft_po_args->window we see here) can be updated by parsing the command line arguments (with OPT_STRING)? It turns out that this worry is not unfounded but you already covered it ...
builtin/repack.c int cmd_repack(int argc, const char **argv, const char *prefix) · line 1418
Quoted 3 lines
builtin/repack.c +1
-1if (!cruft_po_args.window) - cruft_po_args.window = po_args.window; + cruft_po_args.window =xstrdup_or_null (po_args.window);
... here, so we are safe.
Looking good.
Thanks.
PATCH 15/23 builtin/difftool: plug several trivial memory leaks
In reply to Patrick SteinhardtThere are several leaking data structures in git-difftool(1). Plug them.
--- builtin/difftool.c | 6 ++++++ t/t7800-difftool.sh | 1 + 2 files changed, 7 insertions(+)
builtin/difftool.c +6 -0
@@ -660,6 +660,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,if (fp)fclose (fp); + hashmap_clear_and_free(&working_tree_dups, struct working_tree_entry, entry); + hashmap_clear_and_free(&wt_modified, struct path_entry, entry); + hashmap_clear_and_free(&tmp_modified, struct path_entry, entry); + hashmap_clear_and_free(&submodules, struct pair_entry, entry); + hashmap_clear_and_free(&symlinks2, struct pair_entry, entry); +release_index (&wtindex);free (lbase_dir);free (rbase_dir);strbuf_release (&info);
t/t7800-difftool.sh +1 -0
@@ -11,6 +11,7 @@ Testing basic diff tool invocation GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh difftool_test_setup ()
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 16/23 trace2: destroy context stored in thread-local storage
In reply to Patrick SteinhardtEach thread may have a specific context in the trace2 subsystem that we set up via thread-local storage. We do not set up a destructor for this data though, which means that the context data will leak.
Plug this leak by installing a destructor. This leak is exposed by t7814, but plugging it alone does not make the whole test suite pass.
--- trace2/tr2_tls.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
trace2/tr2_tls.c +9 -1
@@ -152,11 +152,19 @@ uint64_t tr2tls_absolute_elapsed(uint64_t us)return us - tr2tls_us_start_process; } +static void tr2tls_key_destructor (void *payload) +{ +struct tr2tls_thread_ctx *ctx = payload; +free ((char *)ctx->thread_name); +free (ctx->array_us_start); +free (ctx); +} +void tr2tls_init (void ) {tr2tls_start_process_clock (); -pthread_key_create (&tr2tls_key,NULL ); +pthread_key_create (&tr2tls_key, tr2tls_key_destructor);init_recursive_mutex (&tr2tls_mutex); tr2tls_thread_main =
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 17/23 submodule: fix leaking submodule ODB paths
In reply to Patrick SteinhardtIn add_submodule_odb_by_path() we add a path into a global string list. The list is initialized with NODUP, which means that we do not pass ownership of strings to the list. But we use xstrdup() when we insert a path, with the consequence that the string will never get free'd.
Plug the leak by marking the list as DUP. There is only a single callsite where we insert paths anyway, and as explained above that callsite was mishandling the allocation.
This leak is exposed by t7814, but plugging it does not make the whole test suite pass.
--- submodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
submodule.c +2 -2
@@ -175,11 +175,11 @@ void stage_updated_gitmodules(struct index_state *istate)die (_ ("staging updated .gitmodules failed" )); } -static struct string_list added_submodule_odb_paths =STRING_LIST_INIT_NODUP ; +static struct string_list added_submodule_odb_paths =STRING_LIST_INIT_DUP ;void add_submodule_odb_by_path (const char *path) { -string_list_insert (&added_submodule_odb_paths,xstrdup (path)); +string_list_insert (&added_submodule_odb_paths, path); }int register_all_submodule_odb_as_alternates (void )
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 18/23 grep: fix leaking grep pattern
In reply to Patrick SteinhardtWhen creating a pattern via create_grep_pat() we allocate the pattern member of the structure regardless of the token type. But later, when we try to free the structure, we free the pattern member conditionally on the token type and thus leak memory.
Plug this leak. The leak is exposed by t7814, but plugging it alone does not make the whole test suite pass.
--- grep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
grep.c +1 -1
@@ -843,11 +843,11 @@ static void free_grep_pat(struct grep_pat *pattern)free_pcre2_pattern (p); else regfree(&p->regexp); -free (p->pattern);break ;default :break ; } +free (p->pattern);free (p); } }
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 19/23 promisor-remote: fix leaking partial clone filter
In reply to Patrick SteinhardtThe partial clone filter of a promisor remote is never free'd, causing memory leaks. Furthermore, in case multiple partial clone filters are defined for the same remote, we'd overwrite previous values without freeing them.
Fix these leaks.
--- promisor-remote.c | 2 ++ t/t7814-grep-recurse-submodules.sh | 1 + 2 files changed, 3 insertions(+)
promisor-remote.c +2 -0
@@ -154,6 +154,7 @@ static int promisor_remote_config(const char *var, const char *value,if (!r)return 0 ; +FREE_AND_NULL (r->partial_clone_filter);return git_config_string (&r->partial_clone_filter, var, value); }
@@ -189,6 +190,7 @@ void promisor_remote_clear(struct promisor_remote_config *config) {while (config->promisors) {struct promisor_remote *r = config->promisors; +free (r->partial_clone_filter); config->promisors = config->promisors->next;free (r); }
t/t7814-grep-recurse-submodules.sh +1 -0
@@ -7,6 +7,7 @@ submodules. ' TEST_CREATE_REPO_NO_TEMPLATE=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 20/23 builtin/maintenance: fix leaking config string
In reply to Patrick SteinhardtWhen parsing the maintenance strategy from config we allocate a config string, but do not free it after parsing it. Plug this leak by instead using git_config_get_string_tmp(), which does not allocate any memory.
This leak is exposed by t7900, but plugging it alone does not make the test suite pass.
--- builtin/gc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/gc.c +2 -2
@@ -1476,9 +1476,9 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,static void initialize_maintenance_strategy (void ) { -char *config_str; +const char *config_str; -if (git_config_get_string ("maintenance.strategy" , &config_str)) +if (git_config_get_string_tmp ("maintenance.strategy" , &config_str))return ;if (!strcasecmp (config_str,"incremental" )) {
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
Re: [PATCH 20/23] builtin/maintenance: fix leaking config string
Quoted 17 lines · Patrick Steinhardt
When parsing the maintenance strategy from config we allocate a config string, but do not free it after parsing it. Plug this leak by instead using
git_config_get_string_tmp(), which does not allocate any memory.This leak is exposed by t7900, but plugging it alone does not make the test suite pass.
Signed-off-by Patrick Steinhardt <[email protected]>--- builtin/gc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)builtin/gc.c +0
-0
builtin/gc.c static int maintenance_run_tasks(struct maintenance_run_opts *opts, · line 1482
Quoted 9 lines
builtin/gc.c +2
-2@@ -1476,9 +1476,9 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,static void initialize_maintenance_strategy (void ) { -char *config_str; +const char *config_str; -if (git_config_get_string ("maintenance.strategy" , &config_str)) +if (git_config_get_string_tmp ("maintenance.strategy" , &config_str))return ;
I am not a huge fan of the name of this function, because many callers use the result as a fairly long-lived string (the primary reason why they call them is to avoid having to free() the result at the end), but this new caller's use is very much in line with the "tmp" in its name. We peek into the configset to learn the value of this string, and check its value against "incremental".
Looking good.
Thanks.
PATCH 21/23 builtin/maintenance: fix leak in `get_schedule_cmd()`
In reply to Patrick SteinhardtThe get_schedule_cmd() function allows us to override the schedule command with a specific test command such that we can verify the underlying logic in a platform-independent way. Its memory management is somewhat wild though, because it basically gives up and assigns an allocated string to the string constant output pointer. While this part is marked with UNLEAK() to mask this, we also leak the local string lists.
Rework the function such that it has a separate out parameter. If set, we will assign it the final allocated command. Plug the other memory leaks and create a common exit path.
--- builtin/gc.c | 127 ++++++++++++++++++++++++++--------------- t/t7900-maintenance.sh | 1 + 2 files changed, 81 insertions(+), 47 deletions(-)
builtin/gc.c +80 -47
@@ -1780,32 +1780,33 @@ static const char *get_frequency(enum schedule_priority schedule)* * If $GIT_TEST_MAINT_SCHEDULER is set, return true. * In this case, the *cmd value is read as input. * -* * if the input value *cmd is the key of one of the comma-separated list -* item, then *is_available is set to true and *cmd is modified and becomes +* * if the input value cmd is the key of one of the comma-separated list +* item, then *is_available is set to true and *out is set to * the mock command. * * * if the input value *cmd isn’t the key of any of the comma-separated list -* item, then *is_available is set to false. +* item, then *is_available is set to false and *out is set to the original +* command. * * Ex.: * GIT_TEST_MAINT_SCHEDULER not set * +-------+-------------------------------------------------+ * | Input | Output | -* | *cmd | return code | *cmd | *is_available | +* | *cmd | return code | *out | *is_available | * +-------+-------------+-------------------+---------------+ -* | "foo" | false | "foo" (unchanged) | (unchanged) | +* | "foo" | false | NULL | (unchanged) | * +-------+-------------+-------------------+---------------+ * * GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh” * +-------+-------------------------------------------------+ * | Input | Output | -* | *cmd | return code | *cmd | *is_available | +* | *cmd | return code | *out | *is_available | * +-------+-------------+-------------------+---------------+ * | "foo" | true | "./mock.foo.sh" | true | -* | "qux" | true | "qux" (unchanged) | false | +* | "qux" | true | "qux" (allocated) | false | * +-------+-------------+-------------------+---------------+ */ -static int get_schedule_cmd (const char **cmd,int *is_available) +static int get_schedule_cmd (const char *cmd,int *is_available,char **out) {char *testing =xstrdup_or_null (getenv ("GIT_TEST_MAINT_SCHEDULER" ));struct string_list_item *item;
@@ -1824,16 +1825,22 @@ static int get_schedule_cmd(const char **cmd, int *is_available)if (string_list_split_in_place (&pair, item->string,":" ,2 ) !=2 )continue ; -if (!strcmp (*cmd, pair.items[0 ].string)) { - *cmd = pair.items[1 ].string; +if (!strcmp (cmd, pair.items[0 ].string)) { +if (out) + *out =xstrdup (pair.items[1 ].string);if (is_available) *is_available =1 ; -string_list_clear (&list,0 ); -UNLEAK (testing); -return 1 ; +string_list_clear (&pair,0 ); + goto out; } + +string_list_clear (&pair,0 ); } +if (out) + *out =xstrdup (cmd); + +out:string_list_clear (&list,0 );free (testing);return 1 ;
@@ -1850,9 +1857,8 @@ static int get_random_minute(void)static int is_launchctl_available (void ) { -const char *cmd ="launchctl" ;int is_available; -if (get_schedule_cmd (&cmd, &is_available)) +if (get_schedule_cmd ("launchctl" , &is_available,NULL ))return is_available;#ifdef __APPLE__
@@ -1890,12 +1896,12 @@ static char *launchctl_get_uid(void)static int launchctl_boot_plist (int enable,const char *filename) { -const char *cmd ="launchctl" ; +char *cmd;int result;struct child_process child =CHILD_PROCESS_INIT ;char *uid =launchctl_get_uid (); -get_schedule_cmd (&cmd,NULL ); +get_schedule_cmd ("launchctl" ,NULL , &cmd);strvec_split (&child.args, cmd);strvec_pushl (&child.args, enable ?"bootstrap" :"bootout" , uid, filename,NULL );
@@ -1908,6 +1914,7 @@ static int launchctl_boot_plist(int enable, const char *filename) result =finish_command (&child); +free (cmd);free (uid);return result; }
@@ -1959,10 +1966,10 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_prioritstatic unsigned long lock_file_timeout_ms =ULONG_MAX ;struct strbuf plist =STRBUF_INIT , plist2 =STRBUF_INIT ;struct stat st; -const char *cmd ="launchctl" ; +char *cmd;int minute =get_random_minute (); -get_schedule_cmd (&cmd,NULL ); +get_schedule_cmd ("launchctl" ,NULL , &cmd); preamble ="<?xml version=\"1.0\"?>\n" "<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n" "<plist version=\"1.0\">"
@@ -2052,6 +2059,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_prioritfree (filename);free (name); +free (cmd);strbuf_release (&plist);strbuf_release (&plist2);return 0 ;
@@ -2076,9 +2084,8 @@ static int launchctl_update_schedule(int run_maintenance, int fd UNUSED)static int is_schtasks_available (void ) { -const char *cmd ="schtasks" ;int is_available; -if (get_schedule_cmd (&cmd, &is_available)) +if (get_schedule_cmd ("schtasks" , &is_available,NULL ))return is_available;#ifdef GIT_WINDOWS_NATIVE
@@ -2097,15 +2104,16 @@ static char *schtasks_task_name(const char *frequency)static int schtasks_remove_task (enum schedule_priority schedule) { -const char *cmd ="schtasks" ; +char *cmd;struct child_process child =CHILD_PROCESS_INIT ;const char *frequency =get_frequency (schedule);char *name =schtasks_task_name (frequency); -get_schedule_cmd (&cmd,NULL ); +get_schedule_cmd ("schtasks" ,NULL , &cmd);strvec_split (&child.args, cmd);strvec_pushl (&child.args,"/delete" ,"/tn" , name,"/f" ,NULL );free (name); +free (cmd);return run_command (&child); }
@@ -2119,7 +2127,7 @@ static int schtasks_remove_tasks(void)static int schtasks_schedule_task (const char *exec_path,enum schedule_priority schedule) { -const char *cmd ="schtasks" ; +char *cmd;int result;struct child_process child =CHILD_PROCESS_INIT ;const char *xml;
@@ -2129,7 +2137,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_prioritystruct strbuf tfilename =STRBUF_INIT ;int minute =get_random_minute (); -get_schedule_cmd (&cmd,NULL ); +get_schedule_cmd ("schtasks" ,NULL , &cmd);strbuf_addf (&tfilename,"%s/schedule_%s_XXXXXX" ,get_git_common_dir (), frequency);
@@ -2235,6 +2243,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_prioritydelete_tempfile (&tfile);free (name); +free (cmd);return result; }
@@ -2276,21 +2285,28 @@ static int check_crontab_process(const char *cmd)static int is_crontab_available (void ) { -const char *cmd ="crontab" ; +char *cmd;int is_available; +int ret; -if (get_schedule_cmd (&cmd, &is_available)) -return is_available; +if (get_schedule_cmd ("crontab" , &is_available, &cmd)) { + ret = is_available; + goto out; + }#ifdef __APPLE__/* * macOS has cron, but it requires special permissions and will * create a UI alert when attempting to run this command. */ -return 0 ; + ret =0 ;#else -return check_crontab_process (cmd); + ret =check_crontab_process (cmd);#endif + +out: +free (cmd); +return ret; }#define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE"
@@ -2298,7 +2314,7 @@ static int is_crontab_available(void)static int crontab_update_schedule (int run_maintenance,int fd) { -const char *cmd ="crontab" ; +char *cmd;int result =0 ;int in_old_region =0 ;struct child_process crontab_list =CHILD_PROCESS_INIT ;
@@ -2308,15 +2324,17 @@ static int crontab_update_schedule(int run_maintenance, int fd)struct tempfile *tmpedit =NULL ;int minute =get_random_minute (); -get_schedule_cmd (&cmd,NULL ); +get_schedule_cmd ("crontab" ,NULL , &cmd);strvec_split (&crontab_list.args, cmd);strvec_push (&crontab_list.args,"-l" ); crontab_list.in =-1 ; crontab_list.out =dup (fd); crontab_list.git_cmd =0 ; -if (start_command (&crontab_list)) -return error (_ ("failed to run 'crontab -l'; your system might not support 'cron'" )); +if (start_command (&crontab_list)) { + result =error (_ ("failed to run 'crontab -l'; your system might not support 'cron'" )); + goto out; + }/* Ignore exit code, as an empty crontab will return error. */ finish_command (&crontab_list);
@@ -2386,8 +2404,10 @@ static int crontab_update_schedule(int run_maintenance, int fd) result =error (_ ("'crontab' died" )); elsefclose (cron_list ); + out:delete_tempfile (&tmpedit); +free (cmd);return result; }
@@ -2410,10 +2430,9 @@ static int real_is_systemd_timer_available(void)static int is_systemd_timer_available (void ) { -const char *cmd ="systemctl" ;int is_available; -if (get_schedule_cmd (&cmd, &is_available)) +if (get_schedule_cmd ("systemctl" , &is_available,NULL ))return is_available;return real_is_systemd_timer_available ();
@@ -2594,9 +2613,10 @@ static int systemd_timer_enable_unit(int enable,enum schedule_priority schedule, int minute) { -const char *cmd ="systemctl" ; + char *cmd = NULL;struct child_process child =CHILD_PROCESS_INIT ;const char *frequency =get_frequency (schedule); +int ret;/* * Disabling the systemd unit while it is already disabled makes
@@ -2607,20 +2627,25 @@ static int systemd_timer_enable_unit(int enable,* On the other hand, enabling a systemd unit which is already enabled * produces no error. */ -if (!enable) +if (!enable) { child.no_stderr =1 ; -else if (systemd_timer_write_timer_file (schedule, minute)) -return -1 ; + }else if (systemd_timer_write_timer_file (schedule, minute)) { + ret =-1 ; + goto out; + } -get_schedule_cmd (&cmd,NULL ); +get_schedule_cmd ("systemctl" ,NULL , &cmd);strvec_split (&child.args, cmd);strvec_pushl (&child.args,"--user" , enable ?"enable" :"disable" ,"--now" ,NULL );strvec_pushf (&child.args,SYSTEMD_UNIT_FORMAT , frequency,"timer" ); -if (start_command (&child)) -return error (_ ("failed to start systemctl" )); -if (finish_command (&child)) +if (start_command (&child)) { + ret =error (_ ("failed to start systemctl" )); + goto out; + } + +if (finish_command (&child)) {/* * Disabling an already disabled systemd unit makes * systemctl fail.
@@ -2628,9 +2653,17 @@ static int systemd_timer_enable_unit(int enable,* * Enabling an enabled systemd unit doesn't fail. */ -if (enable) -return error (_ ("failed to run systemctl" )); -return 0 ; +if (enable) { + ret =error (_ ("failed to run systemctl" )); + goto out; + } + } + + ret =0 ; + +out: +free (cmd); +return ret; }/*
t/t7900-maintenance.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git maintenance builtin' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GIT_TEST_COMMIT_GRAPH=0
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
PATCH 22/23 revision: fix leaking parents when simplifying commits
In reply to Patrick SteinhardtWhen simplifying commits, e.g. because they are treesame with their parents, we unset the commit's parent pointers but never free them. Plug the resulting memory leaks.
--- revision.c | 5 +++++ t/t1414-reflog-walk.sh | 1 + t/t5310-pack-bitmaps.sh | 1 + t/t5326-multi-pack-bitmaps.sh | 2 ++ t/t6004-rev-list-path-optim.sh | 1 + t/t6019-rev-list-ancestry-path.sh | 1 + t/t6111-rev-list-treesame.sh | 1 + 7 files changed, 12 insertions(+)
revision.c +5 -0
@@ -1071,7 +1071,11 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit) ts->treesame[nth_parent] =1 ;continue ; } + +free_commit_list (parent->next); parent->next =NULL ; +while (commit->parents != parent) +pop_commit (&commit->parents); commit->parents = parent;/*
@@ -1103,6 +1107,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)die ("cannot simplify commit %s (invalid %s)" ,oid_to_hex (&commit->object.oid),oid_to_hex (&p->object.oid)); +free_commit_list (p->parents); p->parents =NULL ; }/* fallthrough */
t/t1414-reflog-walk.sh +1 -0
@@ -4,6 +4,7 @@ test_description='various tests of reflog walk (log -g) behavior' 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 'set up some reflog entries' '
t/t5310-pack-bitmaps.sh +1 -0
@@ -2,6 +2,7 @@ test_description='exercise basic bitmap functionality' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-bitmap.sh
t/t5326-multi-pack-bitmaps.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='exercise basic multi-pack bitmap functionality' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "${TEST_DIRECTORY}/lib-bitmap.sh"
t/t6004-rev-list-path-optim.sh +1 -0
@@ -16,6 +16,7 @@ test_description='git rev-list trivial path optimization test 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 '
t/t6019-rev-list-ancestry-path.sh +1 -0
@@ -29,6 +29,7 @@ test_description='--ancestry-path' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_merge () {
t/t6111-rev-list-treesame.sh +1 -0
@@ -16,6 +16,7 @@ test_description='TREESAME and limiting' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh note () {
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
Re: [PATCH 22/23] revision: fix leaking parents when simplifying commits
Quoted 20 lines · Patrick Steinhardt
When simplifying commits, e.g. because they are treesame with their parents, we unset the commit's parent pointers but never free them. Plug the resulting memory leaks.
Signed-off-by Patrick Steinhardt <[email protected]>--- revision.c | 5 +++++ t/t1414-reflog-walk.sh | 1 + t/t5310-pack-bitmaps.sh | 1 + t/t5326-multi-pack-bitmaps.sh | 2 ++ t/t6004-rev-list-path-optim.sh | 1 + t/t6019-rev-list-ancestry-path.sh | 1 + t/t6111-rev-list-treesame.sh | 1 + 7 files changed, 12 insertions(+)revision.c +0
-0
revision.c static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit) · line 1076
Quoted 6 lines
revision.c +2
-0@@ -1071,7 +1071,11 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit) ts->treesame[nth_parent] =1 ;continue ; } + +free_commit_list (parent->next); parent->next =NULL ;
We have been walking commit->parents linked list, "parent" is the current parent we are looking at. We are simplifying the history and decided that later parents are not needed, hence we discard the remaining commit_list entries starting from parents->next. But we didn't discard; we just updated parent->next pointer and made the pointed data unreachable, leaking. So we free_commit_list().
revision.c static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit) · line 1079
Quoted 3 lines
revision.c +2
-0+while (commit->parents != parent) +pop_commit (&commit->parents); commit->parents = parent;
And this is the other direction, discarding the parents before the current one we are looking at.
Of course it makes me wonder if we can just free_commit_list() the whole thing and then add the current parent commit (in "p", which was taken with "p = parent->item" before) as the sole parent, with something like
free_commit_list(commit->parents);
commit->parents = NULL;
/* parent = */ commit_list_insert(p, &commit->parents);to replace the 5-line (2 to discard later parents, 3 to discard earlier ones) code, but I do not think it becomes particularly easier to read, so let's drop that.
revision.c static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit) · line 1113
Quoted 8 lines
revision.c +1
-0@@ -1103,6 +1107,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)die ("cannot simplify commit %s (invalid %s)" ,oid_to_hex (&commit->object.oid),oid_to_hex (&p->object.oid)); +free_commit_list (p->parents); p->parents =NULL ; }/* fallthrough */
Looking good.
Thanks.
PATCH 23/23 diffcore-break: fix leaking filespecs when merging broken pairs
In reply to Patrick SteinhardtWhen merging file pairs after they have been broken up we queue a new file pair and discard the broken-up ones. The newly-queued file pair reuses one filespec of the broken up pairs each, where the respective other filespec gets discarded. But we only end up freeing the filespec's data, not the filespec itself, and thus leak memory.
Fix these leaks by using free_filespec() instead.
--- diffcore-break.c | 4 ++-- t/t4008-diff-break-rewrite.sh | 2 ++ t/t4022-diff-rewrite.sh | 1 + t/t4023-diff-rename-typechange.sh | 1 + t/t4031-diff-rewrite-binary.sh | 1 + t/t7524-commit-summary.sh | 2 ++ 6 files changed, 9 insertions(+), 2 deletions(-)
diffcore-break.c +2 -2
@@ -266,8 +266,8 @@ static void merge_broken(struct diff_filepair *p,* in the resulting tree. */ d->one->rename_used++; -diff_free_filespec_data (d->two); -diff_free_filespec_data (c->one); +free_filespec (d->two); +free_filespec (c->one);free (d);free (c); }
t/t4008-diff-break-rewrite.sh +2 -0
@@ -21,6 +21,8 @@ With -B, this should be detected as two complete rewrites. Further, with -B and -M together, these should turn into two renames. ' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh ;# test-lib chdir's into trash
t/t4022-diff-rewrite.sh +1 -0
@@ -2,6 +2,7 @@ test_description='rewrite diff' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff-data.sh
t/t4023-diff-rename-typechange.sh +1 -0
@@ -2,6 +2,7 @@ test_description='typechange rename detection' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh
t/t4031-diff-rewrite-binary.sh +1 -0
@@ -2,6 +2,7 @@ test_description='rewrite diff on binary file' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # We must be large enough to meet the MINIMUM_BREAK_SIZE
t/t7524-commit-summary.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='git commit summary' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty
Quoted 3 lines · Patrick Steinhardt
The patch series is built on top of
ed155187b4(Sync with Git 2.46.1, 2024-09-13) with ps/leakfixes-part-6 at46f6ca2a68(builtin/repack: fix leaking keep-pack list, 2024-09-05) merged into it.
I haven't said this, but I really appreciate contributors (like you) pacing their series submission perfectly with the rate of related topics in flight solidifying. If people hold off sending new and dependent topics for too long, that would waste available review and testing bandwidth, but if people send them while the other topics that they depend on are not yet in a good shape, it would require conflict resolutions that may get wasted when the depended topics have to be updated.
I've been trying to be more explicit in the "What's cooking" report what my evaluation of each topic's "doneness" is, to help contributors who need to send new topics that may conflict with the topics that are already cooking, and I hope it helped in your (and other contributors') pacing their submissions to help the process run more smoothly.
THanks.
Quoted 15 lines · Junio C Hamano
Quoted 3 lines · Patrick Steinhardt
The patch series is built on top of
ed155187b4(Sync with Git 2.46.1, 2024-09-13) with ps/leakfixes-part-6 at46f6ca2a68(builtin/repack: fix leaking keep-pack list, 2024-09-05) merged into it.I haven't said this, but I really appreciate contributors (like you) pacing their series submission perfectly with the rate of related topics in flight solidifying. If people hold off sending new and dependent topics for too long, that would waste available review and testing bandwidth, but if people send them while the other topics that they depend on are not yet in a good shape, it would require conflict resolutions that may get wasted when the depended topics have to be updated.
Quoted 6 lines
I've been trying to be more explicit in the "What's cooking" report what my evaluation of each topic's "doneness" is, to help contributors who need to send new topics that may conflict with the topics that are already cooking, and I hope it helped in your (and other contributors') pacing their submissions to help the process run more smoothly.
This series of patch series was something where I wasn't immediately sure for how well-received it will be. In order to make progress and not be busy with memory leak fixes for the next couple years I had to keep a steady pace that wasn't too slow. But sending out a 20-patch series every other week could feel overwhelming to some folks.
So I'm happy to hear that this is well-received overall. Thanks, and also thanks for your reviews!
Patrick
v2 cover Memory leak fixes (pt.7)
In reply to Patrick SteinhardtHi,
this is the second version of another round of memory leak fixes. There's only a single change compared to v1, namely a revised commit message.
Thanks!
Patrick
Patrick Steinhardt (23):
builtin/help: fix dangling reference to html_path builtin/help: fix leaking html_path when reading config multiple
times
git: fix leaking argv when handling builtins
submodule: fix leaking update strategy builtin/submodule--helper: clear child process when not running it builtin/submodule--helper: fix leaking error buffer t/helper: fix leaking subrepo in nested submodule config helper builtin/submodule--helper: fix leaking remote ref on errors
dir: fix off by one errors for ignored and untracked entries builtin/pull: fix leaking "ff" option
diff: fix leaking orderfile option
parse-options: free previous value of OPTION_FILENAME
diffcore-order: fix leaking buffer when parsing orderfiles builtin/repack: fix leaking configuration builtin/difftool: plug several trivial memory leaks
trace2: destroy context stored in thread-local storage
submodule: fix leaking submodule ODB paths
grep: fix leaking grep pattern
promisor-remote: fix leaking partial clone filter builtin/maintenance: fix leaking config string builtin/maintenance: fix leak in get_schedule_cmd()
revision: fix leaking parents when simplifying commits
diffcore-break: fix leaking filespecs when merging broken pairs
builtin/difftool.c | 6 + builtin/gc.c | 131 +++++++++++------- builtin/help.c | 16 ++- builtin/pull.c | 11 +- builtin/repack.c | 57 ++++++-- builtin/submodule--helper.c | 26 +++- combine-diff.c | 3 +- diff.c | 7 +- diff.h | 2 +- diffcore-break.c | 4 +- diffcore-order.c | 19 +-- dir.c | 6 +- git.c | 22 ++- grep.c | 2 +- parse-options.c | 22 +-- promisor-remote.c | 2 + revision.c | 5 + submodule-config.c | 2 +- submodule.c | 9 +- submodule.h | 6 +- t/helper/test-submodule-nested-repo-config.c | 2 +- t/t0012-help.sh | 1 + t/t1414-reflog-walk.sh | 1 + ...common-prefixes-and-directory-traversal.sh | 1 + t/t4008-diff-break-rewrite.sh | 2 + t/t4022-diff-rewrite.sh | 1 + t/t4023-diff-rename-typechange.sh | 1 + t/t4031-diff-rewrite-binary.sh | 1 + t/t4056-diff-order.sh | 1 + t/t4204-patch-id.sh | 1 + t/t5310-pack-bitmaps.sh | 1 + t/t5326-multi-pack-bitmaps.sh | 2 + t/t5329-pack-objects-cruft.sh | 2 + t/t6004-rev-list-path-optim.sh | 1 + t/t6019-rev-list-ancestry-path.sh | 1 + t/t6111-rev-list-treesame.sh | 1 + t/t7061-wtstatus-ignore.sh | 1 + t/t7406-submodule-update.sh | 1 + t/t7407-submodule-foreach.sh | 1 + t/t7408-submodule-reference.sh | 2 + t/t7411-submodule-config.sh | 1 + t/t7420-submodule-set-url.sh | 1 + t/t7521-ignored-mode.sh | 1 + t/t7524-commit-summary.sh | 2 + t/t7601-merge-pull-config.sh | 1 + t/t7700-repack.sh | 1 + t/t7800-difftool.sh | 1 + t/t7814-grep-recurse-submodules.sh | 1 + t/t7900-maintenance.sh | 1 + trace2/tr2_tls.c | 10 +- 50 files changed, 279 insertions(+), 124 deletions(-)
Range-diff against v1:
1: e3bed973af = 1: e3bed973af builtin/help: fix dangling reference to `html_path` 2: 4a59fe15ae = 2: 4a59fe15ae builtin/help: fix leaking `html_path` when reading config multiple times 3: ea3dd851ad = 3: ea3dd851ad git: fix leaking argv when handling builtins 4: 7cdd2691b7 = 4: 7cdd2691b7 submodule: fix leaking update strategy 5: 0d0964a2be = 5: 0d0964a2be builtin/submodule--helper: clear child process when not running it 6: 52d12e034b = 6: 52d12e034b builtin/submodule--helper: fix leaking error buffer 7: 96bd7f01d5 = 7: 96bd7f01d5 t/helper: fix leaking subrepo in nested submodule config helper 8: d088703d31 ! 8: d5e7a24aac builtin/submodule--helper: fix leaking remote ref on errors
@@ Metadata
## Commit message ##
builtin/submodule--helper: fix leaking remote ref on errors
- When update_submodule() fails we return with die_message().
- Curiously enough, this causes a memory leak because we use the
- run_process_parallel() interfaces here, which swap out the die
- routine.
+ When update_submodule() fails we return with die_message(), which
+ only causes us to print the same message as die() would without
+ actually causing the process to die. We don't free memory in that case
+ and thus leak memory.
Fix the leak by freeing the remote ref.
9: d5c9cccb82 = 9: fca161d389 dir: fix off by one errors for ignored and untracked entries 10: 747c9a76a2 = 10: 2338b5e2a8 builtin/pull: fix leaking "ff" option 11: 85c0f9e5f5 = 11: cb08db4d37 diff: fix leaking orderfile option 12: 330b6c52a0 = 12: 650b89bcca parse-options: free previous value of `OPTION_FILENAME` 13: c975dfe462 = 13: cd79422087 diffcore-order: fix leaking buffer when parsing orderfiles 14: a5f3931eee = 14: e015d1704b builtin/repack: fix leaking configuration 15: c79a5118e4 = 15: 7bb07ec2f0 builtin/difftool: plug several trivial memory leaks 16: 0fb3dc55e5 = 16: 30928eb8f9 trace2: destroy context stored in thread-local storage 17: f1cb8122d1 = 17: 35f5de5467 submodule: fix leaking submodule ODB paths 18: 411df7248d = 18: 3d1cece660 grep: fix leaking grep pattern 19: 690de28bef = 19: da7768bad5 promisor-remote: fix leaking partial clone filter 20: ed4091255c = 20: 0a72fc83f7 builtin/maintenance: fix leaking config string 21: 46956bd8fb = 21: 95200b8a76 builtin/maintenance: fix leak in `get_schedule_cmd()` 22: 2a23df9a68 = 22: 8e7ea54863 revision: fix leaking parents when simplifying commits 23: 57a3a9e9f8 = 23: 8cbc41425f diffcore-break: fix leaking filespecs when merging broken pairs
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 1/23 builtin/help: fix dangling reference to `html_path`
In get_html_page_path() we may end up assigning the return value of system_path() to the global html_path variable. But as we also assign the returned value to to_free, we will deallocate its memory upon returning from the function. Consequently, html_path will now point to deallocated memory.
Fix this issue by instead assigning the value to a separate local variable.
--- builtin/help.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
builtin/help.c +7 -6
@@ -513,23 +513,24 @@ static void show_info_page(const char *page)static void get_html_page_path (struct strbuf *page_path,const char *page) {struct stat st; +const char *path = html_path;char *to_free =NULL ; -if (!html_path) - html_path = to_free =system_path (GIT_HTML_PATH ); +if (!path) + path = to_free =system_path (GIT_HTML_PATH );/* * Check that the page we're looking for exists. */ -if (!strstr (html_path,"://" )) { -if (stat (mkpath ("%s/%s.html" , html_path, page), &st) +if (!strstr (path,"://" )) { +if (stat (mkpath ("%s/%s.html" , path, page), &st) || !S_ISREG (st.st_mode))die ("'%s/%s.html': documentation file not found." , - html_path, page); + path, page); }strbuf_init (page_path,0 ); -strbuf_addf (page_path,"%s/%s.html" , html_path, page); +strbuf_addf (page_path,"%s/%s.html" , path, page);free (to_free); }
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 2/23 builtin/help: fix leaking `html_path` when reading config multiple times
In reply to Patrick SteinhardtThe html_path variable gets populated via git_help_config(), which puts an allocated string into it if its value has been configured. We do not clear the old value though, which causes a memory leak in case the config exists multiple times.
Plug this leak. The leak is exposed by t0012, but plugging it alone is not sufficient to make the test suite pass.
--- builtin/help.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
builtin/help.c +2 -1
@@ -52,7 +52,7 @@ static enum help_action {HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION , } cmd_mode; -static const char *html_path; +static char *html_path;static int verbose =1 ;static enum help_format help_format =HELP_FORMAT_NONE ;static int exclude_guides;
@@ -409,6 +409,7 @@ static int git_help_config(const char *var, const char *value,if (!strcmp (var,"help.htmlpath" )) {if (!value)return config_error_nonbool (var); +free (html_path); html_path =xstrdup (value);return 0 ; }
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 3/23 git: fix leaking argv when handling builtins
In reply to Patrick SteinhardtIn handle_builtin() we may end up creating an ad-hoc argv array in case we see that the command line contains the "--help" parameter. In this case we observe two memory leaks though:
- We leak the struct strvec itself because we directly exit after calling run_builtin(), without bothering about any cleanups.
- Even if we free'd that vector we'd end up leaking some of its strings because run_builtin() will modify the array.
Plug both of these leaks.
--- git.c | 22 +++++++++++++++++++--- t/t0012-help.sh | 1 + 2 files changed, 20 insertions(+), 3 deletions(-)
git.c +19 -3
@@ -711,6 +711,7 @@ static void strip_extension(const char **argv)static void handle_builtin (int argc,const char **argv) {struct strvec args =STRVEC_INIT ; +const char **argv_copy =NULL ;const char *cmd;struct cmd_struct *builtin;
@@ -731,13 +732,28 @@ static void handle_builtin(int argc, const char **argv) } argc++; - argv = args.v; + +/* +* `run_builtin()` will modify the argv array, so we need to +* create a shallow copy such that we can free all of its +* strings. +*/ +CALLOC_ARRAY (argv_copy, argc +1 ); +COPY_ARRAY (argv_copy, args.v, argc); + + argv = argv_copy; } builtin =get_builtin (cmd); -if (builtin) -exit (run_builtin (builtin, argc, argv)); +if (builtin) { +int ret =run_builtin (builtin, argc, argv); +strvec_clear (&args); +free (argv_copy); +exit (ret); + } +strvec_clear (&args); +free (argv_copy); }static void execv_dashed_external (const char **argv)
t/t0012-help.sh +1 -0
@@ -2,6 +2,7 @@ test_description='help' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh configure_help () {
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 4/23 submodule: fix leaking update strategy
In reply to Patrick SteinhardtWe're not freeing the submodule update strategy command. Provide a helper function that does this for us and call it in update_data_release().
--- builtin/submodule--helper.c | 1 + submodule-config.c | 2 +- submodule.c | 5 +++++ submodule.h | 6 ++++-- t/t7406-submodule-update.sh | 1 + 5 files changed, 12 insertions(+), 3 deletions(-)
builtin/submodule--helper.c +1 -0
@@ -2021,6 +2021,7 @@ struct update_data {static void update_data_release (struct update_data *ud) {free (ud->displaypath); +submodule_update_strategy_release (&ud->update_strategy);module_list_release (&ud->list); }
submodule-config.c +1 -1
@@ -95,7 +95,7 @@ static void free_one_config(struct submodule_entry *entry)free ((void *) entry->config->branch);free ((void *) entry->config->url);free ((void *) entry->config->ignore); -free ((void *) entry->config->update_strategy.command); +submodule_update_strategy_release (&entry->config->update_strategy);free (entry->config); }
submodule.c +5 -0
@@ -424,6 +424,11 @@ int parse_submodule_update_strategy(const char *value,return 0 ; } +void submodule_update_strategy_release (struct submodule_update_strategy *strategy) +{ +free ((char *) strategy->command); +} +const char *submodule_update_type_to_string (enum submodule_update_type type) {switch (type) {
submodule.h +4 -2
@@ -41,6 +41,10 @@ struct submodule_update_strategy { .type = SM_UPDATE_UNSPECIFIED, \ } +int parse_submodule_update_strategy (const char *value, +struct submodule_update_strategy *dst); +void submodule_update_strategy_release (struct submodule_update_strategy *strategy); +int is_gitmodules_unmerged (struct index_state *istate);int is_writing_gitmodules_ok (void );int is_staging_gitmodules_ok (struct index_state *istate);
@@ -70,8 +74,6 @@ void die_in_unpopulated_submodule(struct index_state *istate,void die_path_inside_submodule (struct index_state *istate,const struct pathspec *ps);enum submodule_update_type parse_submodule_update_type (const char *value); -int parse_submodule_update_strategy (const char *value, -struct submodule_update_strategy *dst);const char *submodule_update_type_to_string (enum submodule_update_type type);void handle_ignore_submodules_arg (struct diff_options *,const char *); void show_submodule_diff_summary(struct diff_options *o, const char *path,
t/t7406-submodule-update.sh +1 -0
@@ -12,6 +12,7 @@ submodule and "git submodule update --rebase/--merge" does not detach the HEAD. GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 5/23 builtin/submodule--helper: clear child process when not running it
In reply to Patrick SteinhardtIn runcommand_in_submodule_cb() we may end up not executing the child command when argv is empty. But we still populate the command with environment variables and other things, which needs cleanup. This leads to a memory leak because we do not call finish_command().
Fix this by clearing the child process when we don't execute it.
--- builtin/submodule--helper.c | 10 +++++++--- t/t7407-submodule-foreach.sh | 1 + 2 files changed, 8 insertions(+), 3 deletions(-)
builtin/submodule--helper.c +7 -3
@@ -363,9 +363,13 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,if (!info->quiet)printf (_ ("Entering '%s'\n" ), displaypath); -if (info->argv[0 ] &&run_command (&cp)) -die (_ ("run_command returned non-zero status for %s\n." ), - displaypath); +if (info->argv[0 ]) { +if (run_command (&cp)) +die (_ ("run_command returned non-zero status for %s\n." ), + displaypath); + }else { +child_process_clear (&cp); + }if (info->recursive) {struct child_process cpr =CHILD_PROCESS_INIT ;
t/t7407-submodule-foreach.sh +1 -0
@@ -12,6 +12,7 @@ that are currently checked out. GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 6/23 builtin/submodule--helper: fix leaking error buffer
In reply to Patrick SteinhardtFix leaking error buffer when compute_alternate_path() fails.
--- builtin/submodule--helper.c | 2 ++ t/t7408-submodule-reference.sh | 2 ++ 2 files changed, 4 insertions(+)
builtin/submodule--helper.c +2 -0
@@ -1621,6 +1621,8 @@ static int add_possible_reference_from_superproject( ;/* nothing */ } } + +strbuf_release (&err);strbuf_release (&sb); }
t/t7408-submodule-reference.sh +2 -0
@@ -4,6 +4,8 @@ # test_description='test clone --reference' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh base_dir=$(pwd)
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 7/23 t/helper: fix leaking subrepo in nested submodule config helper
In reply to Patrick SteinhardtIn the "submodule-nested-repo-config" helper we create a submodule repository and print its configuration. We do not clear the repo, causing a memory leak. Plug it.
--- t/helper/test-submodule-nested-repo-config.c | 2 +- t/t7411-submodule-config.sh | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)
t/helper/test-submodule-nested-repo-config.c +1 -1
@@ -29,6 +29,6 @@ int cmd__submodule_nested_repo_config(int argc, const char **argv)print_config_from_gitmodules (&subrepo, argv[2 ]);submodule_free (the_repository); - +repo_clear (&subrepo);return 0 ; }
t/t7411-submodule-config.sh +1 -0
@@ -10,6 +10,7 @@ from the database and from the worktree works. ' TEST_NO_CREATE_REPO=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 8/23 builtin/submodule--helper: fix leaking remote ref on errors
In reply to Patrick SteinhardtWhen update_submodule() fails we return with die_message(), which only causes us to print the same message as die() would without actually causing the process to die. We don't free memory in that case and thus leak memory.
Fix the leak by freeing the remote ref.
--- builtin/submodule--helper.c | 13 +++++++++---- t/t7420-submodule-set-url.sh | 1 + 2 files changed, 10 insertions(+), 4 deletions(-)
builtin/submodule--helper.c +9 -4
@@ -2648,15 +2648,20 @@ static int update_submodule(struct update_data *update_data)if (!update_data->nofetch) {if (fetch_in_submodule (update_data->sm_path, update_data->depth, -0 ,NULL )) +0 ,NULL )) { +free (remote_ref);return die_message (_ ("Unable to fetch in submodule path '%s'" ), update_data->sm_path); + } }if (repo_resolve_gitlink_ref (the_repository, update_data->sm_path, - remote_ref, &update_data->oid)) -return die_message (_ ("Unable to find %s revision in submodule path '%s'" ), - remote_ref, update_data->sm_path); + remote_ref, &update_data->oid)) { + ret =die_message (_ ("Unable to find %s revision in submodule path '%s'" ), + remote_ref, update_data->sm_path); +free (remote_ref); +return ret; + }free (remote_ref); }
t/t7420-submodule-set-url.sh +1 -0
@@ -10,6 +10,7 @@ as expected. ' TEST_NO_CREATE_REPO=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 9/23 dir: fix off by one errors for ignored and untracked entries
In reply to Patrick SteinhardtIn treat_directory() we perform some logic to handle ignored and untracked entries. When populating a directory with entries we first save the current number of ignored/untracked entries and then populate new entries at the end of our arrays that keep track of those entries. When we figure out that all entries have been ignored/are untracked we then remove this tail of entries from those vectors again. But there is an off by one error in both paths that causes us to not free the first ignored and untracked entries, respectively.
Fix these off-by-one errors to plug the resulting leak. While at it, massage the code a bit to match our modern code style.
--- dir.c | 6 ++---- t/t3011-common-prefixes-and-directory-traversal.sh | 1 + t/t7061-wtstatus-ignore.sh | 1 + t/t7521-ignored-mode.sh | 1 + 4 files changed, 5 insertions(+), 4 deletions(-)
dir.c +2 -4
@@ -2135,8 +2135,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,*/ state = path_none; }else { -int i; -for (i = old_ignored_nr +1 ; i<dir->ignored_nr; ++i) +for (int i = old_ignored_nr; i < dir->ignored_nr; i++)FREE_AND_NULL (dir->ignored[i]); dir->ignored_nr = old_ignored_nr; }
@@ -2148,8 +2147,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,*/ if ((dir->flags &DIR_SHOW_IGNORED_TOO ) && !(dir->flags &DIR_KEEP_UNTRACKED_CONTENTS )) { -int i; -for (i = old_untracked_nr +1 ; i<dir->nr; ++i) +for (int i = old_untracked_nr; i < dir->nr; i++)FREE_AND_NULL (dir->entries[i]); dir->nr = old_untracked_nr; }
t/t3011-common-prefixes-and-directory-traversal.sh +1 -0
@@ -2,6 +2,7 @@ test_description='directory traversal handling, especially with common prefixes' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
t/t7061-wtstatus-ignore.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git-status ignored files' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh cat >expected <<\EOF
t/t7521-ignored-mode.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git status ignored modes' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup initial commit and ignore file' '
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 10/23 builtin/pull: fix leaking "ff" option
In reply to Patrick SteinhardtThe opt_ff field gets populated either via OPT_PASSTHRU via config_get_ff() or when --rebase is passed. So we sometimes end up overriding the value in opt_ff with another value, but we do not free the old value, causing a memory leak.
Adapt the type of the variable to be char * and consistently assign allocated strings to it such that we can easily free it when it is being overridden.
--- builtin/pull.c | 11 +++++++---- t/t7601-merge-pull-config.sh | 1 + 2 files changed, 8 insertions(+), 4 deletions(-)
builtin/pull.c +7 -4
@@ -84,7 +84,7 @@ static const char *opt_squash;static const char *opt_commit;static const char *opt_edit;static const char *cleanup_arg; -static const char *opt_ff; +static char *opt_ff;static const char *opt_verify_signatures;static const char *opt_verify;static int opt_autostash =-1 ;
@@ -1024,8 +1024,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)* "--rebase" can override a config setting of * pull.ff=only. */ -if (opt_rebase >=0 && opt_ff && !strcmp (opt_ff,"--ff-only" )) - opt_ff ="--ff" ; +if (opt_rebase >=0 && opt_ff && !strcmp (opt_ff,"--ff-only" )) { +free (opt_ff); + opt_ff =xstrdup ("--ff" ); + } }if (opt_rebase <0 )
@@ -1135,7 +1137,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)if (can_ff) {/* we can fast-forward this without invoking rebase */ - opt_ff ="--ff-only" ; +free (opt_ff); + opt_ff =xstrdup ("--ff-only" ); ret =run_merge (); }else { ret =run_rebase (&newbase, &upstream);
t/t7601-merge-pull-config.sh +1 -0
@@ -4,6 +4,7 @@ test_description='git merge Testing pull.* configuration parsing and other things.' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 11/23 diff: fix leaking orderfile option
In reply to Patrick SteinhardtThe orderfile diff option is being assigned via OPT_FILENAME(), which assigns an allocated string to the variable. We never free it though, causing a memory leak.
Change the type of the string to char * and free it to plug the leak. This also requires us to use xstrdup() to assign the global config to it in case it is set.
This leak is being hit in t7621, but plugging it alone does not make the test suite pass.
--- combine-diff.c | 3 +-- diff.c | 7 +++++-- diff.h | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-)
combine-diff.c +1 -2
@@ -1393,9 +1393,8 @@ static struct combine_diff_path *find_paths_generic(const struct object_id *oid, {struct combine_diff_path *paths =NULL ;int i, num_parent = parents->nr; -int output_format = opt->output_format; -const char *orderfile = opt->orderfile; +char *orderfile = opt->orderfile; opt->output_format =DIFF_FORMAT_NO_OUTPUT ;/* tell diff_tree to emit paths in sorted (=tree) order */
diff.c +5 -2
@@ -441,8 +441,10 @@ int git_diff_ui_config(const char *var, const char *value, }if (!strcmp (var,"diff.wordregex" ))return git_config_string (&diff_word_regex_cfg, var, value); -if (!strcmp (var,"diff.orderfile" )) +if (!strcmp (var,"diff.orderfile" )) { +FREE_AND_NULL (diff_order_file_cfg);return git_config_pathname (&diff_order_file_cfg, var, value); + }if (!strcmp (var,"diff.ignoresubmodules" )) {if (!value)
@@ -4775,7 +4777,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)if (diff_indent_heuristic)DIFF_XDL_SET (options,INDENT_HEURISTIC ); - options->orderfile = diff_order_file_cfg; + options->orderfile =xstrdup_or_null (diff_order_file_cfg);if (!options->flags.ignore_submodule_set) options->flags.ignore_untracked_in_submodules =1 ;
@@ -6727,6 +6729,7 @@ void diff_free(struct diff_options *options)FREE_AND_NULL (options->objfind); } +FREE_AND_NULL (options->orderfile);for (size_t i =0 ; i < options->anchors_nr; i++)free (options->anchors[i]);FREE_AND_NULL (options->anchors);
diff.h +1 -1
@@ -235,7 +235,7 @@ enum diff_submodule_format {* diffcore library with. */ struct diff_options { -const char *orderfile; +char *orderfile;/* * "--rotate-to=<file>" would start showing at <file> and when
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 12/23 parse-options: free previous value of `OPTION_FILENAME`
In reply to Patrick SteinhardtThe OPTION_FILENAME option always assigns either an allocated string or NULL to the value. In case it is passed multiple times it does not know to free the previous value though, which causes a memory leak.
Refactor the function to always free the previous value. None of the sites where this option is used pass a string constant, so this change is safe.
While at it, fix the argument of fix_filename() to be a string constant. The only reason why it's not is because we use it as an in-out-parameter, where the input is a constant and the output is not. This is weird and unnecessary, as we can just return the result instead of using the parameter for this.
This leak is being hit in t7621, but plugging it alone does not make the test suite pass.
--- parse-options.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
parse-options.c +14 -8
@@ -60,12 +60,12 @@ static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,return 0 ; } -static void fix_filename (const char *prefix,char **file) +static char *fix_filename (const char *prefix,const char *file) {if (!file || !*file) - ;/* leave as NULL */ +return NULL ;else - *file =prefix_filename_except_for_dash (prefix, *file); +return prefix_filename_except_for_dash (prefix, file); } static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
@@ -129,18 +129,24 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,return 0 ;case OPTION_FILENAME : + { +const char *value; + +FREE_AND_NULL (*(char **)opt->value); + err =0 ; +if (unset) - *(const char **)opt->value =NULL ; + value =NULL ;else if (opt->flags &PARSE_OPT_OPTARG && !p->opt) - *(const char **)opt->value = (const char *)opt->defval; + value = (const char *) opt->defval;else - err =get_arg (p, opt, flags, (const char **)opt->value); + err =get_arg (p, opt, flags, &value);if (!err) -fix_filename (p->prefix, (char **)opt->value); + *(char **)opt->value =fix_filename (p->prefix, value);return err; - + }case OPTION_CALLBACK : {const char *p_arg =NULL ;
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 13/23 diffcore-order: fix leaking buffer when parsing orderfiles
In reply to Patrick SteinhardtIn prepare_order() we parse an orderfile and assign it to a global array. In order to save on some allocations, we replace newlines with NUL characters and then assign pointers into the allocated buffer to that array. This can cause the buffer to be completely unreferenced though in some cases, e.g. because the order file is empty or because we had to use xmemdupz() to copy the lines instead of NUL-terminating them.
Refactor the code to always xmemdupz() the strings. This is a bit simpler, and it is rather unlikely that saving a handful of allocations really matters. This allows us to release the string buffer and thus plug the memory leak.
--- diffcore-order.c | 19 +++++++------------ t/t4056-diff-order.sh | 1 + t/t4204-patch-id.sh | 1 + 3 files changed, 9 insertions(+), 12 deletions(-)
diffcore-order.c +7 -12
@@ -14,8 +14,7 @@ static void prepare_order(const char *orderfile) {int cnt, pass;struct strbuf sb =STRBUF_INIT ; -void *map; -char *cp, *endp; +const char *cp, *endp;ssize_t sz;if (order)
@@ -24,14 +23,13 @@ static void prepare_order(const char *orderfile) sz =strbuf_read_file (&sb, orderfile,0 );if (sz <0 )die_errno (_ ("failed to read orderfile '%s'" ), orderfile); - map =strbuf_detach (&sb,NULL ); - endp = (char *) map + sz; + endp = sb.buf + sz;for (pass =0 ; pass <2 ; pass++) { cnt =0 ; - cp = map; + cp = sb.buf;while (cp < endp) { -char *ep; +const char *ep;for (ep = cp; ep < endp && *ep !='\n' ; ep++) ;/* cp to ep has one line */
@@ -40,12 +38,7 @@ static void prepare_order(const char *orderfile)else if (pass ==0 ) cnt++;else { -if (*ep =='\n' ) { - *ep =0 ; - order[cnt] = cp; - }else { - order[cnt] =xmemdupz (cp, ep - cp); - } + order[cnt] =xmemdupz (cp, ep - cp); cnt++; }if (ep < endp)
@@ -57,6 +50,8 @@ static void prepare_order(const char *orderfile)ALLOC_ARRAY (order, cnt); } } + +strbuf_release (&sb); }static int match_order (const char *path)
t/t4056-diff-order.sh +1 -0
@@ -5,6 +5,7 @@ test_description='diff order & rotate' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh create_files () {
t/t4204-patch-id.sh +1 -0
@@ -5,6 +5,7 @@ test_description='git patch-id' 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' '
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 14/23 builtin/repack: fix leaking configuration
In reply to Patrick SteinhardtWhen repacking, we assemble git-pack-objects(1) arguments both for the "normal" pack and for the cruft pack. This configuration gets populated with a bunch of OPT_PASSTHRU options that we end up passing to the child process. These options are allocated, but never free'd.
Create a new pack_objects_args_release() function that releases the memory for us and call it for both sets of options.
--- builtin/repack.c | 57 ++++++++++++++++++++++++++--------- t/t5329-pack-objects-cruft.sh | 2 ++ t/t7700-repack.sh | 1 + 3 files changed, 45 insertions(+), 15 deletions(-)
builtin/repack.c +42 -15
@@ -85,17 +85,34 @@ static int repack_config(const char *var, const char *value, run_update_server_info =git_config_bool (var, value);return 0 ; } -if (!strcmp (var,"repack.cruftwindow" )) +if (!strcmp (var,"repack.cruftwindow" )) { +free (cruft_po_args->window);return git_config_string (&cruft_po_args->window, var, value); -if (!strcmp (var,"repack.cruftwindowmemory" )) + } +if (!strcmp (var,"repack.cruftwindowmemory" )) { +free (cruft_po_args->window_memory);return git_config_string (&cruft_po_args->window_memory, var, value); -if (!strcmp (var,"repack.cruftdepth" )) + } +if (!strcmp (var,"repack.cruftdepth" )) { +free (cruft_po_args->depth);return git_config_string (&cruft_po_args->depth, var, value); -if (!strcmp (var,"repack.cruftthreads" )) + } +if (!strcmp (var,"repack.cruftthreads" )) { +free (cruft_po_args->threads);return git_config_string (&cruft_po_args->threads, var, value); + }return git_default_config (var, value, ctx, cb); } +static void pack_objects_args_release (struct pack_objects_args *args) +{ +free (args->window); +free (args->window_memory); +free (args->depth); +free (args->threads); +list_objects_filter_release (&args->filter_options); +} +struct existing_packs {struct string_list kept_packs;struct string_list non_kept_packs;
@@ -1152,12 +1169,16 @@ int cmd_repack(int argc, const char **argv, const char *prefix)const char *unpack_unreachable =NULL ;int keep_unreachable =0 ;struct string_list keep_pack_list =STRING_LIST_INIT_NODUP ; -struct pack_objects_args po_args = {NULL }; -struct pack_objects_args cruft_po_args = {NULL }; +struct pack_objects_args po_args = {0 }; +struct pack_objects_args cruft_po_args = {0 };int write_midx =0 ;const char *cruft_expiration =NULL ;const char *expire_to =NULL ;const char *filter_to =NULL ; +const char *opt_window =NULL ; +const char *opt_window_memory =NULL ; +const char *opt_depth =NULL ; +const char *opt_threads =NULL ;struct option builtin_repack_options[] = { OPT_BIT('a', NULL, &pack_everything,
@@ -1191,13 +1212,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix) N_("with -A, do not loosen objects older than this")),OPT_BOOL ('k' ,"keep-unreachable" , &keep_unreachable,N_ ("with -a, repack unreachable objects" )), -OPT_STRING (0 ,"window" , &po_args.window,N_ ("n" ), +OPT_STRING (0 ,"window" , &opt_window,N_ ("n" ),N_ ("size of the window used for delta compression" )), -OPT_STRING (0 ,"window-memory" , &po_args.window_memory,N_ ("bytes" ), +OPT_STRING (0 ,"window-memory" , &opt_window_memory,N_ ("bytes" ),N_ ("same as the above, but limit memory size instead of entries count" )), -OPT_STRING (0 ,"depth" , &po_args.depth,N_ ("n" ), +OPT_STRING (0 ,"depth" , &opt_depth,N_ ("n" ),N_ ("limits the maximum delta depth" )), -OPT_STRING (0 ,"threads" , &po_args.threads,N_ ("n" ), +OPT_STRING (0 ,"threads" , &opt_threads,N_ ("n" ),N_ ("limits the maximum number of threads" )),OPT_MAGNITUDE (0 ,"max-pack-size" , &po_args.max_pack_size, N_("maximum size of each packfile")),
@@ -1224,6 +1245,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix) argc =parse_options (argc, argv, prefix, builtin_repack_options, git_repack_usage,0 ); + po_args.window =xstrdup_or_null (opt_window); + po_args.window_memory =xstrdup_or_null (opt_window_memory); + po_args.depth =xstrdup_or_null (opt_depth); + po_args.threads =xstrdup_or_null (opt_threads); +if (delete_redundant && repository_format_precious_objects)die (_ ("cannot delete packs in a precious-objects repo" ));
@@ -1389,13 +1415,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)const char *pack_prefix =find_pack_prefix (packdir, packtmp);if (!cruft_po_args.window) - cruft_po_args.window = po_args.window; + cruft_po_args.window =xstrdup_or_null (po_args.window);if (!cruft_po_args.window_memory) - cruft_po_args.window_memory = po_args.window_memory; + cruft_po_args.window_memory =xstrdup_or_null (po_args.window_memory);if (!cruft_po_args.depth) - cruft_po_args.depth = po_args.depth; + cruft_po_args.depth =xstrdup_or_null (po_args.depth);if (!cruft_po_args.threads) - cruft_po_args.threads = po_args.threads; + cruft_po_args.threads =xstrdup_or_null (po_args.threads);if (!cruft_po_args.max_pack_size) cruft_po_args.max_pack_size = po_args.max_pack_size;
@@ -1547,7 +1573,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)string_list_clear (&names,1 );existing_packs_release (&existing);free_pack_geometry (&geometry); -list_objects_filter_release (&po_args.filter_options); +pack_objects_args_release (&po_args); +pack_objects_args_release (&cruft_po_args);return ret; }
t/t5329-pack-objects-cruft.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='cruft pack related pack-objects tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh objdir=.git/objects
t/t7700-repack.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git repack works correctly' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "${TEST_DIRECTORY}/lib-bitmap.sh" . "${TEST_DIRECTORY}/lib-midx.sh"
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 15/23 builtin/difftool: plug several trivial memory leaks
In reply to Patrick SteinhardtThere are several leaking data structures in git-difftool(1). Plug them.
--- builtin/difftool.c | 6 ++++++ t/t7800-difftool.sh | 1 + 2 files changed, 7 insertions(+)
builtin/difftool.c +6 -0
@@ -660,6 +660,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,if (fp)fclose (fp); + hashmap_clear_and_free(&working_tree_dups, struct working_tree_entry, entry); + hashmap_clear_and_free(&wt_modified, struct path_entry, entry); + hashmap_clear_and_free(&tmp_modified, struct path_entry, entry); + hashmap_clear_and_free(&submodules, struct pair_entry, entry); + hashmap_clear_and_free(&symlinks2, struct pair_entry, entry); +release_index (&wtindex);free (lbase_dir);free (rbase_dir);strbuf_release (&info);
t/t7800-difftool.sh +1 -0
@@ -11,6 +11,7 @@ Testing basic diff tool invocation GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh difftool_test_setup ()
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 16/23 trace2: destroy context stored in thread-local storage
In reply to Patrick SteinhardtEach thread may have a specific context in the trace2 subsystem that we set up via thread-local storage. We do not set up a destructor for this data though, which means that the context data will leak.
Plug this leak by installing a destructor. This leak is exposed by t7814, but plugging it alone does not make the whole test suite pass.
--- trace2/tr2_tls.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
trace2/tr2_tls.c +9 -1
@@ -152,11 +152,19 @@ uint64_t tr2tls_absolute_elapsed(uint64_t us)return us - tr2tls_us_start_process; } +static void tr2tls_key_destructor (void *payload) +{ +struct tr2tls_thread_ctx *ctx = payload; +free ((char *)ctx->thread_name); +free (ctx->array_us_start); +free (ctx); +} +void tr2tls_init (void ) {tr2tls_start_process_clock (); -pthread_key_create (&tr2tls_key,NULL ); +pthread_key_create (&tr2tls_key, tr2tls_key_destructor);init_recursive_mutex (&tr2tls_mutex); tr2tls_thread_main =
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 17/23 submodule: fix leaking submodule ODB paths
In reply to Patrick SteinhardtIn add_submodule_odb_by_path() we add a path into a global string list. The list is initialized with NODUP, which means that we do not pass ownership of strings to the list. But we use xstrdup() when we insert a path, with the consequence that the string will never get free'd.
Plug the leak by marking the list as DUP. There is only a single callsite where we insert paths anyway, and as explained above that callsite was mishandling the allocation.
This leak is exposed by t7814, but plugging it does not make the whole test suite pass.
--- submodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
submodule.c +2 -2
@@ -175,11 +175,11 @@ void stage_updated_gitmodules(struct index_state *istate)die (_ ("staging updated .gitmodules failed" )); } -static struct string_list added_submodule_odb_paths =STRING_LIST_INIT_NODUP ; +static struct string_list added_submodule_odb_paths =STRING_LIST_INIT_DUP ;void add_submodule_odb_by_path (const char *path) { -string_list_insert (&added_submodule_odb_paths,xstrdup (path)); +string_list_insert (&added_submodule_odb_paths, path); }int register_all_submodule_odb_as_alternates (void )
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 18/23 grep: fix leaking grep pattern
In reply to Patrick SteinhardtWhen creating a pattern via create_grep_pat() we allocate the pattern member of the structure regardless of the token type. But later, when we try to free the structure, we free the pattern member conditionally on the token type and thus leak memory.
Plug this leak. The leak is exposed by t7814, but plugging it alone does not make the whole test suite pass.
--- grep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
grep.c +1 -1
@@ -843,11 +843,11 @@ static void free_grep_pat(struct grep_pat *pattern)free_pcre2_pattern (p); else regfree(&p->regexp); -free (p->pattern);break ;default :break ; } +free (p->pattern);free (p); } }
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 19/23 promisor-remote: fix leaking partial clone filter
In reply to Patrick SteinhardtThe partial clone filter of a promisor remote is never free'd, causing memory leaks. Furthermore, in case multiple partial clone filters are defined for the same remote, we'd overwrite previous values without freeing them.
Fix these leaks.
--- promisor-remote.c | 2 ++ t/t7814-grep-recurse-submodules.sh | 1 + 2 files changed, 3 insertions(+)
promisor-remote.c +2 -0
@@ -154,6 +154,7 @@ static int promisor_remote_config(const char *var, const char *value,if (!r)return 0 ; +FREE_AND_NULL (r->partial_clone_filter);return git_config_string (&r->partial_clone_filter, var, value); }
@@ -189,6 +190,7 @@ void promisor_remote_clear(struct promisor_remote_config *config) {while (config->promisors) {struct promisor_remote *r = config->promisors; +free (r->partial_clone_filter); config->promisors = config->promisors->next;free (r); }
t/t7814-grep-recurse-submodules.sh +1 -0
@@ -7,6 +7,7 @@ submodules. ' TEST_CREATE_REPO_NO_TEMPLATE=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 20/23 builtin/maintenance: fix leaking config string
In reply to Patrick SteinhardtWhen parsing the maintenance strategy from config we allocate a config string, but do not free it after parsing it. Plug this leak by instead using git_config_get_string_tmp(), which does not allocate any memory.
This leak is exposed by t7900, but plugging it alone does not make the test suite pass.
--- builtin/gc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/gc.c +2 -2
@@ -1476,9 +1476,9 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,static void initialize_maintenance_strategy (void ) { -char *config_str; +const char *config_str; -if (git_config_get_string ("maintenance.strategy" , &config_str)) +if (git_config_get_string_tmp ("maintenance.strategy" , &config_str))return ;if (!strcasecmp (config_str,"incremental" )) {
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 21/23 builtin/maintenance: fix leak in `get_schedule_cmd()`
In reply to Patrick SteinhardtThe get_schedule_cmd() function allows us to override the schedule command with a specific test command such that we can verify the underlying logic in a platform-independent way. Its memory management is somewhat wild though, because it basically gives up and assigns an allocated string to the string constant output pointer. While this part is marked with UNLEAK() to mask this, we also leak the local string lists.
Rework the function such that it has a separate out parameter. If set, we will assign it the final allocated command. Plug the other memory leaks and create a common exit path.
--- builtin/gc.c | 127 ++++++++++++++++++++++++++--------------- t/t7900-maintenance.sh | 1 + 2 files changed, 81 insertions(+), 47 deletions(-)
builtin/gc.c +80 -47
@@ -1780,32 +1780,33 @@ static const char *get_frequency(enum schedule_priority schedule)* * If $GIT_TEST_MAINT_SCHEDULER is set, return true. * In this case, the *cmd value is read as input. * -* * if the input value *cmd is the key of one of the comma-separated list -* item, then *is_available is set to true and *cmd is modified and becomes +* * if the input value cmd is the key of one of the comma-separated list +* item, then *is_available is set to true and *out is set to * the mock command. * * * if the input value *cmd isn’t the key of any of the comma-separated list -* item, then *is_available is set to false. +* item, then *is_available is set to false and *out is set to the original +* command. * * Ex.: * GIT_TEST_MAINT_SCHEDULER not set * +-------+-------------------------------------------------+ * | Input | Output | -* | *cmd | return code | *cmd | *is_available | +* | *cmd | return code | *out | *is_available | * +-------+-------------+-------------------+---------------+ -* | "foo" | false | "foo" (unchanged) | (unchanged) | +* | "foo" | false | NULL | (unchanged) | * +-------+-------------+-------------------+---------------+ * * GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh” * +-------+-------------------------------------------------+ * | Input | Output | -* | *cmd | return code | *cmd | *is_available | +* | *cmd | return code | *out | *is_available | * +-------+-------------+-------------------+---------------+ * | "foo" | true | "./mock.foo.sh" | true | -* | "qux" | true | "qux" (unchanged) | false | +* | "qux" | true | "qux" (allocated) | false | * +-------+-------------+-------------------+---------------+ */ -static int get_schedule_cmd (const char **cmd,int *is_available) +static int get_schedule_cmd (const char *cmd,int *is_available,char **out) {char *testing =xstrdup_or_null (getenv ("GIT_TEST_MAINT_SCHEDULER" ));struct string_list_item *item;
@@ -1824,16 +1825,22 @@ static int get_schedule_cmd(const char **cmd, int *is_available)if (string_list_split_in_place (&pair, item->string,":" ,2 ) !=2 )continue ; -if (!strcmp (*cmd, pair.items[0 ].string)) { - *cmd = pair.items[1 ].string; +if (!strcmp (cmd, pair.items[0 ].string)) { +if (out) + *out =xstrdup (pair.items[1 ].string);if (is_available) *is_available =1 ; -string_list_clear (&list,0 ); -UNLEAK (testing); -return 1 ; +string_list_clear (&pair,0 ); + goto out; } + +string_list_clear (&pair,0 ); } +if (out) + *out =xstrdup (cmd); + +out:string_list_clear (&list,0 );free (testing);return 1 ;
@@ -1850,9 +1857,8 @@ static int get_random_minute(void)static int is_launchctl_available (void ) { -const char *cmd ="launchctl" ;int is_available; -if (get_schedule_cmd (&cmd, &is_available)) +if (get_schedule_cmd ("launchctl" , &is_available,NULL ))return is_available;#ifdef __APPLE__
@@ -1890,12 +1896,12 @@ static char *launchctl_get_uid(void)static int launchctl_boot_plist (int enable,const char *filename) { -const char *cmd ="launchctl" ; +char *cmd;int result;struct child_process child =CHILD_PROCESS_INIT ;char *uid =launchctl_get_uid (); -get_schedule_cmd (&cmd,NULL ); +get_schedule_cmd ("launchctl" ,NULL , &cmd);strvec_split (&child.args, cmd);strvec_pushl (&child.args, enable ?"bootstrap" :"bootout" , uid, filename,NULL );
@@ -1908,6 +1914,7 @@ static int launchctl_boot_plist(int enable, const char *filename) result =finish_command (&child); +free (cmd);free (uid);return result; }
@@ -1959,10 +1966,10 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_prioritstatic unsigned long lock_file_timeout_ms =ULONG_MAX ;struct strbuf plist =STRBUF_INIT , plist2 =STRBUF_INIT ;struct stat st; -const char *cmd ="launchctl" ; +char *cmd;int minute =get_random_minute (); -get_schedule_cmd (&cmd,NULL ); +get_schedule_cmd ("launchctl" ,NULL , &cmd); preamble ="<?xml version=\"1.0\"?>\n" "<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n" "<plist version=\"1.0\">"
@@ -2052,6 +2059,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_prioritfree (filename);free (name); +free (cmd);strbuf_release (&plist);strbuf_release (&plist2);return 0 ;
@@ -2076,9 +2084,8 @@ static int launchctl_update_schedule(int run_maintenance, int fd UNUSED)static int is_schtasks_available (void ) { -const char *cmd ="schtasks" ;int is_available; -if (get_schedule_cmd (&cmd, &is_available)) +if (get_schedule_cmd ("schtasks" , &is_available,NULL ))return is_available;#ifdef GIT_WINDOWS_NATIVE
@@ -2097,15 +2104,16 @@ static char *schtasks_task_name(const char *frequency)static int schtasks_remove_task (enum schedule_priority schedule) { -const char *cmd ="schtasks" ; +char *cmd;struct child_process child =CHILD_PROCESS_INIT ;const char *frequency =get_frequency (schedule);char *name =schtasks_task_name (frequency); -get_schedule_cmd (&cmd,NULL ); +get_schedule_cmd ("schtasks" ,NULL , &cmd);strvec_split (&child.args, cmd);strvec_pushl (&child.args,"/delete" ,"/tn" , name,"/f" ,NULL );free (name); +free (cmd);return run_command (&child); }
@@ -2119,7 +2127,7 @@ static int schtasks_remove_tasks(void)static int schtasks_schedule_task (const char *exec_path,enum schedule_priority schedule) { -const char *cmd ="schtasks" ; +char *cmd;int result;struct child_process child =CHILD_PROCESS_INIT ;const char *xml;
@@ -2129,7 +2137,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_prioritystruct strbuf tfilename =STRBUF_INIT ;int minute =get_random_minute (); -get_schedule_cmd (&cmd,NULL ); +get_schedule_cmd ("schtasks" ,NULL , &cmd);strbuf_addf (&tfilename,"%s/schedule_%s_XXXXXX" ,get_git_common_dir (), frequency);
@@ -2235,6 +2243,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_prioritydelete_tempfile (&tfile);free (name); +free (cmd);return result; }
@@ -2276,21 +2285,28 @@ static int check_crontab_process(const char *cmd)static int is_crontab_available (void ) { -const char *cmd ="crontab" ; +char *cmd;int is_available; +int ret; -if (get_schedule_cmd (&cmd, &is_available)) -return is_available; +if (get_schedule_cmd ("crontab" , &is_available, &cmd)) { + ret = is_available; + goto out; + }#ifdef __APPLE__/* * macOS has cron, but it requires special permissions and will * create a UI alert when attempting to run this command. */ -return 0 ; + ret =0 ;#else -return check_crontab_process (cmd); + ret =check_crontab_process (cmd);#endif + +out: +free (cmd); +return ret; }#define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE"
@@ -2298,7 +2314,7 @@ static int is_crontab_available(void)static int crontab_update_schedule (int run_maintenance,int fd) { -const char *cmd ="crontab" ; +char *cmd;int result =0 ;int in_old_region =0 ;struct child_process crontab_list =CHILD_PROCESS_INIT ;
@@ -2308,15 +2324,17 @@ static int crontab_update_schedule(int run_maintenance, int fd)struct tempfile *tmpedit =NULL ;int minute =get_random_minute (); -get_schedule_cmd (&cmd,NULL ); +get_schedule_cmd ("crontab" ,NULL , &cmd);strvec_split (&crontab_list.args, cmd);strvec_push (&crontab_list.args,"-l" ); crontab_list.in =-1 ; crontab_list.out =dup (fd); crontab_list.git_cmd =0 ; -if (start_command (&crontab_list)) -return error (_ ("failed to run 'crontab -l'; your system might not support 'cron'" )); +if (start_command (&crontab_list)) { + result =error (_ ("failed to run 'crontab -l'; your system might not support 'cron'" )); + goto out; + }/* Ignore exit code, as an empty crontab will return error. */ finish_command (&crontab_list);
@@ -2386,8 +2404,10 @@ static int crontab_update_schedule(int run_maintenance, int fd) result =error (_ ("'crontab' died" )); elsefclose (cron_list ); + out:delete_tempfile (&tmpedit); +free (cmd);return result; }
@@ -2410,10 +2430,9 @@ static int real_is_systemd_timer_available(void)static int is_systemd_timer_available (void ) { -const char *cmd ="systemctl" ;int is_available; -if (get_schedule_cmd (&cmd, &is_available)) +if (get_schedule_cmd ("systemctl" , &is_available,NULL ))return is_available;return real_is_systemd_timer_available ();
@@ -2594,9 +2613,10 @@ static int systemd_timer_enable_unit(int enable,enum schedule_priority schedule, int minute) { -const char *cmd ="systemctl" ; + char *cmd = NULL;struct child_process child =CHILD_PROCESS_INIT ;const char *frequency =get_frequency (schedule); +int ret;/* * Disabling the systemd unit while it is already disabled makes
@@ -2607,20 +2627,25 @@ static int systemd_timer_enable_unit(int enable,* On the other hand, enabling a systemd unit which is already enabled * produces no error. */ -if (!enable) +if (!enable) { child.no_stderr =1 ; -else if (systemd_timer_write_timer_file (schedule, minute)) -return -1 ; + }else if (systemd_timer_write_timer_file (schedule, minute)) { + ret =-1 ; + goto out; + } -get_schedule_cmd (&cmd,NULL ); +get_schedule_cmd ("systemctl" ,NULL , &cmd);strvec_split (&child.args, cmd);strvec_pushl (&child.args,"--user" , enable ?"enable" :"disable" ,"--now" ,NULL );strvec_pushf (&child.args,SYSTEMD_UNIT_FORMAT , frequency,"timer" ); -if (start_command (&child)) -return error (_ ("failed to start systemctl" )); -if (finish_command (&child)) +if (start_command (&child)) { + ret =error (_ ("failed to start systemctl" )); + goto out; + } + +if (finish_command (&child)) {/* * Disabling an already disabled systemd unit makes * systemctl fail.
@@ -2628,9 +2653,17 @@ static int systemd_timer_enable_unit(int enable,* * Enabling an enabled systemd unit doesn't fail. */ -if (enable) -return error (_ ("failed to run systemctl" )); -return 0 ; +if (enable) { + ret =error (_ ("failed to run systemctl" )); + goto out; + } + } + + ret =0 ; + +out: +free (cmd); +return ret; }/*
t/t7900-maintenance.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git maintenance builtin' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GIT_TEST_COMMIT_GRAPH=0
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 22/23 revision: fix leaking parents when simplifying commits
In reply to Patrick SteinhardtWhen simplifying commits, e.g. because they are treesame with their parents, we unset the commit's parent pointers but never free them. Plug the resulting memory leaks.
--- revision.c | 5 +++++ t/t1414-reflog-walk.sh | 1 + t/t5310-pack-bitmaps.sh | 1 + t/t5326-multi-pack-bitmaps.sh | 2 ++ t/t6004-rev-list-path-optim.sh | 1 + t/t6019-rev-list-ancestry-path.sh | 1 + t/t6111-rev-list-treesame.sh | 1 + 7 files changed, 12 insertions(+)
revision.c +5 -0
@@ -1071,7 +1071,11 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit) ts->treesame[nth_parent] =1 ;continue ; } + +free_commit_list (parent->next); parent->next =NULL ; +while (commit->parents != parent) +pop_commit (&commit->parents); commit->parents = parent;/*
@@ -1103,6 +1107,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)die ("cannot simplify commit %s (invalid %s)" ,oid_to_hex (&commit->object.oid),oid_to_hex (&p->object.oid)); +free_commit_list (p->parents); p->parents =NULL ; }/* fallthrough */
t/t1414-reflog-walk.sh +1 -0
@@ -4,6 +4,7 @@ test_description='various tests of reflog walk (log -g) behavior' 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 'set up some reflog entries' '
t/t5310-pack-bitmaps.sh +1 -0
@@ -2,6 +2,7 @@ test_description='exercise basic bitmap functionality' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-bitmap.sh
t/t5326-multi-pack-bitmaps.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='exercise basic multi-pack bitmap functionality' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "${TEST_DIRECTORY}/lib-bitmap.sh"
t/t6004-rev-list-path-optim.sh +1 -0
@@ -16,6 +16,7 @@ test_description='git rev-list trivial path optimization test 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 '
t/t6019-rev-list-ancestry-path.sh +1 -0
@@ -29,6 +29,7 @@ test_description='--ancestry-path' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_merge () {
t/t6111-rev-list-treesame.sh +1 -0
@@ -16,6 +16,7 @@ test_description='TREESAME and limiting' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh note () {
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty
PATCH v2 23/23 diffcore-break: fix leaking filespecs when merging broken pairs
In reply to Patrick SteinhardtWhen merging file pairs after they have been broken up we queue a new file pair and discard the broken-up ones. The newly-queued file pair reuses one filespec of the broken up pairs each, where the respective other filespec gets discarded. But we only end up freeing the filespec's data, not the filespec itself, and thus leak memory.
Fix these leaks by using free_filespec() instead.
--- diffcore-break.c | 4 ++-- t/t4008-diff-break-rewrite.sh | 2 ++ t/t4022-diff-rewrite.sh | 1 + t/t4023-diff-rename-typechange.sh | 1 + t/t4031-diff-rewrite-binary.sh | 1 + t/t7524-commit-summary.sh | 2 ++ 6 files changed, 9 insertions(+), 2 deletions(-)
diffcore-break.c +2 -2
@@ -266,8 +266,8 @@ static void merge_broken(struct diff_filepair *p,* in the resulting tree. */ d->one->rename_used++; -diff_free_filespec_data (d->two); -diff_free_filespec_data (c->one); +free_filespec (d->two); +free_filespec (c->one);free (d);free (c); }
t/t4008-diff-break-rewrite.sh +2 -0
@@ -21,6 +21,8 @@ With -B, this should be detected as two complete rewrites. Further, with -B and -M together, these should turn into two renames. ' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh ;# test-lib chdir's into trash
t/t4022-diff-rewrite.sh +1 -0
@@ -2,6 +2,7 @@ test_description='rewrite diff' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff-data.sh
t/t4023-diff-rename-typechange.sh +1 -0
@@ -2,6 +2,7 @@ test_description='typechange rename detection' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh
t/t4031-diff-rewrite-binary.sh +1 -0
@@ -2,6 +2,7 @@ test_description='rewrite diff on binary file' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # We must be large enough to meet the MINIMUM_BREAK_SIZE
t/t7524-commit-summary.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='git commit summary' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.2.852.g229c0bf0e5.dirty