{"thread":{"id":"59786","subject":"[BUG] Segmentation fault in git v2.41.0.rc1","startedAt":"2023-05-24T07:00:14Z","lastAt":"2023-05-24T22:58:12Z","messageCount":12,"participants":["Tribo Dar","Taylor Blau","Eric Sunshine","Junio C Hamano","René Scharfe","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"477684","messageId":"CACJVABb8tvgXAowgtHLrC6Uau8Q03VRFu1m7K91SJwjvufqXYw@mail.gmail.com","threadId":"59786","inReplyTo":null,"subject":"[BUG] Segmentation fault in git v2.41.0.rc1","fromName":"Tribo Dar","fromEmail":"3bodar@gmail.com","sentAt":"2023-05-24T06:59:55Z","receivedAt":"2023-05-24T07:00:14Z","isPatch":false,"sender":{"key":"3bodar@gmail.com","avatar":null},"body":"Running `git submodule update` after commenting out the url setting in both\n.gitmodules and the corresponding section for the submodule in .git/config\nresults in a segmentation fault instead of a suitable error message.\n\nThe cause is `sub->url` being NULL here (actual segfault\ncomes a couple calls further):\nhttps://github.com/git/git/blob/9e49351c3060e1fa6e0d2de64505b7becf157f28/builtin/submodule--helper.c#L2027\n\nPossibly relevant: a similar snippet is being properly protected against\nnull-pointer dereferencing here:\nhttps://github.com/git/git/blob/9e49351c3060e1fa6e0d2de64505b7becf157f28/builtin/submodule--helper.c#L1243\n"},{"id":"477687","messageId":"ZG4jcceXn2G6Xic0@nand.local","threadId":"59786","inReplyTo":"CACJVABb8tvgXAowgtHLrC6Uau8Q03VRFu1m7K91SJwjvufqXYw@mail.gmail.com","subject":"Re: [BUG] Segmentation fault in git v2.41.0.rc1","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-24T14:47:13Z","receivedAt":"2023-05-24T14:47:25Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Tribo,\n\nOn Wed, May 24, 2023 at 06:59:55AM +0000, Tribo Dar wrote:\n> Running `git submodule update` after commenting out the url setting in both\n> .gitmodules and the corresponding section for the submodule in .git/config\n> results in a segmentation fault instead of a suitable error message.\n\nThanks for reporting. I was able to bisect this with the following\nbisection script:\n\n--- >8 ---\n#!/bin/sh\n\nrm -fr clone\n\nif ! test -d repo\nthen\n    git init repo\n    (\n        cd repo\n\n        git submodule add git@github.com:ttaylorr/dotfiles.git\n        git add --all .\n        git commit -m \"initial commit\"\n    )\nfi\n\ngit.compile clone repo clone\n(\n    cd clone\n\n    git submodule init\n    sed -ie 's/\\turl = git@/#\\0/g' .gitmodules .git/config\n\n    git.compile submodule update\n    test $? -gt 128 && exit 1\n\n    exit 0\n)\n--- 8< ---\n\nwhich points to e0a862fdaf (submodule helper: convert relative URL to\nabsolute URL if needed, 2018-10-16) as the culprit. The fix appears to\nbe something like:\n\n--- 8< ---\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6bf8d666ce..98ee7697f3 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2024,8 +2024,9 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n \tif (repo_config_get_string_tmp(the_repository, sb.buf, &url)) {\n-\t\tif (starts_with_dot_slash(sub->url) ||\n-\t\t    starts_with_dot_dot_slash(sub->url)) {\n+\t\tif (sub && sub->url &&\n+\t\t    (starts_with_dot_slash(sub->url) ||\n+\t\t     starts_with_dot_dot_slash(sub->url))) {\n \t\t\turl = resolve_relative_url(sub->url, NULL, 0);\n \t\t\tneed_free_url = 1;\n \t\t} else\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex eae6a46ef3..c97c543dd8 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1351,6 +1351,22 @@ test_expect_success 'clone active submodule without submodule url set' '\n \t)\n '\n\n+test_expect_success 'update submodules without url set in .gitconfig' '\n+\ttest_when_finished \"rm -rf multisuper_clone\" &&\n+\tgit clone file://\"$pwd\"/multisuper multisuper_clone &&\n+\n+\tgit -C multisuper_clone submodule init &&\n+\tfor s in sub0 sub1 sub2 sub3\n+\tdo\n+\t\tkey=submodule.$s.url &&\n+\t\tgit -C multisuper_clone config --local --unset $key &&\n+\t\tgit -C multisuper_clone config --file .gitmodules --unset $key || return 1\n+\tdone &&\n+\n+\ttest_must_fail git -C multisuper_clone submodule update 2>err &&\n+\tgrep \"option .url. requires a value\" err\n+'\n+\n test_expect_success 'clone --recurse-submodules with a pathspec works' '\n \ttest_when_finished \"rm -rf multisuper_clone\" &&\n \tcat >expected <<-\\EOF &&\n--- >8 ---\n\nI'll clean it up and submit it as a patch shortly.\n\nThanks,\nTaylor\n"},{"id":"477688","messageId":"f7a8de14fe255286e62fc46d0a3083189f46bcc6.1684944140.git.me@ttaylorr.com","threadId":"59786","inReplyTo":"ZG4jcceXn2G6Xic0@nand.local","subject":"[PATCH] builtin/submodule--helper.c: handle missing submodule URLs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-24T16:02:26Z","receivedAt":"2023-05-24T16:03:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In e0a862fdaf (submodule helper: convert relative URL to absolute URL if\nneeded, 2018-10-16), `prepare_to_clone_next_submodule()` lost the\nability to handle URL-less submodules, due to a change from:\n\n    if (repo_get_config_string_const(the_repostiory, sb.buf, &url))\n        url = sub->url;\n\nto\n\n    if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) {\n        if (starts_with_dot_slash(sub->url) ||\n            starts_with_dot_dot_slash(sub->url)) {\n                /* ... */\n            }\n    }\n\n, which will segfault when `sub->url` is NULL, since both\n`starts_with_dot_slash()` does not guard its arguments as non-NULL.\n\nGuard the checks to both of the above functions by first whether\n`sub->url` is non-NULL. There is no need to check whether `sub` itself\nis NULL, since we already perform this check earlier in\n`prepare_to_clone_next_submodule()`.\n\nBy adding a NULL-ness check on `sub->url`, we'll fall into the 'else'\nbranch, setting `url` to `sub->url` (which is NULL). When we then try\nand invoke `git submodule-helper` below, we'll get the expected error\nmessage:\n\n    error: option `url' requires a value\n\nNote that this means that `--url` and its value must come last when\npreparing the argument list, to avoid confusing the next argument as the\nvalue for `--url` if `url` itself is NULL.\n\nReported-by: Tribo Dar <3bodar@gmail.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/submodule--helper.c | 11 ++++++++---\n t/t7400-submodule-basic.sh  | 16 ++++++++++++++++\n 2 files changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6bf8d666ce..a7b7ea374f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2024,8 +2024,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n \tif (repo_config_get_string_tmp(the_repository, sb.buf, &url)) {\n-\t\tif (starts_with_dot_slash(sub->url) ||\n-\t\t    starts_with_dot_dot_slash(sub->url)) {\n+\t\tif (sub->url && (starts_with_dot_slash(sub->url) ||\n+\t\t\t\t starts_with_dot_dot_slash(sub->url))) {\n \t\t\turl = resolve_relative_url(sub->url, NULL, 0);\n \t\t\tneed_free_url = 1;\n \t\t} else\n@@ -2069,7 +2069,6 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\tstrvec_push(&child->args, \"--require-init\");\n \tstrvec_pushl(&child->args, \"--path\", sub->path, NULL);\n \tstrvec_pushl(&child->args, \"--name\", sub->name, NULL);\n-\tstrvec_pushl(&child->args, \"--url\", url, NULL);\n \tif (suc->update_data->references.nr) {\n \t\tstruct string_list_item *item;\n \n@@ -2082,6 +2081,12 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\tstrvec_push(&child->args, suc->update_data->single_branch ?\n \t\t\t\t\t      \"--single-branch\" :\n \t\t\t\t\t      \"--no-single-branch\");\n+\t/*\n+\t * `url` might be NULL, so ensure that this argument is passed\n+\t * last to avoid confusing the next argument as the value for\n+\t * `--url`.\n+\t */\n+\tstrvec_pushl(&child->args, \"--url\", url, NULL);\n \n cleanup:\n \tfree(displaypath);\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex eae6a46ef3..c97c543dd8 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1351,6 +1351,22 @@ test_expect_success 'clone active submodule without submodule url set' '\n \t)\n '\n \n+test_expect_success 'update submodules without url set in .gitconfig' '\n+\ttest_when_finished \"rm -rf multisuper_clone\" &&\n+\tgit clone file://\"$pwd\"/multisuper multisuper_clone &&\n+\n+\tgit -C multisuper_clone submodule init &&\n+\tfor s in sub0 sub1 sub2 sub3\n+\tdo\n+\t\tkey=submodule.$s.url &&\n+\t\tgit -C multisuper_clone config --local --unset $key &&\n+\t\tgit -C multisuper_clone config --file .gitmodules --unset $key || return 1\n+\tdone &&\n+\n+\ttest_must_fail git -C multisuper_clone submodule update 2>err &&\n+\tgrep \"option .url. requires a value\" err\n+'\n+\n test_expect_success 'clone --recurse-submodules with a pathspec works' '\n \ttest_when_finished \"rm -rf multisuper_clone\" &&\n \tcat >expected <<-\\EOF &&\n-- \n2.41.0.rc1.11.gf7a8de14fe\n"},{"id":"477689","messageId":"bc204f450a80f7e6379b3a8564fc54637a266e7e.1684945504.git.me@ttaylorr.com","threadId":"59786","inReplyTo":"f7a8de14fe255286e62fc46d0a3083189f46bcc6.1684944140.git.me@ttaylorr.com","subject":"[PATCH v2] builtin/submodule--helper.c: handle missing submodule URLs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-24T16:25:25Z","receivedAt":"2023-05-24T16:25:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In e0a862fdaf (submodule helper: convert relative URL to absolute URL if\nneeded, 2018-10-16), `prepare_to_clone_next_submodule()` lost the\nability to handle URL-less submodules, due to a change from:\n\n    if (repo_get_config_string_const(the_repostiory, sb.buf, &url))\n        url = sub->url;\n\nto\n\n    if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) {\n        if (starts_with_dot_slash(sub->url) ||\n            starts_with_dot_dot_slash(sub->url)) {\n                /* ... */\n            }\n    }\n\n, which will segfault when `sub->url` is NULL, since both\n`starts_with_dot_slash()` does not guard its arguments as non-NULL.\n\nGuard the checks to both of the above functions by first whether\n`sub->url` is non-NULL. There is no need to check whether `sub` itself\nis NULL, since we already perform this check earlier in\n`prepare_to_clone_next_submodule()`.\n\nBy adding a NULL-ness check on `sub->url`, we'll fall into the 'else'\nbranch, setting `url` to `sub->url` (which is NULL). Before attempting\nto invoke `git submodule--helper clone`, check whether `url` is NULL,\nand die() if it is.\n\nReported-by: Tribo Dar <3bodar@gmail.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n\nJunio: feel free to queue either, but after thinking on v1 a little more\nI think that it is pretty gross and that this version is much cleaner.\n\nRange-diff against v1:\n1:  f7a8de14fe ! 1:  bc204f450a builtin/submodule--helper.c: handle missing submodule URLs\n    @@ Commit message\n         `prepare_to_clone_next_submodule()`.\n\n         By adding a NULL-ness check on `sub->url`, we'll fall into the 'else'\n    -    branch, setting `url` to `sub->url` (which is NULL). When we then try\n    -    and invoke `git submodule-helper` below, we'll get the expected error\n    -    message:\n    -\n    -        error: option `url' requires a value\n    -\n    -    Note that this means that `--url` and its value must come last when\n    -    preparing the argument list, to avoid confusing the next argument as the\n    -    value for `--url` if `url` itself is NULL.\n    +    branch, setting `url` to `sub->url` (which is NULL). Before attempting\n    +    to invoke `git submodule--helper clone`, check whether `url` is NULL,\n    +    and die() if it is.\n\n         Reported-by: Tribo Dar <3bodar@gmail.com>\n         Signed-off-by: Taylor Blau <me@ttaylorr.com>\n    @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st\n      \t\t\turl = resolve_relative_url(sub->url, NULL, 0);\n      \t\t\tneed_free_url = 1;\n      \t\t} else\n    + \t\t\turl = sub->url;\n    + \t}\n    +\n    ++\tif (!url)\n    ++\t\tdie(_(\"cannot clone submodule '%s' without a URL\"), sub->name);\n    ++\n    + \tstrbuf_reset(&sb);\n    + \tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n    + \tneeds_cloning = !file_exists(sb.buf);\n     @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n    + \tif (suc->update_data->filter_options && suc->update_data->filter_options->choice)\n    + \t\tstrvec_pushf(&child->args, \"--filter=%s\",\n    + \t\t\t     expand_list_objects_filter_spec(suc->update_data->filter_options));\n    ++\tstrvec_pushl(&child->args, \"--url\", url, NULL);\n    + \tif (suc->update_data->require_init)\n      \t\tstrvec_push(&child->args, \"--require-init\");\n      \tstrvec_pushl(&child->args, \"--path\", sub->path, NULL);\n      \tstrvec_pushl(&child->args, \"--name\", sub->name, NULL);\n    @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st\n      \tif (suc->update_data->references.nr) {\n      \t\tstruct string_list_item *item;\n\n    -@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n    - \t\tstrvec_push(&child->args, suc->update_data->single_branch ?\n    - \t\t\t\t\t      \"--single-branch\" :\n    - \t\t\t\t\t      \"--no-single-branch\");\n    -+\t/*\n    -+\t * `url` might be NULL, so ensure that this argument is passed\n    -+\t * last to avoid confusing the next argument as the value for\n    -+\t * `--url`.\n    -+\t */\n    -+\tstrvec_pushl(&child->args, \"--url\", url, NULL);\n    -\n    - cleanup:\n    - \tfree(displaypath);\n\n      ## t/t7400-submodule-basic.sh ##\n     @@ t/t7400-submodule-basic.sh: test_expect_success 'clone active submodule without submodule url set' '\n    @@ t/t7400-submodule-basic.sh: test_expect_success 'clone active submodule without\n     +\tdone &&\n     +\n     +\ttest_must_fail git -C multisuper_clone submodule update 2>err &&\n    -+\tgrep \"option .url. requires a value\" err\n    ++\tgrep \"cannot clone submodule .sub[0-3]. without a URL\" err\n     +'\n     +\n      test_expect_success 'clone --recurse-submodules with a pathspec works' '\n\n builtin/submodule--helper.c |  9 ++++++---\n t/t7400-submodule-basic.sh  | 16 ++++++++++++++++\n 2 files changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6bf8d666ce..efd0761cd0 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2024,14 +2024,17 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n \tif (repo_config_get_string_tmp(the_repository, sb.buf, &url)) {\n-\t\tif (starts_with_dot_slash(sub->url) ||\n-\t\t    starts_with_dot_dot_slash(sub->url)) {\n+\t\tif (sub->url && (starts_with_dot_slash(sub->url) ||\n+\t\t\t\t starts_with_dot_dot_slash(sub->url))) {\n \t\t\turl = resolve_relative_url(sub->url, NULL, 0);\n \t\t\tneed_free_url = 1;\n \t\t} else\n \t\t\turl = sub->url;\n \t}\n\n+\tif (!url)\n+\t\tdie(_(\"cannot clone submodule '%s' without a URL\"), sub->name);\n+\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n \tneeds_cloning = !file_exists(sb.buf);\n@@ -2065,11 +2068,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tif (suc->update_data->filter_options && suc->update_data->filter_options->choice)\n \t\tstrvec_pushf(&child->args, \"--filter=%s\",\n \t\t\t     expand_list_objects_filter_spec(suc->update_data->filter_options));\n+\tstrvec_pushl(&child->args, \"--url\", url, NULL);\n \tif (suc->update_data->require_init)\n \t\tstrvec_push(&child->args, \"--require-init\");\n \tstrvec_pushl(&child->args, \"--path\", sub->path, NULL);\n \tstrvec_pushl(&child->args, \"--name\", sub->name, NULL);\n-\tstrvec_pushl(&child->args, \"--url\", url, NULL);\n \tif (suc->update_data->references.nr) {\n \t\tstruct string_list_item *item;\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex eae6a46ef3..d9fbabb2b9 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1351,6 +1351,22 @@ test_expect_success 'clone active submodule without submodule url set' '\n \t)\n '\n\n+test_expect_success 'update submodules without url set in .gitconfig' '\n+\ttest_when_finished \"rm -rf multisuper_clone\" &&\n+\tgit clone file://\"$pwd\"/multisuper multisuper_clone &&\n+\n+\tgit -C multisuper_clone submodule init &&\n+\tfor s in sub0 sub1 sub2 sub3\n+\tdo\n+\t\tkey=submodule.$s.url &&\n+\t\tgit -C multisuper_clone config --local --unset $key &&\n+\t\tgit -C multisuper_clone config --file .gitmodules --unset $key || return 1\n+\tdone &&\n+\n+\ttest_must_fail git -C multisuper_clone submodule update 2>err &&\n+\tgrep \"cannot clone submodule .sub[0-3]. without a URL\" err\n+'\n+\n test_expect_success 'clone --recurse-submodules with a pathspec works' '\n \ttest_when_finished \"rm -rf multisuper_clone\" &&\n \tcat >expected <<-\\EOF &&\n--\n2.41.0.rc1.11.gf7a8de14fe.dirty\n"},{"id":"477691","messageId":"CAPig+cT4c4f8DtNJOmNJKRYVX0HNz9fSgkF9hAbzaCXV0ozydg@mail.gmail.com","threadId":"59786","inReplyTo":"bc204f450a80f7e6379b3a8564fc54637a266e7e.1684945504.git.me@ttaylorr.com","subject":"Re: [PATCH v2] builtin/submodule--helper.c: handle missing submodule URLs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-05-24T18:48:40Z","receivedAt":"2023-05-24T18:49:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 24, 2023 at 12:40 PM Taylor Blau <me@ttaylorr.com> wrote:\n> In e0a862fdaf (submodule helper: convert relative URL to absolute URL if\n> needed, 2018-10-16), `prepare_to_clone_next_submodule()` lost the\n> ability to handle URL-less submodules, due to a change from:\n>\n>     if (repo_get_config_string_const(the_repostiory, sb.buf, &url))\n>         url = sub->url;\n>\n> to\n>\n>     if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) {\n>         if (starts_with_dot_slash(sub->url) ||\n>             starts_with_dot_dot_slash(sub->url)) {\n>                 /* ... */\n>             }\n>     }\n>\n> , which will segfault when `sub->url` is NULL, since both\n> `starts_with_dot_slash()` does not guard its arguments as non-NULL.\n>\n> Guard the checks to both of the above functions by first whether\n\ns/first/first checking/\n\n> `sub->url` is non-NULL. There is no need to check whether `sub` itself\n> is NULL, since we already perform this check earlier in\n> `prepare_to_clone_next_submodule()`.\n>\n> By adding a NULL-ness check on `sub->url`, we'll fall into the 'else'\n> branch, setting `url` to `sub->url` (which is NULL). Before attempting\n> to invoke `git submodule--helper clone`, check whether `url` is NULL,\n> and die() if it is.\n>\n> Reported-by: Tribo Dar <3bodar@gmail.com>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> @@ -2024,14 +2024,17 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n> -               if (starts_with_dot_slash(sub->url) ||\n> -                   starts_with_dot_dot_slash(sub->url)) {\n> +               if (sub->url && (starts_with_dot_slash(sub->url) ||\n> +                                starts_with_dot_dot_slash(sub->url))) {\n>                         url = resolve_relative_url(sub->url, NULL, 0);\n>                         need_free_url = 1;\n>                 } else\n>                         url = sub->url;\n>         }\n>\n> +       if (!url)\n> +               die(_(\"cannot clone submodule '%s' without a URL\"), sub->name);\n\nGood. The first version of this patch was more difficult to reason\nabout due to its \"error-at-a-distance\" approach. This version is much\ncleaner and obvious.\n\n> @@ -2065,11 +2068,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n>                 strvec_pushf(&child->args, \"--filter=%s\",\n>                              expand_list_objects_filter_spec(suc->update_data->filter_options));\n> +       strvec_pushl(&child->args, \"--url\", url, NULL);\n>         if (suc->update_data->require_init)\n>                 strvec_push(&child->args, \"--require-init\");\n>         strvec_pushl(&child->args, \"--path\", sub->path, NULL);\n>         strvec_pushl(&child->args, \"--name\", sub->name, NULL);\n> -       strvec_pushl(&child->args, \"--url\", url, NULL);\n\nThis change is unnecessary now, isn't it? Or is there something\nnonobvious going on here?\n"},{"id":"477692","messageId":"ZG5qac9YVvjN/R3o@nand.local","threadId":"59786","inReplyTo":"CAPig+cT4c4f8DtNJOmNJKRYVX0HNz9fSgkF9hAbzaCXV0ozydg@mail.gmail.com","subject":"Re: [PATCH v2] builtin/submodule--helper.c: handle missing submodule URLs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-24T19:50:01Z","receivedAt":"2023-05-24T19:50:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 24, 2023 at 02:48:40PM -0400, Eric Sunshine wrote:\n> On Wed, May 24, 2023 at 12:40 PM Taylor Blau <me@ttaylorr.com> wrote:\n> > Guard the checks to both of the above functions by first whether\n>\n> s/first/first checking/\n\nOops. Good eyes, thanks for spotting.\n\n> > @@ -2065,11 +2068,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n> >                 strvec_pushf(&child->args, \"--filter=%s\",\n> >                              expand_list_objects_filter_spec(suc->update_data->filter_options));\n> > +       strvec_pushl(&child->args, \"--url\", url, NULL);\n> >         if (suc->update_data->require_init)\n> >                 strvec_push(&child->args, \"--require-init\");\n> >         strvec_pushl(&child->args, \"--path\", sub->path, NULL);\n> >         strvec_pushl(&child->args, \"--name\", sub->name, NULL);\n> > -       strvec_pushl(&child->args, \"--url\", url, NULL);\n>\n> This change is unnecessary now, isn't it? Or is there something\n> nonobvious going on here?\n\nYeah, this is a stray diff. I'll send a cleaned up version as soon as\n'make test' finishes ;-).\n\nThanks,\nTaylor\n"},{"id":"477693","messageId":"ae6cf3fa461b85e346f034371dae56a2790dfa20.1684957882.git.me@ttaylorr.com","threadId":"59786","inReplyTo":"f7a8de14fe255286e62fc46d0a3083189f46bcc6.1684944140.git.me@ttaylorr.com","subject":"[PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-24T19:51:43Z","receivedAt":"2023-05-24T19:51:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In e0a862fdaf (submodule helper: convert relative URL to absolute URL if\nneeded, 2018-10-16), `prepare_to_clone_next_submodule()` lost the\nability to handle URL-less submodules, due to a change from:\n\n    if (repo_get_config_string_const(the_repostiory, sb.buf, &url))\n        url = sub->url;\n\nto\n\n    if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) {\n        if (starts_with_dot_slash(sub->url) ||\n            starts_with_dot_dot_slash(sub->url)) {\n                /* ... */\n            }\n    }\n\n, which will segfault when `sub->url` is NULL, since both\n`starts_with_dot_slash()` does not guard its arguments as non-NULL.\n\nGuard the checks to both of the above functions by first checking\nwhether `sub->url` is non-NULL. There is no need to check whether `sub`\nitself is NULL, since we already perform this check earlier in\n`prepare_to_clone_next_submodule()`.\n\nBy adding a NULL-ness check on `sub->url`, we'll fall into the 'else'\nbranch, setting `url` to `sub->url` (which is NULL). Before attempting\nto invoke `git submodule--helper clone`, check whether `url` is NULL,\nand die() if it is.\n\nReported-by: Tribo Dar <3bodar@gmail.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nRange-diff against v2:\n1:  38202878b5 ! 1:  ae6cf3fa46 builtin/submodule--helper.c: handle missing submodule URLs\n    @@ Commit message\n         , which will segfault when `sub->url` is NULL, since both\n         `starts_with_dot_slash()` does not guard its arguments as non-NULL.\n     \n    -    Guard the checks to both of the above functions by first whether\n    -    `sub->url` is non-NULL. There is no need to check whether `sub` itself\n    -    is NULL, since we already perform this check earlier in\n    +    Guard the checks to both of the above functions by first checking\n    +    whether `sub->url` is non-NULL. There is no need to check whether `sub`\n    +    itself is NULL, since we already perform this check earlier in\n         `prepare_to_clone_next_submodule()`.\n     \n         By adding a NULL-ness check on `sub->url`, we'll fall into the 'else'\n    @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st\n      \tstrbuf_reset(&sb);\n      \tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n      \tneeds_cloning = !file_exists(sb.buf);\n    -@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n    - \tif (suc->update_data->filter_options && suc->update_data->filter_options->choice)\n    - \t\tstrvec_pushf(&child->args, \"--filter=%s\",\n    - \t\t\t     expand_list_objects_filter_spec(suc->update_data->filter_options));\n    -+\tstrvec_pushl(&child->args, \"--url\", url, NULL);\n    - \tif (suc->update_data->require_init)\n    - \t\tstrvec_push(&child->args, \"--require-init\");\n    - \tstrvec_pushl(&child->args, \"--path\", sub->path, NULL);\n    - \tstrvec_pushl(&child->args, \"--name\", sub->name, NULL);\n    --\tstrvec_pushl(&child->args, \"--url\", url, NULL);\n    - \tif (suc->update_data->references.nr) {\n    - \t\tstruct string_list_item *item;\n    - \n     \n      ## t/t7400-submodule-basic.sh ##\n     @@ t/t7400-submodule-basic.sh: test_expect_success 'clone active submodule without submodule url set' '\n\n builtin/submodule--helper.c |  7 +++++--\n t/t7400-submodule-basic.sh  | 16 ++++++++++++++++\n 2 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6bf8d666ce..6a16208e8a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2024,14 +2024,17 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n \tif (repo_config_get_string_tmp(the_repository, sb.buf, &url)) {\n-\t\tif (starts_with_dot_slash(sub->url) ||\n-\t\t    starts_with_dot_dot_slash(sub->url)) {\n+\t\tif (sub->url && (starts_with_dot_slash(sub->url) ||\n+\t\t\t\t starts_with_dot_dot_slash(sub->url))) {\n \t\t\turl = resolve_relative_url(sub->url, NULL, 0);\n \t\t\tneed_free_url = 1;\n \t\t} else\n \t\t\turl = sub->url;\n \t}\n \n+\tif (!url)\n+\t\tdie(_(\"cannot clone submodule '%s' without a URL\"), sub->name);\n+\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n \tneeds_cloning = !file_exists(sb.buf);\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex eae6a46ef3..d9fbabb2b9 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1351,6 +1351,22 @@ test_expect_success 'clone active submodule without submodule url set' '\n \t)\n '\n \n+test_expect_success 'update submodules without url set in .gitconfig' '\n+\ttest_when_finished \"rm -rf multisuper_clone\" &&\n+\tgit clone file://\"$pwd\"/multisuper multisuper_clone &&\n+\n+\tgit -C multisuper_clone submodule init &&\n+\tfor s in sub0 sub1 sub2 sub3\n+\tdo\n+\t\tkey=submodule.$s.url &&\n+\t\tgit -C multisuper_clone config --local --unset $key &&\n+\t\tgit -C multisuper_clone config --file .gitmodules --unset $key || return 1\n+\tdone &&\n+\n+\ttest_must_fail git -C multisuper_clone submodule update 2>err &&\n+\tgrep \"cannot clone submodule .sub[0-3]. without a URL\" err\n+'\n+\n test_expect_success 'clone --recurse-submodules with a pathspec works' '\n \ttest_when_finished \"rm -rf multisuper_clone\" &&\n \tcat >expected <<-\\EOF &&\n-- \n2.41.0.rc1.11.gae6cf3fa46\n"},{"id":"477695","messageId":"xmqqttw1fpd9.fsf@gitster.g","threadId":"59786","inReplyTo":"ZG4jcceXn2G6Xic0@nand.local","subject":"Re: [BUG] Segmentation fault in git v2.41.0.rc1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-24T20:25:54Z","receivedAt":"2023-05-24T20:26:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> which points to e0a862fdaf (submodule helper: convert relative URL to\n> absolute URL if needed, 2018-10-16) as the culprit.\n\nWhew, that is fairly ancient.  I was afraid if we have another\nregression, but it does not look that way.  Fixing is certainly good\nand we'd need to eventually get to it, but we have luxury to make\nsure that the fix is sound without having to rush anything ;-)\n\nIn the meantime, \"if it hurts, don't do it\" is what we can say.\nTelling random users to muck with their config in certain ways that\nviolate the way how the system represents (un-)initialized\nsubmodules and then to run certain command to induce a NULL pointer\ndereference is rather an ineffective social engineering as an attack\nvector, so this is not urgent in that sense.\n\nI am more worried that the original report talked about mucking with\nthe in-tree .gitmodules file affects the result, though.  Once a\nsubmodule is initialized, what is in the file for that submodule\nshould not affect the working of local Git (otherwise the file can\nbe used as a route to inject stuff to unsuspecting repositories),\nbut in this case apparently it does?\n\nThanks.\n"},{"id":"477696","messageId":"c8d32d5a-9803-9eb7-baaa-49b4521f0c37@web.de","threadId":"59786","inReplyTo":"ae6cf3fa461b85e346f034371dae56a2790dfa20.1684957882.git.me@ttaylorr.com","subject":"Re: [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-05-24T20:29:39Z","receivedAt":"2023-05-24T20:29:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.05.23 um 21:51 schrieb Taylor Blau:\n> There is no need to check whether `sub`\n> itself is NULL, since we already perform this check earlier in\n> `prepare_to_clone_next_submodule()`.\n\nRight, and if \"sub\" is NULL then next_submodule_warn_missing() is called\nand prepare_to_clone_next_submodule() is exited early.\n\n> By adding a NULL-ness check on `sub->url`, we'll fall into the 'else'\n> branch, setting `url` to `sub->url` (which is NULL). Before attempting\n> to invoke `git submodule--helper clone`, check whether `url` is NULL,\n> and die() if it is.\n\nWhy die() here instead of just warn and skip as well?\n\nRené\n\n"},{"id":"477697","messageId":"20230524203321.GD892557@coredump.intra.peff.net","threadId":"59786","inReplyTo":"ae6cf3fa461b85e346f034371dae56a2790dfa20.1684957882.git.me@ttaylorr.com","subject":"Re: [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-24T20:33:21Z","receivedAt":"2023-05-24T20:33:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 24, 2023 at 03:51:43PM -0400, Taylor Blau wrote:\n\n> In e0a862fdaf (submodule helper: convert relative URL to absolute URL if\n> needed, 2018-10-16), `prepare_to_clone_next_submodule()` lost the\n> ability to handle URL-less submodules, due to a change from:\n> \n>     if (repo_get_config_string_const(the_repostiory, sb.buf, &url))\n>         url = sub->url;\n> \n> to\n> \n>     if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) {\n>         if (starts_with_dot_slash(sub->url) ||\n>             starts_with_dot_dot_slash(sub->url)) {\n>                 /* ... */\n>             }\n>     }\n\nThis patch looks pretty good to me. I read your v1 and the word \"gross\"\nalso crossed my mind at the \"--url\" handling. This one is much better.\nI did have a few questions, though (below).\n\nIf I understand correctly, this is not at all new in the -rc releases,\nbut just something that happened to get unearthed? I.e., it can wait\nuntil post-release.\n\n> , which will segfault when `sub->url` is NULL, since both\n> `starts_with_dot_slash()` does not guard its arguments as non-NULL.\n\nFunny gramm-o, presumably from editing: \"both\" is plural, but \"does\" and\n\"its\" are singular. I think the gist of it is communicated, though.\n\n> Guard the checks to both of the above functions by first checking\n> whether `sub->url` is non-NULL. There is no need to check whether `sub`\n> itself is NULL, since we already perform this check earlier in\n> `prepare_to_clone_next_submodule()`.\n\nGood, thanks for checking (and communicating) that possible gotha.\n\n> By adding a NULL-ness check on `sub->url`, we'll fall into the 'else'\n> branch, setting `url` to `sub->url` (which is NULL). Before attempting\n> to invoke `git submodule--helper clone`, check whether `url` is NULL,\n> and die() if it is.\n\nIf I hadn't read v1, I might wonder whether this die() is consistent\nwith the existing behavior. But the point is that submodule--helper\nwould have barfed in such a case anyway, so we are just trading one\nerror for another.\n\nOne side effect, though, is that this die() will take down the whole\nsuperproject process. Whereas I think the intent of the submodule code\nis to keep going, handling other submodules, even if one fails. This\nisn't a failure exactly (more of a misconfiguration, if I understand\nit). But should we be somehow returning an error instead?\n\nI say \"somehow\" because it's not clear how to work that in with the\nneeds_cloning return value (obviously we can say \"0\", but that is the\nsame as the \"skipped\" code path; we presumably want to tell the caller\nthere was a failure, so it affects the ultimate return code).\n\n> +test_expect_success 'update submodules without url set in .gitconfig' '\n\nShould this be .gitmodules in the title?\n\n-Peff\n"},{"id":"477698","messageId":"ZG51XsEq4JB9IK33@nand.local","threadId":"59786","inReplyTo":"c8d32d5a-9803-9eb7-baaa-49b4521f0c37@web.de","subject":"Re: [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-24T20:36:46Z","receivedAt":"2023-05-24T20:36:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 24, 2023 at 10:29:39PM +0200, René Scharfe wrote:\n> Am 24.05.23 um 21:51 schrieb Taylor Blau:\n>\n> > By adding a NULL-ness check on `sub->url`, we'll fall into the 'else'\n> > branch, setting `url` to `sub->url` (which is NULL). Before attempting\n> > to invoke `git submodule--helper clone`, check whether `url` is NULL,\n> > and die() if it is.\n>\n> Why die() here instead of just warn and skip as well?\n\nThat's a good point. When I read prepare_to_clone_next_submodule(), I\nthought that it was already too late to skip that submodule. But my\nunderstanding was incorrect, we could easily issue a warning() and\nreturn '0', which would indicate to skip it.\n\nBut I concur with Junio earlier in the thread that we don't have to rush\nthings to get this into -rc2 or even 2.41, since this bug has been with\nus since v2.20.0.\n\nThanks,\nTaylor\n"},{"id":"477702","messageId":"20230524225807.GA904398@coredump.intra.peff.net","threadId":"59786","inReplyTo":"f7a8de14fe255286e62fc46d0a3083189f46bcc6.1684944140.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/submodule--helper.c: handle missing submodule URLs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-24T22:58:07Z","receivedAt":"2023-05-24T22:58:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 24, 2023 at 12:02:26PM -0400, Taylor Blau wrote:\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 6bf8d666ce..a7b7ea374f 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2024,8 +2024,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n>  \tstrbuf_reset(&sb);\n>  \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n>  \tif (repo_config_get_string_tmp(the_repository, sb.buf, &url)) {\n> -\t\tif (starts_with_dot_slash(sub->url) ||\n> -\t\t    starts_with_dot_dot_slash(sub->url)) {\n> +\t\tif (sub->url && (starts_with_dot_slash(sub->url) ||\n> +\t\t\t\t starts_with_dot_dot_slash(sub->url))) {\n>  \t\t\turl = resolve_relative_url(sub->url, NULL, 0);\n>  \t\t\tneed_free_url = 1;\n>  \t\t} else\n\nOh, btw, this need_free_url made me look at the memory handling for the\nother side of the else. I think it is all good, but it would be a little\nsimpler using an extra pointer instead of a boolean:\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6a16208e8a..6b7849b828 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1984,7 +1984,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\t\t\t\t\t      ud->super_prefix);\n \tstruct strbuf sb = STRBUF_INIT;\n \tint needs_cloning = 0;\n-\tint need_free_url = 0;\n+\tchar *to_free_url = NULL;\n \n \tif (ce_stage(ce)) {\n \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n@@ -2025,10 +2025,9 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n \tif (repo_config_get_string_tmp(the_repository, sb.buf, &url)) {\n \t\tif (sub->url && (starts_with_dot_slash(sub->url) ||\n-\t\t\t\t starts_with_dot_dot_slash(sub->url))) {\n-\t\t\turl = resolve_relative_url(sub->url, NULL, 0);\n-\t\t\tneed_free_url = 1;\n-\t\t} else\n+\t\t\t\t starts_with_dot_dot_slash(sub->url)))\n+\t\t\turl = to_free_url = resolve_relative_url(sub->url, NULL, 0);\n+\t\telse\n \t\t\turl = sub->url;\n \t}\n \n@@ -2089,8 +2088,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n cleanup:\n \tfree(displaypath);\n \tstrbuf_release(&sb);\n-\tif (need_free_url)\n-\t\tfree((void*)url);\n+\tfree(to_free_url);\n \n \treturn needs_cloning;\n }\n\nProbably at this point that is just churn, but I thought I'd throw it\nout there.\n\n-Peff\n"}]}