Re: [PATCH v3 1/7] remote: remove branch->merge_name and fix branch_release()
- From
Jacob Keller <jacob.e.keller@intel.com>
- Date
- Jun 23, 2025, 23:06 UTC
- Message-ID
- <6a61a09f-464c-49d5-b57d-1f8a4d8b1907@intel.com>
- In-Reply-To
- <CA42712D-C127-4142-9424-2A512F9488CB@gmail.com>
On 6/20/2025 7:12 PM, Lidong Yan wrote:
Show 34 quoted lines
> Junio C Hamano <gitster@pobox.com> writes: >> >> Jacob Keller <jacob.e.keller@intel.com> writes: >> >>> 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/ >>> --- >>> remote.h | 4 ++-- >>> branch.c | 4 ++-- >>> builtin/pull.c | 2 +- >>> remote.c | 42 +++++++++++++++++++++++++++--------------- >>> 4 files changed, 32 insertions(+), 20 deletions(-) >> >> This unfortunately makes t5582 segfault. > > I used Clang's Address Sanitizer and found that the segmentation fault > was caused by the following two null pointer dereferences. > > ==501602==Hint: address points to the zero page. > #0 0x72be47ec6ce1 in strcmp ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:465 > #1 0x5d7e2f246b11 in do_fetch builtin/fetch.c:1732 > > ==501614==Hint: address points to the zero page. > #0 0x7b9812ac6ce1 in strcmp ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:465 > #1 0x64e39a76cdc1 in get_ref_map builtin/fetch.c:555 > > I believe this is because we didn't update the corresponding > branch_has_merge_config() function. In the previous implementation, > if branch->remote_name was a null pointer, branch_has_merge_config() > would return false. However, PATCH[v3 1/7] broke this convention. >
Yep, this is likely the correct fix.
Show 7 quoted lines
> The solution could be: > - Replace !!branch->merge with branch->set_merge in branch_has_merge_config(). > - Replace free(branch->merge) with FREE_AND_NULL(branch->merge) in merge_release() > to prevent double free. > > I test my solution locally by just running t5582 and it passed. >
I think the FREE_AND_NULL is a good improvement too.
Show 22 quoted lines
> ---
> diff --git a/remote.c b/remote.c
> index dff76e4626..ee95126f3f 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -259,7 +259,7 @@ static void merge_clear(struct branch *branch)
> refspec_item_clear(branch->merge[i]);
> free(branch->merge[i]);
> }
> - free(branch->merge);
> + FREE_AND_NULL(branch->merge);
> branch->merge_nr = 0;
> }
>
> @@ -1788,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;
> }
> This is probably the real "fix" since branch_has_merge_config should return false until set_merge is called.
> int branch_merge_matches(struct branch *branch, > --- >