# [BUG] Segmentation fault in git v2.41.0.rc1 12 messages from 2023-05-24 to 2023-05-24. Participants: Tribo Dar, Taylor Blau, Eric Sunshine, Junio C Hamano, René Scharfe, Jeff King. Thread: https://gitlist.dev/t/59786 ## Tribo Dar, 2023-05-24 06:59 Subject: [BUG] Segmentation fault in git v2.41.0.rc1 Message-ID: URL: https://gitlist.dev/e/CACJVABb8tvgXAowgtHLrC6Uau8Q03VRFu1m7K91SJwjvufqXYw%40mail.gmail.com ``` Running `git submodule update` after commenting out the url setting in both .gitmodules and the corresponding section for the submodule in .git/config results in a segmentation fault instead of a suitable error message. The cause is `sub->url` being NULL here (actual segfault comes a couple calls further): https://github.com/git/git/blob/9e49351c3060e1fa6e0d2de64505b7becf157f28/builtin/submodule--helper.c#L2027 Possibly relevant: a similar snippet is being properly protected against null-pointer dereferencing here: https://github.com/git/git/blob/9e49351c3060e1fa6e0d2de64505b7becf157f28/builtin/submodule--helper.c#L1243 ``` ## Taylor Blau, 2023-05-24 14:47 Subject: Re: [BUG] Segmentation fault in git v2.41.0.rc1 Message-ID: URL: https://gitlist.dev/e/ZG4jcceXn2G6Xic0%40nand.local In-Reply-To: ``` Hi Tribo, On Wed, May 24, 2023 at 06:59:55AM +0000, Tribo Dar wrote: > Running `git submodule update` after commenting out the url setting in both > .gitmodules and the corresponding section for the submodule in .git/config > results in a segmentation fault instead of a suitable error message. Thanks for reporting. I was able to bisect this with the following bisection script: --- >8 --- #!/bin/sh rm -fr clone if ! test -d repo then git init repo ( cd repo git submodule add git@github.com:ttaylorr/dotfiles.git git add --all . git commit -m "initial commit" ) fi git.compile clone repo clone ( cd clone git submodule init sed -ie 's/\turl = git@/#\0/g' .gitmodules .git/config git.compile submodule update test $? -gt 128 && exit 1 exit 0 ) --- 8< --- which points to e0a862fdaf (submodule helper: convert relative URL to absolute URL if needed, 2018-10-16) as the culprit. The fix appears to be something like: --- 8< --- diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 6bf8d666ce..98ee7697f3 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2024,8 +2024,9 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, strbuf_reset(&sb); strbuf_addf(&sb, "submodule.%s.url", sub->name); if (repo_config_get_string_tmp(the_repository, sb.buf, &url)) { - if (starts_with_dot_slash(sub->url) || - starts_with_dot_dot_slash(sub->url)) { + if (sub && sub->url && + (starts_with_dot_slash(sub->url) || + starts_with_dot_dot_slash(sub->url))) { url = resolve_relative_url(sub->url, NULL, 0); need_free_url = 1; } else diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index eae6a46ef3..c97c543dd8 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -1351,6 +1351,22 @@ test_expect_success 'clone active submodule without submodule url set' ' ) ' +test_expect_success 'update submodules without url set in .gitconfig' ' + test_when_finished "rm -rf multisuper_clone" && + git clone file://"$pwd"/multisuper multisuper_clone && + + git -C multisuper_clone submodule init && + for s in sub0 sub1 sub2 sub3 + do + key=submodule.$s.url && + git -C multisuper_clone config --local --unset $key && + git -C multisuper_clone config --file .gitmodules --unset $key || return 1 + done && + + test_must_fail git -C multisuper_clone submodule update 2>err && + grep "option .url. requires a value" err +' + test_expect_success 'clone --recurse-submodules with a pathspec works' ' test_when_finished "rm -rf multisuper_clone" && cat >expected <<-\EOF && --- >8 --- I'll clean it up and submit it as a patch shortly. Thanks, Taylor ``` ## Taylor Blau, 2023-05-24 16:02 Subject: [PATCH] builtin/submodule--helper.c: handle missing submodule URLs Message-ID: URL: https://gitlist.dev/e/f7a8de14fe255286e62fc46d0a3083189f46bcc6.1684944140.git.me%40ttaylorr.com In-Reply-To: ``` In e0a862fdaf (submodule helper: convert relative URL to absolute URL if needed, 2018-10-16), `prepare_to_clone_next_submodule()` lost the ability to handle URL-less submodules, due to a change from: if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) url = sub->url; to if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) { if (starts_with_dot_slash(sub->url) || starts_with_dot_dot_slash(sub->url)) { /* ... */ } } , which will segfault when `sub->url` is NULL, since both `starts_with_dot_slash()` does not guard its arguments as non-NULL. Guard the checks to both of the above functions by first whether `sub->url` is non-NULL. There is no need to check whether `sub` itself is NULL, since we already perform this check earlier in `prepare_to_clone_next_submodule()`. By adding a NULL-ness check on `sub->url`, we'll fall into the 'else' branch, setting `url` to `sub->url` (which is NULL). When we then try and invoke `git submodule-helper` below, we'll get the expected error message: error: option `url' requires a value Note that this means that `--url` and its value must come last when preparing the argument list, to avoid confusing the next argument as the value for `--url` if `url` itself is NULL. Reported-by: Tribo Dar <3bodar@gmail.com> Signed-off-by: Taylor Blau --- builtin/submodule--helper.c | 11 ++++++++--- t/t7400-submodule-basic.sh | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 6bf8d666ce..a7b7ea374f 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2024,8 +2024,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, strbuf_reset(&sb); strbuf_addf(&sb, "submodule.%s.url", sub->name); if (repo_config_get_string_tmp(the_repository, sb.buf, &url)) { - if (starts_with_dot_slash(sub->url) || - starts_with_dot_dot_slash(sub->url)) { + if (sub->url && (starts_with_dot_slash(sub->url) || + starts_with_dot_dot_slash(sub->url))) { url = resolve_relative_url(sub->url, NULL, 0); need_free_url = 1; } else @@ -2069,7 +2069,6 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, strvec_push(&child->args, "--require-init"); strvec_pushl(&child->args, "--path", sub->path, NULL); strvec_pushl(&child->args, "--name", sub->name, NULL); - strvec_pushl(&child->args, "--url", url, NULL); if (suc->update_data->references.nr) { struct string_list_item *item; @@ -2082,6 +2081,12 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, strvec_push(&child->args, suc->update_data->single_branch ? "--single-branch" : "--no-single-branch"); + /* + * `url` might be NULL, so ensure that this argument is passed + * last to avoid confusing the next argument as the value for + * `--url`. + */ + strvec_pushl(&child->args, "--url", url, NULL); cleanup: free(displaypath); diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index eae6a46ef3..c97c543dd8 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -1351,6 +1351,22 @@ test_expect_success 'clone active submodule without submodule url set' ' ) ' +test_expect_success 'update submodules without url set in .gitconfig' ' + test_when_finished "rm -rf multisuper_clone" && + git clone file://"$pwd"/multisuper multisuper_clone && + + git -C multisuper_clone submodule init && + for s in sub0 sub1 sub2 sub3 + do + key=submodule.$s.url && + git -C multisuper_clone config --local --unset $key && + git -C multisuper_clone config --file .gitmodules --unset $key || return 1 + done && + + test_must_fail git -C multisuper_clone submodule update 2>err && + grep "option .url. requires a value" err +' + test_expect_success 'clone --recurse-submodules with a pathspec works' ' test_when_finished "rm -rf multisuper_clone" && cat >expected <<-\EOF && -- 2.41.0.rc1.11.gf7a8de14fe ``` ## Taylor Blau, 2023-05-24 16:25 Subject: [PATCH v2] builtin/submodule--helper.c: handle missing submodule URLs Message-ID: URL: https://gitlist.dev/e/bc204f450a80f7e6379b3a8564fc54637a266e7e.1684945504.git.me%40ttaylorr.com In-Reply-To: ``` In e0a862fdaf (submodule helper: convert relative URL to absolute URL if needed, 2018-10-16), `prepare_to_clone_next_submodule()` lost the ability to handle URL-less submodules, due to a change from: if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) url = sub->url; to if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) { if (starts_with_dot_slash(sub->url) || starts_with_dot_dot_slash(sub->url)) { /* ... */ } } , which will segfault when `sub->url` is NULL, since both `starts_with_dot_slash()` does not guard its arguments as non-NULL. Guard the checks to both of the above functions by first whether `sub->url` is non-NULL. There is no need to check whether `sub` itself is NULL, since we already perform this check earlier in `prepare_to_clone_next_submodule()`. By adding a NULL-ness check on `sub->url`, we'll fall into the 'else' branch, setting `url` to `sub->url` (which is NULL). Before attempting to invoke `git submodule--helper clone`, check whether `url` is NULL, and die() if it is. Reported-by: Tribo Dar <3bodar@gmail.com> Signed-off-by: Taylor Blau --- Junio: feel free to queue either, but after thinking on v1 a little more I think that it is pretty gross and that this version is much cleaner. Range-diff against v1: 1: f7a8de14fe ! 1: bc204f450a builtin/submodule--helper.c: handle missing submodule URLs @@ Commit message `prepare_to_clone_next_submodule()`. By adding a NULL-ness check on `sub->url`, we'll fall into the 'else' - branch, setting `url` to `sub->url` (which is NULL). When we then try - and invoke `git submodule-helper` below, we'll get the expected error - message: - - error: option `url' requires a value - - Note that this means that `--url` and its value must come last when - preparing the argument list, to avoid confusing the next argument as the - value for `--url` if `url` itself is NULL. + branch, setting `url` to `sub->url` (which is NULL). Before attempting + to invoke `git submodule--helper clone`, check whether `url` is NULL, + and die() if it is. Reported-by: Tribo Dar <3bodar@gmail.com> Signed-off-by: Taylor Blau @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st url = resolve_relative_url(sub->url, NULL, 0); need_free_url = 1; } else + url = sub->url; + } + ++ if (!url) ++ die(_("cannot clone submodule '%s' without a URL"), sub->name); ++ + strbuf_reset(&sb); + strbuf_addf(&sb, "%s/.git", ce->name); + needs_cloning = !file_exists(sb.buf); @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce, + if (suc->update_data->filter_options && suc->update_data->filter_options->choice) + strvec_pushf(&child->args, "--filter=%s", + expand_list_objects_filter_spec(suc->update_data->filter_options)); ++ strvec_pushl(&child->args, "--url", url, NULL); + if (suc->update_data->require_init) strvec_push(&child->args, "--require-init"); strvec_pushl(&child->args, "--path", sub->path, NULL); strvec_pushl(&child->args, "--name", sub->name, NULL); @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st if (suc->update_data->references.nr) { struct string_list_item *item; -@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce, - strvec_push(&child->args, suc->update_data->single_branch ? - "--single-branch" : - "--no-single-branch"); -+ /* -+ * `url` might be NULL, so ensure that this argument is passed -+ * last to avoid confusing the next argument as the value for -+ * `--url`. -+ */ -+ strvec_pushl(&child->args, "--url", url, NULL); - - cleanup: - free(displaypath); ## t/t7400-submodule-basic.sh ## @@ t/t7400-submodule-basic.sh: test_expect_success 'clone active submodule without submodule url set' ' @@ t/t7400-submodule-basic.sh: test_expect_success 'clone active submodule without + done && + + test_must_fail git -C multisuper_clone submodule update 2>err && -+ grep "option .url. requires a value" err ++ grep "cannot clone submodule .sub[0-3]. without a URL" err +' + test_expect_success 'clone --recurse-submodules with a pathspec works' ' builtin/submodule--helper.c | 9 ++++++--- t/t7400-submodule-basic.sh | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 6bf8d666ce..efd0761cd0 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2024,14 +2024,17 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, strbuf_reset(&sb); strbuf_addf(&sb, "submodule.%s.url", sub->name); if (repo_config_get_string_tmp(the_repository, sb.buf, &url)) { - if (starts_with_dot_slash(sub->url) || - starts_with_dot_dot_slash(sub->url)) { + if (sub->url && (starts_with_dot_slash(sub->url) || + starts_with_dot_dot_slash(sub->url))) { url = resolve_relative_url(sub->url, NULL, 0); need_free_url = 1; } else url = sub->url; } + if (!url) + die(_("cannot clone submodule '%s' without a URL"), sub->name); + strbuf_reset(&sb); strbuf_addf(&sb, "%s/.git", ce->name); needs_cloning = !file_exists(sb.buf); @@ -2065,11 +2068,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, if (suc->update_data->filter_options && suc->update_data->filter_options->choice) strvec_pushf(&child->args, "--filter=%s", expand_list_objects_filter_spec(suc->update_data->filter_options)); + strvec_pushl(&child->args, "--url", url, NULL); if (suc->update_data->require_init) strvec_push(&child->args, "--require-init"); strvec_pushl(&child->args, "--path", sub->path, NULL); strvec_pushl(&child->args, "--name", sub->name, NULL); - strvec_pushl(&child->args, "--url", url, NULL); if (suc->update_data->references.nr) { struct string_list_item *item; diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index eae6a46ef3..d9fbabb2b9 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -1351,6 +1351,22 @@ test_expect_success 'clone active submodule without submodule url set' ' ) ' +test_expect_success 'update submodules without url set in .gitconfig' ' + test_when_finished "rm -rf multisuper_clone" && + git clone file://"$pwd"/multisuper multisuper_clone && + + git -C multisuper_clone submodule init && + for s in sub0 sub1 sub2 sub3 + do + key=submodule.$s.url && + git -C multisuper_clone config --local --unset $key && + git -C multisuper_clone config --file .gitmodules --unset $key || return 1 + done && + + test_must_fail git -C multisuper_clone submodule update 2>err && + grep "cannot clone submodule .sub[0-3]. without a URL" err +' + test_expect_success 'clone --recurse-submodules with a pathspec works' ' test_when_finished "rm -rf multisuper_clone" && cat >expected <<-\EOF && -- 2.41.0.rc1.11.gf7a8de14fe.dirty ``` ## Eric Sunshine, 2023-05-24 18:48 Subject: Re: [PATCH v2] builtin/submodule--helper.c: handle missing submodule URLs Message-ID: URL: https://gitlist.dev/e/CAPig%2BcT4c4f8DtNJOmNJKRYVX0HNz9fSgkF9hAbzaCXV0ozydg%40mail.gmail.com In-Reply-To: ``` On Wed, May 24, 2023 at 12:40 PM Taylor Blau wrote: > In e0a862fdaf (submodule helper: convert relative URL to absolute URL if > needed, 2018-10-16), `prepare_to_clone_next_submodule()` lost the > ability to handle URL-less submodules, due to a change from: > > if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) > url = sub->url; > > to > > if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) { > if (starts_with_dot_slash(sub->url) || > starts_with_dot_dot_slash(sub->url)) { > /* ... */ > } > } > > , which will segfault when `sub->url` is NULL, since both > `starts_with_dot_slash()` does not guard its arguments as non-NULL. > > Guard the checks to both of the above functions by first whether s/first/first checking/ > `sub->url` is non-NULL. There is no need to check whether `sub` itself > is NULL, since we already perform this check earlier in > `prepare_to_clone_next_submodule()`. > > By adding a NULL-ness check on `sub->url`, we'll fall into the 'else' > branch, setting `url` to `sub->url` (which is NULL). Before attempting > to invoke `git submodule--helper clone`, check whether `url` is NULL, > and die() if it is. > > Reported-by: Tribo Dar <3bodar@gmail.com> > Signed-off-by: Taylor Blau > --- > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > @@ -2024,14 +2024,17 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, > - if (starts_with_dot_slash(sub->url) || > - starts_with_dot_dot_slash(sub->url)) { > + if (sub->url && (starts_with_dot_slash(sub->url) || > + starts_with_dot_dot_slash(sub->url))) { > url = resolve_relative_url(sub->url, NULL, 0); > need_free_url = 1; > } else > url = sub->url; > } > > + if (!url) > + die(_("cannot clone submodule '%s' without a URL"), sub->name); Good. The first version of this patch was more difficult to reason about due to its "error-at-a-distance" approach. This version is much cleaner and obvious. > @@ -2065,11 +2068,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, > strvec_pushf(&child->args, "--filter=%s", > expand_list_objects_filter_spec(suc->update_data->filter_options)); > + strvec_pushl(&child->args, "--url", url, NULL); > if (suc->update_data->require_init) > strvec_push(&child->args, "--require-init"); > strvec_pushl(&child->args, "--path", sub->path, NULL); > strvec_pushl(&child->args, "--name", sub->name, NULL); > - strvec_pushl(&child->args, "--url", url, NULL); This change is unnecessary now, isn't it? Or is there something nonobvious going on here? ``` ## Taylor Blau, 2023-05-24 19:50 Subject: Re: [PATCH v2] builtin/submodule--helper.c: handle missing submodule URLs Message-ID: URL: https://gitlist.dev/e/ZG5qac9YVvjN%2FR3o%40nand.local In-Reply-To: ``` On Wed, May 24, 2023 at 02:48:40PM -0400, Eric Sunshine wrote: > On Wed, May 24, 2023 at 12:40 PM Taylor Blau wrote: > > Guard the checks to both of the above functions by first whether > > s/first/first checking/ Oops. Good eyes, thanks for spotting. > > @@ -2065,11 +2068,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, > > strvec_pushf(&child->args, "--filter=%s", > > expand_list_objects_filter_spec(suc->update_data->filter_options)); > > + strvec_pushl(&child->args, "--url", url, NULL); > > if (suc->update_data->require_init) > > strvec_push(&child->args, "--require-init"); > > strvec_pushl(&child->args, "--path", sub->path, NULL); > > strvec_pushl(&child->args, "--name", sub->name, NULL); > > - strvec_pushl(&child->args, "--url", url, NULL); > > This change is unnecessary now, isn't it? Or is there something > nonobvious going on here? Yeah, this is a stray diff. I'll send a cleaned up version as soon as 'make test' finishes ;-). Thanks, Taylor ``` ## Taylor Blau, 2023-05-24 19:51 Subject: [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs Message-ID: URL: https://gitlist.dev/e/ae6cf3fa461b85e346f034371dae56a2790dfa20.1684957882.git.me%40ttaylorr.com In-Reply-To: ``` In e0a862fdaf (submodule helper: convert relative URL to absolute URL if needed, 2018-10-16), `prepare_to_clone_next_submodule()` lost the ability to handle URL-less submodules, due to a change from: if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) url = sub->url; to if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) { if (starts_with_dot_slash(sub->url) || starts_with_dot_dot_slash(sub->url)) { /* ... */ } } , which will segfault when `sub->url` is NULL, since both `starts_with_dot_slash()` does not guard its arguments as non-NULL. Guard the checks to both of the above functions by first checking whether `sub->url` is non-NULL. There is no need to check whether `sub` itself is NULL, since we already perform this check earlier in `prepare_to_clone_next_submodule()`. By adding a NULL-ness check on `sub->url`, we'll fall into the 'else' branch, setting `url` to `sub->url` (which is NULL). Before attempting to invoke `git submodule--helper clone`, check whether `url` is NULL, and die() if it is. Reported-by: Tribo Dar <3bodar@gmail.com> Signed-off-by: Taylor Blau --- Range-diff against v2: 1: 38202878b5 ! 1: ae6cf3fa46 builtin/submodule--helper.c: handle missing submodule URLs @@ Commit message , which will segfault when `sub->url` is NULL, since both `starts_with_dot_slash()` does not guard its arguments as non-NULL. - Guard the checks to both of the above functions by first whether - `sub->url` is non-NULL. There is no need to check whether `sub` itself - is NULL, since we already perform this check earlier in + Guard the checks to both of the above functions by first checking + whether `sub->url` is non-NULL. There is no need to check whether `sub` + itself is NULL, since we already perform this check earlier in `prepare_to_clone_next_submodule()`. By adding a NULL-ness check on `sub->url`, we'll fall into the 'else' @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st strbuf_reset(&sb); strbuf_addf(&sb, "%s/.git", ce->name); needs_cloning = !file_exists(sb.buf); -@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce, - if (suc->update_data->filter_options && suc->update_data->filter_options->choice) - strvec_pushf(&child->args, "--filter=%s", - expand_list_objects_filter_spec(suc->update_data->filter_options)); -+ strvec_pushl(&child->args, "--url", url, NULL); - if (suc->update_data->require_init) - strvec_push(&child->args, "--require-init"); - strvec_pushl(&child->args, "--path", sub->path, NULL); - strvec_pushl(&child->args, "--name", sub->name, NULL); -- strvec_pushl(&child->args, "--url", url, NULL); - if (suc->update_data->references.nr) { - struct string_list_item *item; - ## t/t7400-submodule-basic.sh ## @@ t/t7400-submodule-basic.sh: test_expect_success 'clone active submodule without submodule url set' ' builtin/submodule--helper.c | 7 +++++-- t/t7400-submodule-basic.sh | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 6bf8d666ce..6a16208e8a 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2024,14 +2024,17 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, strbuf_reset(&sb); strbuf_addf(&sb, "submodule.%s.url", sub->name); if (repo_config_get_string_tmp(the_repository, sb.buf, &url)) { - if (starts_with_dot_slash(sub->url) || - starts_with_dot_dot_slash(sub->url)) { + if (sub->url && (starts_with_dot_slash(sub->url) || + starts_with_dot_dot_slash(sub->url))) { url = resolve_relative_url(sub->url, NULL, 0); need_free_url = 1; } else url = sub->url; } + if (!url) + die(_("cannot clone submodule '%s' without a URL"), sub->name); + strbuf_reset(&sb); strbuf_addf(&sb, "%s/.git", ce->name); needs_cloning = !file_exists(sb.buf); diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index eae6a46ef3..d9fbabb2b9 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -1351,6 +1351,22 @@ test_expect_success 'clone active submodule without submodule url set' ' ) ' +test_expect_success 'update submodules without url set in .gitconfig' ' + test_when_finished "rm -rf multisuper_clone" && + git clone file://"$pwd"/multisuper multisuper_clone && + + git -C multisuper_clone submodule init && + for s in sub0 sub1 sub2 sub3 + do + key=submodule.$s.url && + git -C multisuper_clone config --local --unset $key && + git -C multisuper_clone config --file .gitmodules --unset $key || return 1 + done && + + test_must_fail git -C multisuper_clone submodule update 2>err && + grep "cannot clone submodule .sub[0-3]. without a URL" err +' + test_expect_success 'clone --recurse-submodules with a pathspec works' ' test_when_finished "rm -rf multisuper_clone" && cat >expected <<-\EOF && -- 2.41.0.rc1.11.gae6cf3fa46 ``` ## Junio C Hamano, 2023-05-24 20:25 Subject: Re: [BUG] Segmentation fault in git v2.41.0.rc1 Message-ID: URL: https://gitlist.dev/e/xmqqttw1fpd9.fsf%40gitster.g In-Reply-To: ``` Taylor Blau writes: > which points to e0a862fdaf (submodule helper: convert relative URL to > absolute URL if needed, 2018-10-16) as the culprit. Whew, that is fairly ancient. I was afraid if we have another regression, but it does not look that way. Fixing is certainly good and we'd need to eventually get to it, but we have luxury to make sure that the fix is sound without having to rush anything ;-) In the meantime, "if it hurts, don't do it" is what we can say. Telling random users to muck with their config in certain ways that violate the way how the system represents (un-)initialized submodules and then to run certain command to induce a NULL pointer dereference is rather an ineffective social engineering as an attack vector, so this is not urgent in that sense. I am more worried that the original report talked about mucking with the in-tree .gitmodules file affects the result, though. Once a submodule is initialized, what is in the file for that submodule should not affect the working of local Git (otherwise the file can be used as a route to inject stuff to unsuspecting repositories), but in this case apparently it does? Thanks. ``` ## René Scharfe, 2023-05-24 20:29 Subject: Re: [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs Message-ID: URL: https://gitlist.dev/e/c8d32d5a-9803-9eb7-baaa-49b4521f0c37%40web.de In-Reply-To: ``` Am 24.05.23 um 21:51 schrieb Taylor Blau: > There is no need to check whether `sub` > itself is NULL, since we already perform this check earlier in > `prepare_to_clone_next_submodule()`. Right, and if "sub" is NULL then next_submodule_warn_missing() is called and prepare_to_clone_next_submodule() is exited early. > By adding a NULL-ness check on `sub->url`, we'll fall into the 'else' > branch, setting `url` to `sub->url` (which is NULL). Before attempting > to invoke `git submodule--helper clone`, check whether `url` is NULL, > and die() if it is. Why die() here instead of just warn and skip as well? René ``` ## Jeff King, 2023-05-24 20:33 Subject: Re: [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs Message-ID: <20230524203321.GD892557@coredump.intra.peff.net> URL: https://gitlist.dev/e/20230524203321.GD892557%40coredump.intra.peff.net In-Reply-To: ``` On Wed, May 24, 2023 at 03:51:43PM -0400, Taylor Blau wrote: > In e0a862fdaf (submodule helper: convert relative URL to absolute URL if > needed, 2018-10-16), `prepare_to_clone_next_submodule()` lost the > ability to handle URL-less submodules, due to a change from: > > if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) > url = sub->url; > > to > > if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) { > if (starts_with_dot_slash(sub->url) || > starts_with_dot_dot_slash(sub->url)) { > /* ... */ > } > } This patch looks pretty good to me. I read your v1 and the word "gross" also crossed my mind at the "--url" handling. This one is much better. I did have a few questions, though (below). If I understand correctly, this is not at all new in the -rc releases, but just something that happened to get unearthed? I.e., it can wait until post-release. > , which will segfault when `sub->url` is NULL, since both > `starts_with_dot_slash()` does not guard its arguments as non-NULL. Funny gramm-o, presumably from editing: "both" is plural, but "does" and "its" are singular. I think the gist of it is communicated, though. > Guard the checks to both of the above functions by first checking > whether `sub->url` is non-NULL. There is no need to check whether `sub` > itself is NULL, since we already perform this check earlier in > `prepare_to_clone_next_submodule()`. Good, thanks for checking (and communicating) that possible gotha. > By adding a NULL-ness check on `sub->url`, we'll fall into the 'else' > branch, setting `url` to `sub->url` (which is NULL). Before attempting > to invoke `git submodule--helper clone`, check whether `url` is NULL, > and die() if it is. If I hadn't read v1, I might wonder whether this die() is consistent with the existing behavior. But the point is that submodule--helper would have barfed in such a case anyway, so we are just trading one error for another. One side effect, though, is that this die() will take down the whole superproject process. Whereas I think the intent of the submodule code is to keep going, handling other submodules, even if one fails. This isn't a failure exactly (more of a misconfiguration, if I understand it). But should we be somehow returning an error instead? I say "somehow" because it's not clear how to work that in with the needs_cloning return value (obviously we can say "0", but that is the same as the "skipped" code path; we presumably want to tell the caller there was a failure, so it affects the ultimate return code). > +test_expect_success 'update submodules without url set in .gitconfig' ' Should this be .gitmodules in the title? -Peff ``` ## Taylor Blau, 2023-05-24 20:36 Subject: Re: [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs Message-ID: URL: https://gitlist.dev/e/ZG51XsEq4JB9IK33%40nand.local In-Reply-To: ``` On Wed, May 24, 2023 at 10:29:39PM +0200, René Scharfe wrote: > Am 24.05.23 um 21:51 schrieb Taylor Blau: > > > By adding a NULL-ness check on `sub->url`, we'll fall into the 'else' > > branch, setting `url` to `sub->url` (which is NULL). Before attempting > > to invoke `git submodule--helper clone`, check whether `url` is NULL, > > and die() if it is. > > Why die() here instead of just warn and skip as well? That's a good point. When I read prepare_to_clone_next_submodule(), I thought that it was already too late to skip that submodule. But my understanding was incorrect, we could easily issue a warning() and return '0', which would indicate to skip it. But I concur with Junio earlier in the thread that we don't have to rush things to get this into -rc2 or even 2.41, since this bug has been with us since v2.20.0. Thanks, Taylor ``` ## Jeff King, 2023-05-24 22:58 Subject: Re: [PATCH] builtin/submodule--helper.c: handle missing submodule URLs Message-ID: <20230524225807.GA904398@coredump.intra.peff.net> URL: https://gitlist.dev/e/20230524225807.GA904398%40coredump.intra.peff.net In-Reply-To: ``` On Wed, May 24, 2023 at 12:02:26PM -0400, Taylor Blau wrote: > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 6bf8d666ce..a7b7ea374f 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2024,8 +2024,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, > strbuf_reset(&sb); > strbuf_addf(&sb, "submodule.%s.url", sub->name); > if (repo_config_get_string_tmp(the_repository, sb.buf, &url)) { > - if (starts_with_dot_slash(sub->url) || > - starts_with_dot_dot_slash(sub->url)) { > + if (sub->url && (starts_with_dot_slash(sub->url) || > + starts_with_dot_dot_slash(sub->url))) { > url = resolve_relative_url(sub->url, NULL, 0); > need_free_url = 1; > } else Oh, btw, this need_free_url made me look at the memory handling for the other side of the else. I think it is all good, but it would be a little simpler using an extra pointer instead of a boolean: diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 6a16208e8a..6b7849b828 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1984,7 +1984,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, ud->super_prefix); struct strbuf sb = STRBUF_INIT; int needs_cloning = 0; - int need_free_url = 0; + char *to_free_url = NULL; if (ce_stage(ce)) { strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath); @@ -2025,10 +2025,9 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, strbuf_addf(&sb, "submodule.%s.url", sub->name); if (repo_config_get_string_tmp(the_repository, sb.buf, &url)) { if (sub->url && (starts_with_dot_slash(sub->url) || - starts_with_dot_dot_slash(sub->url))) { - url = resolve_relative_url(sub->url, NULL, 0); - need_free_url = 1; - } else + starts_with_dot_dot_slash(sub->url))) + url = to_free_url = resolve_relative_url(sub->url, NULL, 0); + else url = sub->url; } @@ -2089,8 +2088,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, cleanup: free(displaypath); strbuf_release(&sb); - if (need_free_url) - free((void*)url); + free(to_free_url); return needs_cloning; } Probably at this point that is just churn, but I thought I'd throw it out there. -Peff ```