Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> As noted in a preceding commit the get_default_remote_submodule() and
> remote_submodule_branch() functions would invoke die(), and thus leave
> update_submodule() only partially lib-ified. We've addressed the
> former of those in a preceding commit, let's now address the latter.
>
> In addition to lib-ifying the function this fixes a potential (but
> obscure) segfault introduced by a logic error in
> 1012a5cbc3f (submodule--helper run-update-procedure: learn --remote,
> 2022-03-04):
Ah, good catch.
>
> We were assuming that remote_submodule_branch() would always return
> on-NULL, but if the submodule_from_path() call in that function fails
s/on-NULL/non-NULL I assume?
> we'll return NULL. See its introduction in
> 92bbe7ccf1f (submodule--helper: add remote-branch helper, 2016-08-03).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
> builtin/submodule--helper.c | 41 ++++++++++++++++++++++---------------
> 1 file changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 9e4069b36cb..65909255760 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2267,42 +2267,49 @@ static int run_update_procedure(struct update_data *ud)
> return run_update_command(ud, subforce);
> }
>
> -static const char *remote_submodule_branch(const char *path)
> +static int remote_submodule_branch(const char *path, const char **branch)
> {
> const struct submodule *sub;
> - const char *branch = NULL;
> char *key;
> + *branch = NULL;
>
> sub = submodule_from_path(the_repository, null_oid(), path);
> if (!sub)
> - return NULL;
> + return die_message(_("could not initialize submodule at path '%s'"),
> + path);
>
> key = xstrfmt("submodule.%s.branch", sub->name);
> - if (repo_config_get_string_tmp(the_repository, key, &branch))
> - branch = sub->branch;
> + if (repo_config_get_string_tmp(the_repository, key, branch))
> + *branch = sub->branch;
> free(key);
>
> - if (!branch)
> - return "HEAD";
> + if (!*branch) {
> + *branch = "HEAD";
> + return 0;
> + }
>
> - if (!strcmp(branch, ".")) {
> + if (!strcmp(*branch, ".")) {
> const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
>
> if (!refname)
> - die(_("No such ref: %s"), "HEAD");
> + return die_message(_("No such ref: %s"), "HEAD");
>
> /* detached HEAD */
> if (!strcmp(refname, "HEAD"))
> - die(_("Submodule (%s) branch configured to inherit "
> - "branch from superproject, but the superproject "
> - "is not on any branch"), sub->name);
> + return die_message(_("Submodule (%s) branch configured to inherit "
> + "branch from superproject, but the superproject "
> + "is not on any branch"), sub->name);
>
> if (!skip_prefix(refname, "refs/heads/", &refname))
> - die(_("Expecting a full ref name, got %s"), refname);
> - return refname;
> + return die_message(_("Expecting a full ref name, got %s"),
> + refname);
> +
> + *branch = refname;
> + return 0;
> }
>
> - return branch;
> + /* Our "branch" is coming from repo_config_get_string_tmp() */
> + return 0;
> }
>
> static int ensure_core_worktree(const char *path)
> @@ -2437,7 +2444,9 @@ static int update_submodule(struct update_data *update_data)
> code = get_default_remote_submodule(update_data->sm_path, &remote_name);
> if (code)
> return code;
> - branch = remote_submodule_branch(update_data->sm_path);
> + code = remote_submodule_branch(update_data->sm_path, &branch);
> + if (code)
> + return code;
> remote_ref = xstrfmt("refs/remotes/%s/%s", remote_name, branch);
This line is the one that would segfault, I assume?
Looks good.
>
> if (!update_data->nofetch) {
> --
> 2.37.2.1279.g64dec4e13cf
