* [BUG] Segmentation fault in git v2.41.0.rc1
@ 2023-05-24 6:59 Tribo Dar
2023-05-24 14:47 ` Taylor Blau
0 siblings, 1 reply; 12+ messages in thread
From: Tribo Dar @ 2023-05-24 6:59 UTC (permalink / raw)
To: git
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [BUG] Segmentation fault in git v2.41.0.rc1
2023-05-24 6:59 [BUG] Segmentation fault in git v2.41.0.rc1 Tribo Dar
@ 2023-05-24 14:47 ` Taylor Blau
2023-05-24 16:02 ` [PATCH] builtin/submodule--helper.c: handle missing submodule URLs Taylor Blau
2023-05-24 20:25 ` [BUG] Segmentation fault in git v2.41.0.rc1 Junio C Hamano
0 siblings, 2 replies; 12+ messages in thread
From: Taylor Blau @ 2023-05-24 14:47 UTC (permalink / raw)
To: Tribo Dar; +Cc: git
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 14:47 ` Taylor Blau
@ 2023-05-24 16:02 ` Taylor Blau
2023-05-24 16:25 ` [PATCH v2] " Taylor Blau
` (2 more replies)
2023-05-24 20:25 ` [BUG] Segmentation fault in git v2.41.0.rc1 Junio C Hamano
1 sibling, 3 replies; 12+ messages in thread
From: Taylor Blau @ 2023-05-24 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Tribo Dar, Stefan Beller
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(-)
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 16:02 ` [PATCH] builtin/submodule--helper.c: handle missing submodule URLs Taylor Blau
@ 2023-05-24 16:25 ` Taylor Blau
2023-05-24 18:48 ` Eric Sunshine
2023-05-24 19:51 ` [PATCH v3] " Taylor Blau
2023-05-24 22:58 ` [PATCH] " Jeff King
2 siblings, 1 reply; 12+ messages in thread
From: Taylor Blau @ 2023-05-24 16:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Tribo Dar, Stefan Beller
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(-)
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 16:25 ` [PATCH v2] " Taylor Blau
@ 2023-05-24 18:48 ` Eric Sunshine
2023-05-24 19:50 ` Taylor Blau
0 siblings, 1 reply; 12+ messages in thread
From: Eric Sunshine @ 2023-05-24 18:48 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Tribo Dar, Stefan Beller
On Wed, May 24, 2023 at 12:40 PM Taylor Blau <me@ttaylorr.com> 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 <me@ttaylorr.com>
> ---
> 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?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 18:48 ` Eric Sunshine
@ 2023-05-24 19:50 ` Taylor Blau
0 siblings, 0 replies; 12+ messages in thread
From: Taylor Blau @ 2023-05-24 19:50 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Junio C Hamano, Tribo Dar, Stefan Beller
On Wed, May 24, 2023 at 02:48:40PM -0400, Eric Sunshine wrote:
> On Wed, May 24, 2023 at 12:40 PM Taylor Blau <me@ttaylorr.com> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 16:02 ` [PATCH] builtin/submodule--helper.c: handle missing submodule URLs Taylor Blau
2023-05-24 16:25 ` [PATCH v2] " Taylor Blau
@ 2023-05-24 19:51 ` Taylor Blau
2023-05-24 20:29 ` René Scharfe
2023-05-24 20:33 ` Jeff King
2023-05-24 22:58 ` [PATCH] " Jeff King
2 siblings, 2 replies; 12+ messages in thread
From: Taylor Blau @ 2023-05-24 19:51 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Tribo Dar, Stefan Beller, Eric Sunshine
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(-)
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [BUG] Segmentation fault in git v2.41.0.rc1
2023-05-24 14:47 ` Taylor Blau
2023-05-24 16:02 ` [PATCH] builtin/submodule--helper.c: handle missing submodule URLs Taylor Blau
@ 2023-05-24 20:25 ` Junio C Hamano
1 sibling, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2023-05-24 20:25 UTC (permalink / raw)
To: Taylor Blau; +Cc: Tribo Dar, git
Taylor Blau <me@ttaylorr.com> 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 19:51 ` [PATCH v3] " Taylor Blau
@ 2023-05-24 20:29 ` René Scharfe
2023-05-24 20:36 ` Taylor Blau
2023-05-24 20:33 ` Jeff King
1 sibling, 1 reply; 12+ messages in thread
From: René Scharfe @ 2023-05-24 20:29 UTC (permalink / raw)
To: Taylor Blau, git; +Cc: Junio C Hamano, Tribo Dar, Stefan Beller, Eric Sunshine
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é
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 19:51 ` [PATCH v3] " Taylor Blau
2023-05-24 20:29 ` René Scharfe
@ 2023-05-24 20:33 ` Jeff King
1 sibling, 0 replies; 12+ messages in thread
From: Jeff King @ 2023-05-24 20:33 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Tribo Dar, Stefan Beller, Eric Sunshine
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 20:29 ` René Scharfe
@ 2023-05-24 20:36 ` Taylor Blau
0 siblings, 0 replies; 12+ messages in thread
From: Taylor Blau @ 2023-05-24 20:36 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Tribo Dar, Stefan Beller, Eric Sunshine
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 16:02 ` [PATCH] builtin/submodule--helper.c: handle missing submodule URLs Taylor Blau
2023-05-24 16:25 ` [PATCH v2] " Taylor Blau
2023-05-24 19:51 ` [PATCH v3] " Taylor Blau
@ 2023-05-24 22:58 ` Jeff King
2 siblings, 0 replies; 12+ messages in thread
From: Jeff King @ 2023-05-24 22:58 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Tribo Dar, Stefan Beller
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-05-24 22:58 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-24 6:59 [BUG] Segmentation fault in git v2.41.0.rc1 Tribo Dar
2023-05-24 14:47 ` Taylor Blau
2023-05-24 16:02 ` [PATCH] builtin/submodule--helper.c: handle missing submodule URLs Taylor Blau
2023-05-24 16:25 ` [PATCH v2] " Taylor Blau
2023-05-24 18:48 ` Eric Sunshine
2023-05-24 19:50 ` Taylor Blau
2023-05-24 19:51 ` [PATCH v3] " Taylor Blau
2023-05-24 20:29 ` René Scharfe
2023-05-24 20:36 ` Taylor Blau
2023-05-24 20:33 ` Jeff King
2023-05-24 22:58 ` [PATCH] " Jeff King
2023-05-24 20:25 ` [BUG] Segmentation fault in git v2.41.0.rc1 Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).