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