[BUG] Segmentation fault in git v2.41.0.rc1

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

Hi Tribo,

On Wed, May 24, 2023 at 06:59:55AM +0000, Tribo Dar wrote:

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

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 <me@ttaylorr.com>
---
 builtin/submodule--helper.c | 11 ++++++++---
 t/t7400-submodule-basic.sh  | 16 ++++++++++++++++
 2 files changed, 24 insertions(+), 3 deletions(-)

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 <me@ttaylorr.com>
---

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 <me@ttaylorr.com>
    @@ 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(-)

On Wed, May 24, 2023 at 12:40 PM Taylor Blau <me@ttaylorr.com> wrote:

s/first/first checking/

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.

This change is unnecessary now, isn't it? Or is there something nonobvious going on here?

On Wed, May 24, 2023 at 02:48:40PM -0400, Eric Sunshine wrote:

Oops. Good eyes, thanks for spotting.

Yeah, this is a stray diff. I'll send a cleaned up version as soon as 'make test' finishes ;-).

Thanks, Taylor

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 <me@ttaylorr.com>
---
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(-)

Am 24.05.23 um 21:51 schrieb Taylor Blau:

Right, and if "sub" is NULL then next_submodule_warn_missing() is called and prepare_to_clone_next_submodule() is exited early.

Why die() here instead of just warn and skip as well?

René

On Wed, May 24, 2023 at 10:29:39PM +0200, René Scharfe wrote:

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

On Wed, May 24, 2023 at 03:51:43PM -0400, Taylor Blau wrote:

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.

Funny gramm-o, presumably from editing: "both" is plural, but "does" and "its" are singular. I think the gist of it is communicated, though.

Good, thanks for checking (and communicating) that possible gotha.

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

Should this be .gitmodules in the title?

-Peff

On Wed, May 24, 2023 at 12:02:26PM -0400, Taylor Blau wrote:

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:

Taylor Blau <me@ttaylorr.com> writes:

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.