From: Jacob Keller <jacob.keller@xxxxxxxxx>
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@xxxxxxxxx>
---
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
