Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> Fix code added in ce125d431aa (submodule: extract path to submodule
> gitdir func, 2021-09-15) and a77c3fcb5ec (submodule--helper: get
> remote names from any repository, 2022-03-04) which failed to check
> the return values of repo_init() and repo_submodule_init(). If we
> failed to initialize the repository or submodule we could segfault
> when trying to access the invalid repository structs.
Yes, this sounds correct. repo_init() and repo_submodule_init() are pure
initialization and have no intended side effects, so there's no reason
to not check the return value.
>
> Let's also check that these were the only such logic errors in the
> codebase by making use of the "warn_unused_result" attribute. This is
> valid as of GCC 3.4.0 (and clang will catch it via its faking of
> __GNUC__ ).
>
> As the comment being added to git-compat-util.h we're piggy-backing on
> the LAST_ARG_MUST_BE_NULL version check out of lazyness. See
> 9fe3edc47f1 (Add the LAST_ARG_MUST_BE_NULL macro, 2013-07-18) for its
> addition. The marginal benefit of covering gcc 3.4.0..4.0.0 is
> near-zero (or zero) at this point. It mostly matters that we catch
> this somewhere.
I'm not familiar enough with attributes and the requistie compiler
versions, so I won't comment on this bit.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
> builtin/submodule--helper.c | 9 +++++++--
> git-compat-util.h | 3 +++
> repository.h | 3 +++
> 3 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 5f109d422ea..88fc01320f3 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -63,7 +63,10 @@ static char *get_default_remote_submodule(const char *module_path)
> {
> struct repository subrepo;
>
> - repo_submodule_init(&subrepo, the_repository, module_path, null_oid());
> + if (repo_submodule_init(&subrepo, the_repository, module_path,
> + null_oid()) < 0)
> + die(_("could not get a repository handle for submodule '%s'"),
> + module_path);
> return repo_get_default_remote(&subrepo);
> }
>
> @@ -1483,7 +1486,9 @@ static int add_possible_reference_from_superproject(
> struct strbuf err = STRBUF_INIT;
> strbuf_add(&sb, odb->path, len);
>
> - repo_init(&alternate, sb.buf, NULL);
> + if (repo_init(&alternate, sb.buf, NULL) < 0)
> + die(_("could not get a repository handle for gitdir '%s'"),
> + sb.buf);
>
> /*
> * We need to end the new path with '/' to mark it as a dir,
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 36a25ae252f..01d88650ba3 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -568,8 +568,11 @@ static inline int git_has_dir_sep(const char *path)
> /* The sentinel attribute is valid from gcc version 4.0 */
> #if defined(__GNUC__) && (__GNUC__ >= 4)
> #define LAST_ARG_MUST_BE_NULL __attribute__((sentinel))
> +/* warn_unused_result exists as of gcc 3.4.0, but be lazy and check 4.0 */
> +#define RESULT_MUST_BE_USED __attribute__ ((warn_unused_result))
> #else
> #define LAST_ARG_MUST_BE_NULL
> +#define RESULT_MUST_BE_USED
> #endif
>
> #define MAYBE_UNUSED __attribute__((__unused__))
> diff --git a/repository.h b/repository.h
> index 797f471cce9..24316ac944e 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -1,6 +1,7 @@
> #ifndef REPOSITORY_H
> #define REPOSITORY_H
>
> +#include "git-compat-util.h"
> #include "path.h"
>
> struct config_set;
> @@ -186,6 +187,7 @@ void repo_set_gitdir(struct repository *repo, const char *root,
> void repo_set_worktree(struct repository *repo, const char *path);
> void repo_set_hash_algo(struct repository *repo, int algo);
> void initialize_the_repository(void);
> +RESULT_MUST_BE_USED
> int repo_init(struct repository *r, const char *gitdir, const char *worktree);
>
> /*
> @@ -197,6 +199,7 @@ int repo_init(struct repository *r, const char *gitdir, const char *worktree);
> * Return 0 upon success and a non-zero value upon failure.
> */
> struct object_id;
> +RESULT_MUST_BE_USED
> int repo_submodule_init(struct repository *subrepo,
> struct repository *superproject,
> const char *path,
> --
> 2.37.2.1279.g64dec4e13cf
