Re: [PATCH v3 1/7] remote: remove branch->merge_name and fix branch_release()

From: Lidong Yan <yldhome2d2@gmail.com>
Date: Jun 21, 2025, 02:12 UTC
Message-ID: <CA42712D-C127-4142-9424-2A512F9488CB@gmail.com>
In-Reply-To: <xmqqecve0yvz.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 16 quoted lines

> 
> 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.

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.

---

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;
 }
 
 int branch_merge_matches(struct branch *branch,
---