aboutsummaryrefslogtreecommitdiffstats
path: root/remote.c
diff options
context:
space:
mode:
authorJacob Keller <jacob.keller@gmail.com>2025-06-23 16:11:29 -0700
committerJunio C Hamano <gitster@pobox.com>2025-06-23 16:38:55 -0700
commitf62dcc7f30d16af29c0f707005aceb5eb6119279 (patch)
tree7251fb69e14e76d62832974534637b804485847f /remote.c
parent16bd9f20a403117f2e0d9bcda6c6e621d3763e77 (diff)
downloadgit-f62dcc7f30d16af29c0f707005aceb5eb6119279.tar.gz
remote: remove branch->merge_name and fix branch_release()
The branch structure has both branch->merge_name and branch->merge for tracking the merge information. The former is allocated by add_merge() and stores the names read from the configuration file. The latter is allocated by set_merge() which is called by branch_get() when an external caller requests a branch. This leads to the confusing situation where branch->merge_nr tracks both the size of branch->merge (once its allocated) and branch->merge_name. The branch_release() function incorrectly assumes that branch->merge is always set when branch->merge_nr is non-zero, and can potentially crash if read_config() is called without branch_get() being called on every branch. In addition, branch_release() fails to free some of the memory associated with the structure including: * Failure to free the refspec_item containers in branch->merge[i] * Failure to free the strings in branch->merge_name[i] * Failure to free the branch->merge_name parent array. The set_merge() function sets branch->merge_nr to 0 when there is no valid remote_name, to avoid external callers seeing a non-zero merge_nr but a NULL merge array. This results in failure to release most of the merge data as well. These issues could be fixed directly, and indeed I initially proposed such a change at [1] in the past. While this works, there was some confusion during review because of the inconsistencies. Instead, its time to clean up the situation properly. Remove branch->merge_name entirely. Instead, allocate branch->merge earlier within add_merge() instead of within set_merge(). Instead of having set_merge() copy from merge_name[i] to merge[i]->src, just have add_merge() directly initialize merge[i]->src. Modify the add_merge() to call xstrdup() itself, instead of having the caller of add_merge() do so. This makes it more obvious which code owns the memory. Update all callers which use branch->merge_name[i] to use branch->merge[i]->src instead. Add a merge_clear() function which properly releases all of the merge-related memory, and which sets branch->merge_nr to zero. Use this both in branch_release() and in set_merge(), fixing the leak when set_merge() finds no valid remote_name. Add a set_merge variable to the branch structure, which indicates whether set_merge() has been called. This replaces the previous use of a NULL check against the branch->merge array. With these changes, the merge array is always allocated when merge_nr is non-zero. This use of refspec_item to store the names should be safe. External callers should be using branch_get() to obtain a pointer to the branch, which will call set_merge(), and the callers internal to remote.c already handle the partially initialized refpsec_item structure safely. This end result is cleaner, and avoids duplicating the merge names twice. Signed-off-by: Jacob Keller <jacob.keller@gmail.com> Link: [1] https://lore.kernel.org/git/20250617-jk-submodule-helper-use-url-v2-1-04cbb003177d@gmail.com/ Signed-off-by: Junio C Hamano <gitster@pobox.com>
Diffstat (limited to 'remote.c')
-rw-r--r--remote.c44
1 files changed, 28 insertions, 16 deletions
diff --git a/remote.c b/remote.c
index 4099183cac..ee95126f3f 100644
--- a/remote.c
+++ b/remote.c
@@ -174,9 +174,15 @@ static void remote_clear(struct remote *remote)
static void add_merge(struct branch *branch, const char *name)
{
- ALLOC_GROW(branch->merge_name, branch->merge_nr + 1,
+ struct refspec_item *merge;
+
+ ALLOC_GROW(branch->merge, branch->merge_nr + 1,
branch->merge_alloc);
- branch->merge_name[branch->merge_nr++] = name;
+
+ merge = xcalloc(1, sizeof(*merge));
+ merge->src = xstrdup(name);
+
+ branch->merge[branch->merge_nr++] = merge;
}
struct branches_hash_key {
@@ -247,15 +253,23 @@ static struct branch *make_branch(struct remote_state *remote_state,
return ret;
}
+static void merge_clear(struct branch *branch)
+{
+ for (int i = 0; i < branch->merge_nr; i++) {
+ refspec_item_clear(branch->merge[i]);
+ free(branch->merge[i]);
+ }
+ FREE_AND_NULL(branch->merge);
+ branch->merge_nr = 0;
+}
+
static void branch_release(struct branch *branch)
{
free((char *)branch->name);
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]);
- free(branch->merge);
+ merge_clear(branch);
}
static struct rewrite *make_rewrite(struct rewrites *r,
@@ -429,7 +443,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;
}
@@ -692,7 +706,7 @@ char *remote_ref_for_branch(struct branch *branch, int for_push)
if (branch) {
if (!for_push) {
if (branch->merge_nr) {
- return xstrdup(branch->merge_name[0]);
+ return xstrdup(branch->merge[0]->src);
}
} else {
char *dst;
@@ -1731,32 +1745,30 @@ static void set_merge(struct remote_state *remote_state, struct branch *ret)
if (!ret)
return; /* no branch */
- if (ret->merge)
+ if (ret->set_merge)
return; /* already run */
if (!ret->remote_name || !ret->merge_nr) {
/*
* no merge config; let's make sure we don't confuse callers
* with a non-zero merge_nr but a NULL merge
*/
- ret->merge_nr = 0;
+ merge_clear(ret);
return;
}
+ ret->set_merge = 1;
remote = remotes_remote_get(remote_state, ret->remote_name);
- CALLOC_ARRAY(ret->merge, ret->merge_nr);
for (i = 0; i < ret->merge_nr; i++) {
- ret->merge[i] = xcalloc(1, sizeof(**ret->merge));
- ret->merge[i]->src = xstrdup(ret->merge_name[i]);
if (!remote_find_tracking(remote, ret->merge[i]) ||
strcmp(ret->remote_name, "."))
continue;
- if (repo_dwim_ref(the_repository, ret->merge_name[i],
- strlen(ret->merge_name[i]), &oid, &ref,
+ if (repo_dwim_ref(the_repository, ret->merge[i]->src,
+ strlen(ret->merge[i]->src), &oid, &ref,
0) == 1)
ret->merge[i]->dst = ref;
else
- ret->merge[i]->dst = xstrdup(ret->merge_name[i]);
+ ret->merge[i]->dst = xstrdup(ret->merge[i]->src);
}
}
@@ -1776,7 +1788,7 @@ struct branch *branch_get(const char *name)
int branch_has_merge_config(struct branch *branch)
{
- return branch && !!branch->merge;
+ return branch && branch->set_merge;
}
int branch_merge_matches(struct branch *branch,