diff mbox series

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

Message ID patch-07.20-e5a0134d1bb-20221228T175512Z-avarab@gmail.com (mailing list archive)
State Superseded
Headers show
Series leak fixes: various simple leak fixes | expand

Commit Message

Ævar Arnfjörð Bjarmason Dec. 28, 2022, 6 p.m. UTC
Call clear_pathspec() on the pathspec initialized in
push_stash(). 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 };

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/stash.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

René Scharfe Dec. 28, 2022, 7:45 p.m. UTC | #1
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.

> 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));

>
> 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)
Junio C Hamano Dec. 29, 2022, 7:02 a.m. UTC | #2
René Scharfe <l.s.r@web.de> writes:

> 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.

It did mislead me too.  I expected that addition of "= { 0 }" was to
remove memset('\0') somewhere else, but that is not the case.
diff mbox series

Patch

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)