undef/git-commit-vandalism
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

checkout: fix "branch info" memory leaks

Browse Source 
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 96ec7b1e70 (Convert
resolve_ref+xstrdup to new resolve_refdup function, 2011-12-13).

There was some discussion about whether "we should retain the "const
char *" here and cast at free() time, or have it be a "char *". Since
this is not a public API with any sort of API boundary let's use
"char *", as is already being done for the "refname" member of the
same struct.

The tests to mark as passing were found with:

    rm .prove; GIT_SKIP_TESTS=t0027 prove -j8 --state=save t[0-9]*.sh :: --immediate
    # apply & compile this change
    prove -j8 --state=failed :: --immediate

I.e. the ones that were newly passing when the --state=failed command
was run. I left out "t3040-subprojects-basic.sh" and
"t4131-apply-fake-ancestor.sh" to to optimization-level related
differences similar to the ones noted in[1], except that these would
be something the current 'linux-leaks' job would run into.

1. https://lore.kernel.org/git/cover-v3-0.6-00000000000-20211022T175227Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This commit is contained in:
Ævar Arnfjörð Bjarmason 2021-11-16 19:27:38 +01:00 committed by Junio C Hamano
parent cd3e606211
commit 9081a421a6
36 changed files with 98 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

86
builtin/checkout.c
View File

@ -91,8 +91,8 @@ struct checkout_opts {
};
struct branch_info {
const char *name; /* The short name used */
const char *path; /* The full name of a real branch */
char *name; /* The short name used */
char *path; /* The full name of a real branch */
struct commit *commit; /* The named commit */
char *refname; /* The full name of the ref being checked out. */
struct object_id oid; /* The object ID of the commit being checked out. */
@ -103,6 +103,14 @@ struct branch_info {
char *checkout;
};
static void branch_info_release(struct branch_info *info)
{
free(info->name);
free(info->path);
free(info->refname);
free(info->checkout);
}
static int post_checkout_hook(struct commit *old_commit, struct commit *new_commit,
int changed)
{
@ -688,9 +696,12 @@ 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(branch->name);
branch->name = xstrdup(buf.buf);
}
strbuf_splice(&buf, 0, 0, "refs/heads/", 11);
free(branch->path);
branch->path = strbuf_detach(&buf, NULL);
}
@ -894,7 +905,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(new_branch_info->name);
free(new_branch_info->refname);
new_branch_info->name = xstrdup(opts->new_branch);
setup_branch_path(new_branch_info);
}
@ -1062,8 +1075,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;
@ -1071,25 +1083,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))
old_branch_info.path = NULL;
FREE_AND_NULL(old_branch_info.path);
if (old_branch_info.path)
skip_prefix(old_branch_info.path, "refs/heads/", &old_branch_info.name);
if (old_branch_info.path) {
const char *const prefix = "refs/heads/";
const char *p;
if (skip_prefix(old_branch_info.path, prefix, &p))
old_branch_info.name = xstrdup(p);
else
BUG("should be able to skip past '%s' in '%s'!",
prefix, 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"));
@ -1102,7 +1121,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;
}
}
@ -1113,7 +1132,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;
}
@ -1145,16 +1165,15 @@ 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) &&
!read_ref(new_branch_info->path, &branch_rev))
oidcpy(rev, &branch_rev);
else {
free((char *)new_branch_info->path);
new_branch_info->path = NULL; /* not an existing branch */
}
else
/* not an existing branch */
FREE_AND_NULL(new_branch_info->path);
new_branch_info->commit = lookup_commit_reference_gently(the_repository, rev, 1);
if (!new_branch_info->commit) {
@ -1574,12 +1593,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;
@ -1688,7 +1706,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) {
@ -1697,7 +1715,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);
@ -1717,7 +1735,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);
@ -1766,11 +1784,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)
@ -1789,6 +1806,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;
@ -1819,7 +1837,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;
}
@ -1840,6 +1860,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;
@ -1859,7 +1880,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;
}
@ -1881,6 +1903,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;
@ -1896,7 +1919,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;
}

1
t/t0020-crlf.sh
View File

@ -5,6 +5,7 @@ test_description='CRLF conversion'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
has_cr() {

1
t/t1005-read-tree-reset.sh
View File

@ -2,6 +2,7 @@
test_description='read-tree -u --reset'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-read-tree.sh

1
t/t1008-read-tree-overlay.sh
View File

@ -5,6 +5,7 @@ test_description='test multi-tree read-tree without merging'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-read-tree.sh

1
t/t1403-show-ref.sh
View File

@ -4,6 +4,7 @@ test_description='show-ref'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '

1
t/t1406-submodule-ref-store.sh
View File

@ -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"

1
t/t1505-rev-parse-last.sh
View File

@ -5,6 +5,7 @@ test_description='test @{-N} syntax'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh

1
t/t2007-checkout-symlink.sh
View File

@ -7,6 +7,7 @@ test_description='git checkout to switch between branches with symlink<->dir'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '

1
t/t2008-checkout-subdir.sh
View File

@ -4,6 +4,7 @@
test_description='git checkout from subdirectories'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '

1
t/t2009-checkout-statinfo.sh
View File

@ -5,6 +5,7 @@ test_description='checkout should leave clean stat info'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '

1
t/t2010-checkout-ambiguous.sh
View File

@ -5,6 +5,7 @@ test_description='checkout and pathspecs/refspecs ambiguities'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '

1
t/t2011-checkout-invalid-head.sh
View File

@ -5,6 +5,7 @@ test_description='checkout switching away from an invalid branch'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '

2
t/t2014-checkout-switch.sh
View File

@ -1,6 +1,8 @@
#!/bin/sh
test_description='Peter MacMillan'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '

1
t/t2017-checkout-orphan.sh
View File

@ -10,6 +10,7 @@ Main Tests for --orphan functionality.'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
TEST_FILE=foo

2
t/t2019-checkout-ambiguous-ref.sh
View File

@ -1,6 +1,8 @@
#!/bin/sh
test_description='checkout handling of ambiguous (branch/tag) refs'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup ambiguous refs' '

2
t/t2021-checkout-overwrite.sh
View File

@ -1,6 +1,8 @@
#!/bin/sh
test_description='checkout must not overwrite an untracked objects'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '

1
t/t2022-checkout-paths.sh
View File

@ -4,6 +4,7 @@ test_description='checkout $tree -- $paths'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '

1
t/t2026-checkout-pathspec-file.sh
View File

@ -2,6 +2,7 @@
test_description='checkout --pathspec-from-file'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_tick

1
t/t2106-update-index-assume-unchanged.sh
View File

@ -3,6 +3,7 @@
test_description='git update-index --assume-unchanged test.
'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '

2
t/t3040-subprojects-basic.sh
View File

@ -1,6 +1,8 @@
#!/bin/sh
test_description='Basic subproject functionality'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup: create superproject' '

1
t/t3305-notes-fanout.sh
View File

@ -2,6 +2,7 @@
test_description='Test that adding/removing many notes triggers automatic fanout restructuring'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
path_has_fanout() {

2
t/t3422-rebase-incompatible-options.sh
View File

@ -1,6 +1,8 @@
#!/bin/sh
test_description='test if rebase detects and aborts on incompatible options'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '

1
t/t4115-apply-symlink.sh
View File

@ -7,6 +7,7 @@ test_description='git apply symlinks and partial files
'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '

1
t/t4121-apply-diffs.sh
View File

@ -4,6 +4,7 @@ test_description='git apply for contextually independent diffs'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
echo '1

1
t/t4204-patch-id.sh
View File

@ -5,6 +5,7 @@ test_description='git patch-id'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '

1
t/t5410-receive-pack-alternates.sh
View File

@ -5,6 +5,7 @@ test_description='git receive-pack with alternate ref filtering'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '

1
t/t5609-clone-branch.sh
View File

@ -4,6 +4,7 @@ test_description='clone --branch option'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
check_HEAD() {

1
t/t6407-merge-binary.sh
View File

@ -5,6 +5,7 @@ test_description='ask merge-recursive to merge binary files'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '

1
t/t6414-merge-rename-nocruft.sh
View File

@ -4,6 +4,7 @@ test_description='Merge-recursive merging renames'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '

1
t/t7113-post-index-change-hook.sh
View File

@ -5,6 +5,7 @@ test_description='post index change hook'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '

1
t/t7509-commit-authorship.sh
View File

@ -5,6 +5,7 @@
test_description='commit tests of various authorhip options. '
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
author_header () {

1
t/t7815-grep-binary.sh
View File

@ -2,6 +2,7 @@
test_description='git grep in binary files'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' "

2
t/t9102-git-svn-deep-rmdir.sh
View File

@ -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' '

1
t/t9123-git-svn-rebuild-with-rewriteroot.sh
View File

@ -5,6 +5,7 @@
test_description='git svn respects rewriteRoot during rebuild'
TEST_PASSES_SANITIZE_LEAK=true
. ./lib-git-svn.sh
mkdir import

2
t/t9128-git-svn-cmd-branch.sh
View File

@ -4,6 +4,8 @@
#
test_description='git svn partial-rebuild tests'
TEST_PASSES_SANITIZE_LEAK=true
. ./lib-git-svn.sh
test_expect_success 'initialize svnrepo' '

2
t/t9167-git-svn-cmd-branch-subproject.sh
View File

@ -4,6 +4,8 @@
#
test_description='git svn branch for subproject clones'
TEST_PASSES_SANITIZE_LEAK=true
. ./lib-git-svn.sh
test_expect_success 'initialize svnrepo' '