On 6/20/2025 7:12 PM, Lidong Yan wrote: > Junio C Hamano <gitster@xxxxxxxxx> writes: >> >> Jacob Keller <jacob.e.keller@xxxxxxxxx> writes: >> >>> This end result is cleaner, and avoids duplicating the merge names >>> twice. >>> >>> Signed-off-by: Jacob Keller <jacob.keller@xxxxxxxxx> >>> Link: [1] https://lore.kernel.org/git/20250617-jk-submodule-helper-use-url-v2-1-04cbb003177d@xxxxxxxxx/ >>> --- >>> 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. > 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. > --- > 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, > --- >
