[PATCH v2 1/6] remote: fix tear down of struct branch and struct remote
- From
Jacob Keller <jacob.e.keller@intel.com>
- Date
- Jun 17, 2025, 21:30 UTC
- Message-ID
- <20250617-jk-submodule-helper-use-url-v2-1-04cbb003177d@gmail.com>
- In-Reply-To
- <20250617-jk-submodule-helper-use-url-v2-0-04cbb003177d@gmail.com>
From: Jacob Keller <jacob.keller@gmail.com>
The branch_release() and remote_clear() functions fail to completely release all of the memory they point to. This results in leaked memory if you read the configuration for an arbitrary repository and then release that repository.
This should be caught by the leak sanitizer. However, for callers which use ``the_repository``, the values never go out of scope and the sanitizer won't complain.
A future change is going to add a caller of read_config() for a submodule repository structure. Doing so reveals one immediate issue due to a bad NULL pointer access, as well as the mentioned leaks.
* The branch->merge array is accessed without checking if its NULL. Since this array is only setup by calling set_merge, it may in fact not be initialized even though merge_nr is non-zero.
* The remote->push and remote->fetch refspecs are never cleared.
* The branch->merge_name array is never cleared.
* The individual elements of branch->merge are not released.
Add a check against branch->merge before accessing it and calling refspec_item_clear. Update remote_clear() with calls to refspec_clear() for both the push and fetch refspecs. Add a release of the merge_name items as well as a final release of the merge_name array.
Freeing merge_name elements results in a warning because we discard the const qualifier on the parameter name. These values come from a call to add_merge() in handle_config(), which always copies the names with xstrdup. This makes ownership of the memory difficult to track in the code.
Move the call to xstrdup inside add_merge() so that its clear that the memory is duplicated here and must be released when the merge_name array is released. Drop the const qualifier on the branch structure to allow calling free without an explicit cast.
These changes make it safe to call read_config() on a submodule repository without crashing or leaking any of the memory when the submodule repository is released.
There is still some confusion with the difference between branch->merge and branch->merge_name, and the confusion of using branch->merge_nr for both. That could probably use some future cleanup.
Signed-off-by: Jacob Keller <jacob.keller@gmail.com> --- remote.h | 2 +- remote.c | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/remote.h b/remote.h index 7e4943ae3a70ecefa3332d211084762ca30b59b6..0fca14348fd3d0734c599b13278afef2e00107b9 100644 --- a/remote.h +++ b/remote.h @@ -316,7 +316,7 @@ struct branch { char *pushremote_name; /* An array of the "merge" lines in the configuration. */ - const char **merge_name; + char **merge_name; /** * An array of the struct refspecs used for the merge lines. That is, diff --git a/remote.c b/remote.c index 4099183cacdc8a607a8b5eaec86e456b2ef46b48..538d0d24c832ffeb1cc14684eb166c62c42d719d 100644 --- a/remote.c +++ b/remote.c @@ -165,6 +165,9 @@ static void remote_clear(struct remote *remote) strvec_clear(&remote->url); strvec_clear(&remote->pushurl); + refspec_clear(&remote->push); + refspec_clear(&remote->fetch); + free((char *)remote->receivepack); free((char *)remote->uploadpack); FREE_AND_NULL(remote->http_proxy); @@ -176,7 +179,7 @@ static void add_merge(struct branch *branch, const char *name) { ALLOC_GROW(branch->merge_name, branch->merge_nr + 1, branch->merge_alloc); - branch->merge_name[branch->merge_nr++] = name; + branch->merge_name[branch->merge_nr++] = xstrdup(name); } struct branches_hash_key { @@ -253,9 +256,16 @@ static void branch_release(struct branch *branch) free((char *)branch->refname); free(branch->remote_name); free(branch->pushremote_name); - for (int i = 0; i < branch->merge_nr; i++) - refspec_item_clear(branch->merge[i]); + for (int i = 0; i < branch->merge_nr; i++) { + if (branch->merge) { + refspec_item_clear(branch->merge[i]); + free(branch->merge[i]); + } + if (branch->merge_name) + free(branch->merge_name[i]); + } free(branch->merge); + free(branch->merge_name); } static struct rewrite *make_rewrite(struct rewrites *r, @@ -429,7 +439,7 @@ static int handle_config(const char *key, const char *value, } else if (!strcmp(subkey, "merge")) { if (!value) return config_error_nonbool(key); - add_merge(branch, xstrdup(value)); + add_merge(branch, value); } return 0; }
-- 2.48.1.397.gec9d649cc640