diff options
| author | Jacob Keller <jacob.keller@gmail.com> | 2025-06-23 16:11:29 -0700 |
|---|---|---|
| committer | Junio C Hamano <gitster@pobox.com> | 2025-06-23 16:38:55 -0700 |
| commit | f62dcc7f30d16af29c0f707005aceb5eb6119279 (patch) | |
| tree | 7251fb69e14e76d62832974534637b804485847f /remote.h | |
| parent | 16bd9f20a403117f2e0d9bcda6c6e621d3763e77 (diff) | |
| download | git-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.h')
| -rw-r--r-- | remote.h | 4 |
1 files changed, 2 insertions, 2 deletions
@@ -315,8 +315,8 @@ struct branch { char *pushremote_name; - /* An array of the "merge" lines in the configuration. */ - const char **merge_name; + /* True if set_merge() has been called to finalize the merge array */ + int set_merge; /** * An array of the struct refspecs used for the merge lines. That is, |
