Re: [PATCH v3 29/32] submodule--helper: check repo{_submodule,}_init() return values

[Glen Choo <chooglen@google.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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@gmail.com>
> ---
>  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