| Message ID | patch-1.1-9b17170b794-20211014T000949Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | checkout: fix "branch info" memory leaks | expand |
Hi Ævar On 14/10/2021 01:10, Ævar Arnfjörð Bjarmason wrote: > The "checkout" command is one of the main sources of leaks in the test > suite, let's fix the common ones by not leaking from the "struct > branch_info". > > Doing this is rather straightforward, albeit verbose, we need to > xstrdup() constant strings going into the struct, and free() the ones > we clobber as we go along. It's great to see these leaks being fixed. I wonder though if it would be better to change the structure definition so that 'name' and 'path' are no longer 'const'. That would be a better reflection of the new regime. It would also mean we could lose all the casts when freeing and there would be a compiler warning if a string literal is assigned to one of those fields. Best Wishes Phillip > This also means that we can delete previous partial leak fixes in this > area, i.e. the "path_to_free" accounting added by 96ec7b1e708 (Convert > resolve_ref+xstrdup to new resolve_refdup function, 2011-12-13). > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > > As with other leak fixes I merged this to "seen" and tested it in > combination with in-flight topics under > GIT_TEST_PASSING_SANITIZE_LEAK=true. > > builtin/checkout.c | 76 +++++++++++++++++++++---------- > t/t1005-read-tree-reset.sh | 1 + > t/t1406-submodule-ref-store.sh | 1 + > t/t2008-checkout-subdir.sh | 1 + > t/t2014-checkout-switch.sh | 2 + > t/t2026-checkout-pathspec-file.sh | 1 + > t/t9102-git-svn-deep-rmdir.sh | 2 + > 7 files changed, 59 insertions(+), 25 deletions(-) > > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 8c69dcdf72a..a85eb66da16 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -103,6 +103,16 @@ struct branch_info { > char *checkout; > }; > > +static void branch_info_release(struct branch_info *info) > +{ > + if (!info) > + return; > + free((char *)info->name); > + free((char *)info->path); > + free(info->refname); > + free(info->checkout); > +} > + > static int post_checkout_hook(struct commit *old_commit, struct commit *new_commit, > int changed) > { > @@ -686,8 +696,10 @@ static void setup_branch_path(struct branch_info *branch) > repo_get_oid_committish(the_repository, branch->name, &branch->oid); > > strbuf_branchname(&buf, branch->name, INTERPRET_BRANCH_LOCAL); > - if (strcmp(buf.buf, branch->name)) > + if (strcmp(buf.buf, branch->name)) { > + free((char *)branch->name); > branch->name = xstrdup(buf.buf); > + } > strbuf_splice(&buf, 0, 0, "refs/heads/", 11); > branch->path = strbuf_detach(&buf, NULL); > } > @@ -896,7 +908,9 @@ static void update_refs_for_switch(const struct checkout_opts *opts, > opts->new_branch_log, > opts->quiet, > opts->track); > - new_branch_info->name = opts->new_branch; > + free((char *)new_branch_info->name); > + free(new_branch_info->refname); > + new_branch_info->name = xstrdup(opts->new_branch); > setup_branch_path(new_branch_info); > } > > @@ -1064,8 +1078,7 @@ static int switch_branches(const struct checkout_opts *opts, > struct branch_info *new_branch_info) > { > int ret = 0; > - struct branch_info old_branch_info; > - void *path_to_free; > + struct branch_info old_branch_info = { 0 }; > struct object_id rev; > int flag, writeout_error = 0; > int do_merge = 1; > @@ -1073,25 +1086,32 @@ static int switch_branches(const struct checkout_opts *opts, > trace2_cmd_mode("branch"); > > memset(&old_branch_info, 0, sizeof(old_branch_info)); > - old_branch_info.path = path_to_free = resolve_refdup("HEAD", 0, &rev, &flag); > + old_branch_info.path = resolve_refdup("HEAD", 0, &rev, &flag); > if (old_branch_info.path) > old_branch_info.commit = lookup_commit_reference_gently(the_repository, &rev, 1); > - if (!(flag & REF_ISSYMREF)) > + if (!(flag & REF_ISSYMREF)) { > + free((char *)old_branch_info.path); > old_branch_info.path = NULL; > + } > > - if (old_branch_info.path) > - skip_prefix(old_branch_info.path, "refs/heads/", &old_branch_info.name); > + if (old_branch_info.path) { > + const char *p; > + if (skip_prefix(old_branch_info.path, "refs/heads/", &p)) > + old_branch_info.name = xstrdup(p); > + else > + BUG("Should be able to skip with %s!", old_branch_info.path); > + } > > if (opts->new_orphan_branch && opts->orphan_from_empty_tree) { > if (new_branch_info->name) > BUG("'switch --orphan' should never accept a commit as starting point"); > new_branch_info->commit = NULL; > - new_branch_info->name = "(empty)"; > + new_branch_info->name = xstrdup("(empty)"); > do_merge = 1; > } > > if (!new_branch_info->name) { > - new_branch_info->name = "HEAD"; > + new_branch_info->name = xstrdup("HEAD"); > new_branch_info->commit = old_branch_info.commit; > if (!new_branch_info->commit) > die(_("You are on a branch yet to be born")); > @@ -1104,7 +1124,7 @@ static int switch_branches(const struct checkout_opts *opts, > if (do_merge) { > ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error); > if (ret) { > - free(path_to_free); > + branch_info_release(&old_branch_info); > return ret; > } > } > @@ -1115,7 +1135,8 @@ static int switch_branches(const struct checkout_opts *opts, > update_refs_for_switch(opts, &old_branch_info, new_branch_info); > > ret = post_checkout_hook(old_branch_info.commit, new_branch_info->commit, 1); > - free(path_to_free); > + branch_info_release(&old_branch_info); > + > return ret || writeout_error; > } > > @@ -1147,7 +1168,7 @@ static void setup_new_branch_info_and_source_tree( > struct tree **source_tree = &opts->source_tree; > struct object_id branch_rev; > > - new_branch_info->name = arg; > + new_branch_info->name = xstrdup(arg); > setup_branch_path(new_branch_info); > > if (!check_refname_format(new_branch_info->path, 0) && > @@ -1576,12 +1597,11 @@ static char cb_option = 'b'; > > static int checkout_main(int argc, const char **argv, const char *prefix, > struct checkout_opts *opts, struct option *options, > - const char * const usagestr[]) > + const char * const usagestr[], > + struct branch_info *new_branch_info) > { > - struct branch_info new_branch_info; > int parseopt_flags = 0; > > - memset(&new_branch_info, 0, sizeof(new_branch_info)); > opts->overwrite_ignore = 1; > opts->prefix = prefix; > opts->show_progress = -1; > @@ -1690,7 +1710,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, > opts->track == BRANCH_TRACK_UNSPECIFIED && > !opts->new_branch; > int n = parse_branchname_arg(argc, argv, dwim_ok, > - &new_branch_info, opts, &rev); > + new_branch_info, opts, &rev); > argv += n; > argc -= n; > } else if (!opts->accept_ref && opts->from_treeish) { > @@ -1699,7 +1719,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, > if (get_oid_mb(opts->from_treeish, &rev)) > die(_("could not resolve %s"), opts->from_treeish); > > - setup_new_branch_info_and_source_tree(&new_branch_info, > + setup_new_branch_info_and_source_tree(new_branch_info, > opts, &rev, > opts->from_treeish); > > @@ -1719,7 +1739,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, > * Try to give more helpful suggestion. > * new_branch && argc > 1 will be caught later. > */ > - if (opts->new_branch && argc == 1 && !new_branch_info.commit) > + if (opts->new_branch && argc == 1 && !new_branch_info->commit) > die(_("'%s' is not a commit and a branch '%s' cannot be created from it"), > argv[0], opts->new_branch); > > @@ -1768,11 +1788,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix, > strbuf_release(&buf); > } > > - UNLEAK(opts); > if (opts->patch_mode || opts->pathspec.nr) > - return checkout_paths(opts, &new_branch_info); > + return checkout_paths(opts, new_branch_info); > else > - return checkout_branch(opts, &new_branch_info); > + return checkout_branch(opts, new_branch_info); > } > > int cmd_checkout(int argc, const char **argv, const char *prefix) > @@ -1791,6 +1810,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) > OPT_END() > }; > int ret; > + struct branch_info new_branch_info = { 0 }; > > memset(&opts, 0, sizeof(opts)); > opts.dwim_new_local_branch = 1; > @@ -1821,7 +1841,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) > options = add_checkout_path_options(&opts, options); > > ret = checkout_main(argc, argv, prefix, &opts, > - options, checkout_usage); > + options, checkout_usage, &new_branch_info); > + branch_info_release(&new_branch_info); > + clear_pathspec(&opts.pathspec); > FREE_AND_NULL(options); > return ret; > } > @@ -1842,6 +1864,7 @@ int cmd_switch(int argc, const char **argv, const char *prefix) > OPT_END() > }; > int ret; > + struct branch_info new_branch_info = { 0 }; > > memset(&opts, 0, sizeof(opts)); > opts.dwim_new_local_branch = 1; > @@ -1861,7 +1884,8 @@ int cmd_switch(int argc, const char **argv, const char *prefix) > cb_option = 'c'; > > ret = checkout_main(argc, argv, prefix, &opts, > - options, switch_branch_usage); > + options, switch_branch_usage, &new_branch_info); > + branch_info_release(&new_branch_info); > FREE_AND_NULL(options); > return ret; > } > @@ -1883,6 +1907,7 @@ int cmd_restore(int argc, const char **argv, const char *prefix) > OPT_END() > }; > int ret; > + struct branch_info new_branch_info = { 0 }; > > memset(&opts, 0, sizeof(opts)); > opts.accept_ref = 0; > @@ -1898,7 +1923,8 @@ int cmd_restore(int argc, const char **argv, const char *prefix) > options = add_checkout_path_options(&opts, options); > > ret = checkout_main(argc, argv, prefix, &opts, > - options, restore_usage); > + options, restore_usage, &new_branch_info); > + branch_info_release(&new_branch_info); > FREE_AND_NULL(options); > return ret; > } > diff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh > index 83b09e13106..12e30d77d09 100755 > --- a/t/t1005-read-tree-reset.sh > +++ b/t/t1005-read-tree-reset.sh > @@ -2,6 +2,7 @@ > > test_description='read-tree -u --reset' > > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > . "$TEST_DIRECTORY"/lib-read-tree.sh > > diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh > index 0a87058971e..3c19edcf30b 100755 > --- a/t/t1406-submodule-ref-store.sh > +++ b/t/t1406-submodule-ref-store.sh > @@ -5,6 +5,7 @@ test_description='test submodule ref store api' > GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > RUN="test-tool ref-store submodule:sub" > diff --git a/t/t2008-checkout-subdir.sh b/t/t2008-checkout-subdir.sh > index eadb9434ae7..8a518a44ea2 100755 > --- a/t/t2008-checkout-subdir.sh > +++ b/t/t2008-checkout-subdir.sh > @@ -4,6 +4,7 @@ > > test_description='git checkout from subdirectories' > > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > test_expect_success setup ' > diff --git a/t/t2014-checkout-switch.sh b/t/t2014-checkout-switch.sh > index ccfb1471135..c138bdde4fe 100755 > --- a/t/t2014-checkout-switch.sh > +++ b/t/t2014-checkout-switch.sh > @@ -1,6 +1,8 @@ > #!/bin/sh > > test_description='Peter MacMillan' > + > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > test_expect_success setup ' > diff --git a/t/t2026-checkout-pathspec-file.sh b/t/t2026-checkout-pathspec-file.sh > index 43d31d79485..9db11f86dd6 100755 > --- a/t/t2026-checkout-pathspec-file.sh > +++ b/t/t2026-checkout-pathspec-file.sh > @@ -2,6 +2,7 @@ > > test_description='checkout --pathspec-from-file' > > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > test_tick > diff --git a/t/t9102-git-svn-deep-rmdir.sh b/t/t9102-git-svn-deep-rmdir.sh > index 66cd51102c8..7b2049caa0c 100755 > --- a/t/t9102-git-svn-deep-rmdir.sh > +++ b/t/t9102-git-svn-deep-rmdir.sh > @@ -1,5 +1,7 @@ > #!/bin/sh > test_description='git svn rmdir' > + > +TEST_PASSES_SANITIZE_LEAK=true > . ./lib-git-svn.sh > > test_expect_success 'initialize repo' ' >
On Thu, Oct 14 2021, Phillip Wood wrote: [Changed $subject] > On 14/10/2021 01:10, Ævar Arnfjörð Bjarmason wrote: >> The "checkout" command is one of the main sources of leaks in the test >> suite, let's fix the common ones by not leaking from the "struct >> branch_info". >> Doing this is rather straightforward, albeit verbose, we need to >> xstrdup() constant strings going into the struct, and free() the ones >> we clobber as we go along. > > It's great to see these leaks being fixed. I wonder though if it would > be better to change the structure definition so that 'name' and 'path' > are no longer 'const'. That would be a better reflection of the new > regime.[...] I think this is the right thing to do, but I'm not quite sure. There was a thread at it here: https://lore.kernel.org/git/YUZG0D5ayEWd7MLP@carlos-mbp.lan/ Where I chimed in and suggested exactly what you're saying here, but the consensus seemed to go the other way, and if you grep: git grep -F 'free((char *)' You can see that we use this pattern pretty widely. > It would also mean we could lose all the casts when freeing > and there would be a compiler warning if a string literal is assigned > to one of those fields. What compiler/set of warnings gives you a warning when you do that? I don't get warned on e.g.: diff --git a/builtin/checkout.c b/builtin/checkout.c index a32af16d5e4..d7053579bdf 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -94 +94 @@ struct branch_info { - const char *name; /* The short name used */ + char *name; /* The short name used */ @@ -110 +110 @@ static void branch_info_release(struct branch_info *info) - free((char *)info->name); + free(info->name); @@ -1107 +1107 @@ static int switch_branches(const struct checkout_opts *opts, - new_branch_info->name = xstrdup("(empty)"); + new_branch_info->name = "(empty)"; Now, what is really useful is making it a "char * const", especially when hacking up these changes as you'll find all the assignments, but I haven't found the general use in having that make it to a submitted patch, since you need to assign somewhere, and those then need to be a str[n]cpy() (except we banned.h it) or memcpy() with a cast...
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> It's great to see these leaks being fixed. I wonder though if it would >> be better to change the structure definition so that 'name' and 'path' >> are no longer 'const'. That would be a better reflection of the new >> regime.[...] > > I think this is the right thing to do, but I'm not quite sure. There was > a thread at it here: > > https://lore.kernel.org/git/YUZG0D5ayEWd7MLP@carlos-mbp.lan/ > > Where I chimed in and suggested exactly what you're saying here, but the > consensus seemed to go the other way, and if you grep: > > git grep -F 'free((char *)' > > You can see that we use this pattern pretty widely. Unfortunately, we probably need to make a trade-off and cannot eat the cake and have it at the same time. If we leave the .members non-const, the destructor may have to cast the constness away. If it is marked const * const, then we also need to let the constructor do the same. By marking the .members const, we can be sure that the users of the API will not muck with the values once the structure is instanciated and given to them, but the destructor need to cast the constness away. It may be lessor of two evils, as the need to cast is isolated in the _implementation_ of the API, and casts in the _users_ of the API would stand out more.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Thu, Oct 14 2021, Phillip Wood wrote: > > [Changed $subject] Thanks, I might not've noticed this if you hadn't. > > On 14/10/2021 01:10, Ævar Arnfjörð Bjarmason wrote: > >> The "checkout" command is one of the main sources of leaks in the test > >> suite, let's fix the common ones by not leaking from the "struct > >> branch_info". > >> Doing this is rather straightforward, albeit verbose, we need to > >> xstrdup() constant strings going into the struct, and free() the ones > >> we clobber as we go along. > > > > It's great to see these leaks being fixed. I wonder though if it would > > be better to change the structure definition so that 'name' and 'path' > > are no longer 'const'. That would be a better reflection of the new > > regime.[...] > > I think this is the right thing to do, but I'm not quite sure. There was > a thread at it here: > > https://lore.kernel.org/git/YUZG0D5ayEWd7MLP@carlos-mbp.lan/ I'd much prefer we keep const-ness for safety and documentation purposes. > Where I chimed in and suggested exactly what you're saying here, but the > consensus seemed to go the other way, and if you grep: > > git grep -F 'free((char *)' > > You can see that we use this pattern pretty widely. I've been using unions to workaround APIs like free(3) for many years: static inline void deconst_free(const void *ptr) { /* this initializer is a C99-ism */ union { const void *in; void *out; } deconst = { .in = ptr }; free(deconst.out); }
Hi Ævar On 14/10/2021 20:54, Ævar Arnfjörð Bjarmason wrote: > > On Thu, Oct 14 2021, Phillip Wood wrote: > > [Changed $subject] > >> On 14/10/2021 01:10, Ævar Arnfjörð Bjarmason wrote: >>> The "checkout" command is one of the main sources of leaks in the test >>> suite, let's fix the common ones by not leaking from the "struct >>> branch_info". >>> Doing this is rather straightforward, albeit verbose, we need to >>> xstrdup() constant strings going into the struct, and free() the ones >>> we clobber as we go along. >> >> It's great to see these leaks being fixed. I wonder though if it would >> be better to change the structure definition so that 'name' and 'path' >> are no longer 'const'. That would be a better reflection of the new >> regime.[...] > > I think this is the right thing to do, but I'm not quite sure. There was > a thread at it here: > > https://lore.kernel.org/git/YUZG0D5ayEWd7MLP@carlos-mbp.lan/ > > Where I chimed in and suggested exactly what you're saying here, but the > consensus seemed to go the other way, and if you grep: > > git grep -F 'free((char *)' > > You can see that we use this pattern pretty widely. > >> It would also mean we could lose all the casts when freeing >> and there would be a compiler warning if a string literal is assigned >> to one of those fields. > > What compiler/set of warnings gives you a warning when you do that? I > don't get warned on e.g.: Oh, I think I was thinking of -Wwrite-strings but we don't have that warning on and turning it on causes a bunch of -Wdiscarded-qualifier warnings. Best Wishes Phillip > diff --git a/builtin/checkout.c b/builtin/checkout.c > index a32af16d5e4..d7053579bdf 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -94 +94 @@ struct branch_info { > - const char *name; /* The short name used */ > + char *name; /* The short name used */ > @@ -110 +110 @@ static void branch_info_release(struct branch_info *info) > - free((char *)info->name); > + free(info->name); > @@ -1107 +1107 @@ static int switch_branches(const struct checkout_opts *opts, > - new_branch_info->name = xstrdup("(empty)"); > + new_branch_info->name = "(empty)"; > > Now, what is really useful is making it a "char * const", especially > when hacking up these changes as you'll find all the assignments, but I > haven't found the general use in having that make it to a submitted > patch, since you need to assign somewhere, and those then need to be a > str[n]cpy() (except we banned.h it) or memcpy() with a cast... >
Hi Junio On 14/10/2021 21:22, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> It's great to see these leaks being fixed. I wonder though if it would >>> be better to change the structure definition so that 'name' and 'path' >>> are no longer 'const'. That would be a better reflection of the new >>> regime.[...] >> >> I think this is the right thing to do, but I'm not quite sure. There was >> a thread at it here: >> >> https://lore.kernel.org/git/YUZG0D5ayEWd7MLP@carlos-mbp.lan/ >> >> Where I chimed in and suggested exactly what you're saying here, but the >> consensus seemed to go the other way, and if you grep: >> >> git grep -F 'free((char *)' >> >> You can see that we use this pattern pretty widely. > > Unfortunately, we probably need to make a trade-off and cannot eat > the cake and have it at the same time. > > If we leave the .members non-const, the destructor may have to cast > the constness away. If it is marked const * const, then we also > need to let the constructor do the same. It's not just in the destructor though, there are several other places where we cast the value to free it suggesting it is not actually const. I'd rather pass a "const struct branch_info*" around to all the callers that are not mutating the struct (we already do that in some places but not all) and change the structure definition to avoid the casts where it is mutated. > By marking the .members const, we can be sure that the users of the > API will not muck with the values once the structure is instanciated > and given to them, but the destructor need to cast the constness > away. It may be lessor of two evils, as the need to cast is isolated > in the _implementation_ of the API, and casts in the _users_ of the API > would stand out more. If it was just the destructor that was free()'ing the values I'd agree but the struct gets mutated in other places as well. Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > If it was just the destructor that was free()'ing the values I'd agree > but the struct gets mutated in other places as well. Oh, if the members are meant to be mutated by the users (as opposed to the implementation) of the API around the type, I would agree that we'd be much better off having them non-const. Thanks.
diff --git a/builtin/checkout.c b/builtin/checkout.c index 8c69dcdf72a..a85eb66da16 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -103,6 +103,16 @@ struct branch_info { char *checkout; }; +static void branch_info_release(struct branch_info *info) +{ + if (!info) + return; + free((char *)info->name); + free((char *)info->path); + free(info->refname); + free(info->checkout); +} + static int post_checkout_hook(struct commit *old_commit, struct commit *new_commit, int changed) { @@ -686,8 +696,10 @@ static void setup_branch_path(struct branch_info *branch) repo_get_oid_committish(the_repository, branch->name, &branch->oid); strbuf_branchname(&buf, branch->name, INTERPRET_BRANCH_LOCAL); - if (strcmp(buf.buf, branch->name)) + if (strcmp(buf.buf, branch->name)) { + free((char *)branch->name); branch->name = xstrdup(buf.buf); + } strbuf_splice(&buf, 0, 0, "refs/heads/", 11); branch->path = strbuf_detach(&buf, NULL); } @@ -896,7 +908,9 @@ static void update_refs_for_switch(const struct checkout_opts *opts, opts->new_branch_log, opts->quiet, opts->track); - new_branch_info->name = opts->new_branch; + free((char *)new_branch_info->name); + free(new_branch_info->refname); + new_branch_info->name = xstrdup(opts->new_branch); setup_branch_path(new_branch_info); } @@ -1064,8 +1078,7 @@ static int switch_branches(const struct checkout_opts *opts, struct branch_info *new_branch_info) { int ret = 0; - struct branch_info old_branch_info; - void *path_to_free; + struct branch_info old_branch_info = { 0 }; struct object_id rev; int flag, writeout_error = 0; int do_merge = 1; @@ -1073,25 +1086,32 @@ static int switch_branches(const struct checkout_opts *opts, trace2_cmd_mode("branch"); memset(&old_branch_info, 0, sizeof(old_branch_info)); - old_branch_info.path = path_to_free = resolve_refdup("HEAD", 0, &rev, &flag); + old_branch_info.path = resolve_refdup("HEAD", 0, &rev, &flag); if (old_branch_info.path) old_branch_info.commit = lookup_commit_reference_gently(the_repository, &rev, 1); - if (!(flag & REF_ISSYMREF)) + if (!(flag & REF_ISSYMREF)) { + free((char *)old_branch_info.path); old_branch_info.path = NULL; + } - if (old_branch_info.path) - skip_prefix(old_branch_info.path, "refs/heads/", &old_branch_info.name); + if (old_branch_info.path) { + const char *p; + if (skip_prefix(old_branch_info.path, "refs/heads/", &p)) + old_branch_info.name = xstrdup(p); + else + BUG("Should be able to skip with %s!", old_branch_info.path); + } if (opts->new_orphan_branch && opts->orphan_from_empty_tree) { if (new_branch_info->name) BUG("'switch --orphan' should never accept a commit as starting point"); new_branch_info->commit = NULL; - new_branch_info->name = "(empty)"; + new_branch_info->name = xstrdup("(empty)"); do_merge = 1; } if (!new_branch_info->name) { - new_branch_info->name = "HEAD"; + new_branch_info->name = xstrdup("HEAD"); new_branch_info->commit = old_branch_info.commit; if (!new_branch_info->commit) die(_("You are on a branch yet to be born")); @@ -1104,7 +1124,7 @@ static int switch_branches(const struct checkout_opts *opts, if (do_merge) { ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error); if (ret) { - free(path_to_free); + branch_info_release(&old_branch_info); return ret; } } @@ -1115,7 +1135,8 @@ static int switch_branches(const struct checkout_opts *opts, update_refs_for_switch(opts, &old_branch_info, new_branch_info); ret = post_checkout_hook(old_branch_info.commit, new_branch_info->commit, 1); - free(path_to_free); + branch_info_release(&old_branch_info); + return ret || writeout_error; } @@ -1147,7 +1168,7 @@ static void setup_new_branch_info_and_source_tree( struct tree **source_tree = &opts->source_tree; struct object_id branch_rev; - new_branch_info->name = arg; + new_branch_info->name = xstrdup(arg); setup_branch_path(new_branch_info); if (!check_refname_format(new_branch_info->path, 0) && @@ -1576,12 +1597,11 @@ static char cb_option = 'b'; static int checkout_main(int argc, const char **argv, const char *prefix, struct checkout_opts *opts, struct option *options, - const char * const usagestr[]) + const char * const usagestr[], + struct branch_info *new_branch_info) { - struct branch_info new_branch_info; int parseopt_flags = 0; - memset(&new_branch_info, 0, sizeof(new_branch_info)); opts->overwrite_ignore = 1; opts->prefix = prefix; opts->show_progress = -1; @@ -1690,7 +1710,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, opts->track == BRANCH_TRACK_UNSPECIFIED && !opts->new_branch; int n = parse_branchname_arg(argc, argv, dwim_ok, - &new_branch_info, opts, &rev); + new_branch_info, opts, &rev); argv += n; argc -= n; } else if (!opts->accept_ref && opts->from_treeish) { @@ -1699,7 +1719,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, if (get_oid_mb(opts->from_treeish, &rev)) die(_("could not resolve %s"), opts->from_treeish); - setup_new_branch_info_and_source_tree(&new_branch_info, + setup_new_branch_info_and_source_tree(new_branch_info, opts, &rev, opts->from_treeish); @@ -1719,7 +1739,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, * Try to give more helpful suggestion. * new_branch && argc > 1 will be caught later. */ - if (opts->new_branch && argc == 1 && !new_branch_info.commit) + if (opts->new_branch && argc == 1 && !new_branch_info->commit) die(_("'%s' is not a commit and a branch '%s' cannot be created from it"), argv[0], opts->new_branch); @@ -1768,11 +1788,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix, strbuf_release(&buf); } - UNLEAK(opts); if (opts->patch_mode || opts->pathspec.nr) - return checkout_paths(opts, &new_branch_info); + return checkout_paths(opts, new_branch_info); else - return checkout_branch(opts, &new_branch_info); + return checkout_branch(opts, new_branch_info); } int cmd_checkout(int argc, const char **argv, const char *prefix) @@ -1791,6 +1810,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) OPT_END() }; int ret; + struct branch_info new_branch_info = { 0 }; memset(&opts, 0, sizeof(opts)); opts.dwim_new_local_branch = 1; @@ -1821,7 +1841,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) options = add_checkout_path_options(&opts, options); ret = checkout_main(argc, argv, prefix, &opts, - options, checkout_usage); + options, checkout_usage, &new_branch_info); + branch_info_release(&new_branch_info); + clear_pathspec(&opts.pathspec); FREE_AND_NULL(options); return ret; } @@ -1842,6 +1864,7 @@ int cmd_switch(int argc, const char **argv, const char *prefix) OPT_END() }; int ret; + struct branch_info new_branch_info = { 0 }; memset(&opts, 0, sizeof(opts)); opts.dwim_new_local_branch = 1; @@ -1861,7 +1884,8 @@ int cmd_switch(int argc, const char **argv, const char *prefix) cb_option = 'c'; ret = checkout_main(argc, argv, prefix, &opts, - options, switch_branch_usage); + options, switch_branch_usage, &new_branch_info); + branch_info_release(&new_branch_info); FREE_AND_NULL(options); return ret; } @@ -1883,6 +1907,7 @@ int cmd_restore(int argc, const char **argv, const char *prefix) OPT_END() }; int ret; + struct branch_info new_branch_info = { 0 }; memset(&opts, 0, sizeof(opts)); opts.accept_ref = 0; @@ -1898,7 +1923,8 @@ int cmd_restore(int argc, const char **argv, const char *prefix) options = add_checkout_path_options(&opts, options); ret = checkout_main(argc, argv, prefix, &opts, - options, restore_usage); + options, restore_usage, &new_branch_info); + branch_info_release(&new_branch_info); FREE_AND_NULL(options); return ret; } diff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh index 83b09e13106..12e30d77d09 100755 --- a/t/t1005-read-tree-reset.sh +++ b/t/t1005-read-tree-reset.sh @@ -2,6 +2,7 @@ test_description='read-tree -u --reset' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-read-tree.sh diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh index 0a87058971e..3c19edcf30b 100755 --- a/t/t1406-submodule-ref-store.sh +++ b/t/t1406-submodule-ref-store.sh @@ -5,6 +5,7 @@ test_description='test submodule ref store api' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh RUN="test-tool ref-store submodule:sub" diff --git a/t/t2008-checkout-subdir.sh b/t/t2008-checkout-subdir.sh index eadb9434ae7..8a518a44ea2 100755 --- a/t/t2008-checkout-subdir.sh +++ b/t/t2008-checkout-subdir.sh @@ -4,6 +4,7 @@ test_description='git checkout from subdirectories' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t2014-checkout-switch.sh b/t/t2014-checkout-switch.sh index ccfb1471135..c138bdde4fe 100755 --- a/t/t2014-checkout-switch.sh +++ b/t/t2014-checkout-switch.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='Peter MacMillan' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t2026-checkout-pathspec-file.sh b/t/t2026-checkout-pathspec-file.sh index 43d31d79485..9db11f86dd6 100755 --- a/t/t2026-checkout-pathspec-file.sh +++ b/t/t2026-checkout-pathspec-file.sh @@ -2,6 +2,7 @@ test_description='checkout --pathspec-from-file' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_tick diff --git a/t/t9102-git-svn-deep-rmdir.sh b/t/t9102-git-svn-deep-rmdir.sh index 66cd51102c8..7b2049caa0c 100755 --- a/t/t9102-git-svn-deep-rmdir.sh +++ b/t/t9102-git-svn-deep-rmdir.sh @@ -1,5 +1,7 @@ #!/bin/sh test_description='git svn rmdir' + +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-svn.sh test_expect_success 'initialize repo' '
The "checkout" command is one of the main sources of leaks in the test suite, let's fix the common ones by not leaking from the "struct branch_info". Doing this is rather straightforward, albeit verbose, we need to xstrdup() constant strings going into the struct, and free() the ones we clobber as we go along. This also means that we can delete previous partial leak fixes in this area, i.e. the "path_to_free" accounting added by 96ec7b1e708 (Convert resolve_ref+xstrdup to new resolve_refdup function, 2011-12-13). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- As with other leak fixes I merged this to "seen" and tested it in combination with in-flight topics under GIT_TEST_PASSING_SANITIZE_LEAK=true. builtin/checkout.c | 76 +++++++++++++++++++++---------- t/t1005-read-tree-reset.sh | 1 + t/t1406-submodule-ref-store.sh | 1 + t/t2008-checkout-subdir.sh | 1 + t/t2014-checkout-switch.sh | 2 + t/t2026-checkout-pathspec-file.sh | 1 + t/t9102-git-svn-deep-rmdir.sh | 2 + 7 files changed, 59 insertions(+), 25 deletions(-)