Re: [PATCH 07/20] stash: fix a "struct pathspec" leak

From: René Scharfe <l.s.r@web.de>
Date: Dec 28, 2022, 19:45 UTC
Message-ID: <ac9621a0-1046-30de-872f-0171412049bd@web.de>
In-Reply-To: <patch-07.20-e5a0134d1bb-20221228T175512Z-avarab@gmail.com>

Am 28.12.22 um 19:00 schrieb Ævar Arnfjörð Bjarmason:

> Call clear_pathspec() on the pathspec initialized in
> push_stash().

This puzzled me for a while. This patch adds an {0} initializer to the declaration of the pathspec. I assumed that this is necessary to avoid giving clear_pathspec() an uninitialized struct. It isn't, though, because the pathspec is handed to parse_pathspec() first, which initializes it. So you can safely drop the first hunk.

Show 9 quoted lines

> Initializing that structure in this way is already done
> by a few other callers, and now we have:
>
> 	$ git grep -e 'struct pathspec.* = { 0 }' -e memset.pathspec
> 	add-interactive.c:              struct pathspec ps_selected = { 0 };
> 	builtin/sparse-checkout.c:              struct pathspec p = { 0 };
> 	builtin/stash.c:        struct pathspec ps = { 0 };
> 	pathspec.c:     memset(pathspec, 0, sizeof(*pathspec));
> 	wt-status.c:                    struct pathspec ps = { 0 };

Not sure if this part of the commit message is useful. It seems to suggest that the only place to initialize a pathspec with memset is pathspec.c itself, but there are a few more. Here's a really sloppy way to find (some of?) them:

   $ git grep -e 'struct pathspec [^*]' -e memset --all-match -p -n | awk '
      /struct pathspec [^*]/ {
         decl=$0
         declfunc=prevfunc
         var=decl; sub(/^.* /, "", var); sub(/;/, "", var)
         next
      }
      /memset/ && declfunc==prevfunc && match($0, var) {
         print decl
         print
         next
      }
      {prevfunc=$0}'
   builtin/log.c:726:	struct pathspec match_all;
   builtin/log.c:737:	memset(&match_all, 0, sizeof(match_all));
   builtin/ls-files.c:572:	struct pathspec pathspec;
   builtin/ls-files.c:600:		memset(&pathspec, 0, sizeof(pathspec));
   builtin/stash.c:1469:	struct pathspec ps;
   builtin/stash.c:1474:	memset(&ps, 0, sizeof(ps));
   builtin/stash.c:1787:	struct pathspec ps;
   builtin/stash.c:1813:	memset(&ps, 0, sizeof(ps));
   merge-recursive.c:479:	struct pathspec match_all;
   merge-recursive.c:480:	memset(&match_all, 0, sizeof(match_all));
   sparse-index.c:364:		struct pathspec ps;
   sparse-index.c:388:		memset(&ps, 0, sizeof(ps));

Show 40 quoted lines

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/stash.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index bb0fd861434..e82fb69c2b3 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1708,7 +1708,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
>  	int pathspec_file_nul = 0;
>  	const char *stash_msg = NULL;
>  	const char *pathspec_from_file = NULL;
> -	struct pathspec ps;
> +	struct pathspec ps = { 0 };
>  	struct option options[] = {
>  		OPT_BOOL('k', "keep-index", &keep_index,
>  			 N_("keep index")),
> @@ -1727,6 +1727,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
>  		OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),
>  		OPT_END()
>  	};
> +	int ret;
>
>  	if (argc) {
>  		force_assume = !strcmp(argv[0], "-p");
> @@ -1766,8 +1767,10 @@ static int push_stash(int argc, const char **argv, const char *prefix,
>  		die(_("the option '%s' requires '%s'"), "--pathspec-file-nul", "--pathspec-from-file");
>  	}
>
> -	return do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode,
> -			     include_untracked, only_staged);
> +	ret = do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode,
> +			    include_untracked, only_staged);
> +	clear_pathspec(&ps);
> +	return ret;
>  }
>
>  static int push_stash_unassumed(int argc, const char **argv, const char *prefix)