object-name: free leaking object contexts

While it is documented in `struct object_context::path` that this
variable needs to be released by the caller, this fact is rather easy to
miss given that we do not ever provide a function to release the object
context. And of course, while some callers dutifully release the path,
many others don't.

Introduce a new `object_context_release()` function that releases the
path. Convert callsites that used to free the path to use that new
function and add missing calls to callsites that were leaking memory.
Refactor those callsites as required to have a single return path, only.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 43a1d7ac4989a22023d7060baa9ae880d1dd7c9a..18fe58d6b8b043e182b50f6204c8803e32729845 100644 (file)
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -102,7 +102,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
        enum object_type type;
        char *buf;
        unsigned long size;
-       struct object_context obj_context;
+       struct object_context obj_context = {0};
        struct object_info oi = OBJECT_INFO_INIT;
        struct strbuf sb = STRBUF_INIT;
        unsigned flags = OBJECT_INFO_LOOKUP_REPLACE;
@@ -163,7 +163,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
                goto cleanup;
 
        case 'e':
-               return !repo_has_object_file(the_repository, &oid);
+               ret = !repo_has_object_file(the_repository, &oid);
+               goto cleanup;
 
        case 'w':
 
@@ -268,7 +269,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
        ret = 0;
 cleanup:
        free(buf);
-       free(obj_context.path);
+       object_context_release(&obj_context);
        return ret;
 }
 
@@ -520,7 +521,7 @@ static void batch_one_object(const char *obj_name,
                             struct batch_options *opt,
                             struct expand_data *data)
 {
-       struct object_context ctx;
+       struct object_context ctx = {0};
        int flags =
                GET_OID_HASH_ANY |
                (opt->follow_symlinks ? GET_OID_FOLLOW_SYMLINKS : 0);
@@ -557,7 +558,8 @@ static void batch_one_object(const char *obj_name,
                        break;
                }
                fflush(stdout);
-               return;
+
+               goto out;
        }
 
        if (ctx.mode == 0) {
@@ -565,10 +567,13 @@ static void batch_one_object(const char *obj_name,
                       (uintmax_t)ctx.symlink_path.len,
                       opt->output_delim, ctx.symlink_path.buf, opt->output_delim);
                fflush(stdout);
-               return;
+               goto out;
        }
 
        batch_object_write(obj_name, scratch, opt, data, NULL, 0);
+
+out:
+       object_context_release(&ctx);
 }
 
 struct object_cb_data {

diff --git a/builtin/grep.c b/builtin/grep.c
index 5777ba82a988e8a26dcae2e77ab8fb3423b70a6f..dfc3c3e8bd28a009cdf4a4f7b95ab63588140a7f 100644 (file)
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1114,7 +1114,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
        for (i = 0; i < argc; i++) {
                const char *arg = argv[i];
                struct object_id oid;
-               struct object_context oc;
+               struct object_context oc = {0};
                struct object *object;
 
                if (!strcmp(arg, "--")) {
@@ -1140,7 +1140,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
                if (!seen_dashdash)
                        verify_non_filename(prefix, arg);
                add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
-               free(oc.path);
+               object_context_release(&oc);
        }
 
        /*

diff --git a/builtin/log.c b/builtin/log.c
index 4e4b645a21de4ea5f398737cbe5635b688df3314..37ecb3ff8b93324a2aaee5a17d467b15fa41be76 100644 (file)
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -682,7 +682,7 @@ static void show_tagger(const char *buf, struct rev_info *rev)
 static int show_blob_object(const struct object_id *oid, struct rev_info *rev, const char *obj_name)
 {
        struct object_id oidc;
-       struct object_context obj_context;
+       struct object_context obj_context = {0};
        char *buf;
        unsigned long size;
 
@@ -698,7 +698,7 @@ static int show_blob_object(const struct object_id *oid, struct rev_info *rev, c
        if (!obj_context.path ||
            !textconv_object(the_repository, obj_context.path,
                             obj_context.mode, &oidc, 1, &buf, &size)) {
-               free(obj_context.path);
+               object_context_release(&obj_context);
                return stream_blob_to_fd(1, oid, NULL, 0);
        }
 
@@ -706,7 +706,7 @@ static int show_blob_object(const struct object_id *oid, struct rev_info *rev, c
                die(_("git show %s: bad file"), obj_name);
 
        write_or_die(1, buf, size);
-       free(obj_context.path);
+       object_context_release(&obj_context);
        return 0;
 }
 

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 7bf84b235ce69e63aaf0f11f6dfb82bce0a42eeb..bf372c67d773bd4ce5fe9285b818bba1372595c2 100644 (file)
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -367,7 +367,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
                OPT_END()
        };
        struct ls_tree_cmdmode_to_fmt *m2f = ls_tree_cmdmode_format;
-       struct object_context obj_context;
+       struct object_context obj_context = {0};
        int ret;
 
        git_config(git_default_config, NULL);
@@ -441,5 +441,6 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 
        ret = !!read_tree(the_repository, tree, &options.pathspec, fn, &options);
        clear_pathspec(&options.pathspec);
+       object_context_release(&obj_context);
        return ret;
 }

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index ab8a8f3b0ed76189d48b4133a7fd49abb46eda74..2e64f5bda70bc2e41a7477d919548a13ba68bd3f 100644 (file)
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -1128,6 +1128,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
                }
                if (!get_oid_with_context(the_repository, name,
                                          flags, &oid, &unused)) {
+                       object_context_release(&unused);
                        if (output_algo)
                                repo_oid_to_algop(the_repository, &oid,
                                                  output_algo, &oid);
@@ -1137,6 +1138,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
                                show_rev(type, &oid, name);
                        continue;
                }
+               object_context_release(&unused);
                if (verify)
                        die_no_single_rev(quiet);
                if (has_dashdash)

diff --git a/builtin/stash.c b/builtin/stash.c
index 7859bc0866adc10c1c83d1ec9de7b72fc2219d15..628d848a0bc85d3053fc82805710a1ebfb5d66ed 100644 (file)
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1018,13 +1018,14 @@ static int store_stash(int argc, const char **argv, const char *prefix)
        int quiet = 0;
        const char *stash_msg = NULL;
        struct object_id obj;
-       struct object_context dummy;
+       struct object_context dummy = {0};
        struct option options[] = {
                OPT__QUIET(&quiet, N_("be quiet")),
                OPT_STRING('m', "message", &stash_msg, "message",
                           N_("stash message")),
                OPT_END()
        };
+       int ret;
 
        argc = parse_options(argc, argv, prefix, options,
                             git_stash_store_usage,
@@ -1043,10 +1044,15 @@ static int store_stash(int argc, const char **argv, const char *prefix)
                if (!quiet)
                        fprintf_ln(stderr, _("Cannot update %s with %s"),
                                             ref_stash, argv[0]);
-               return -1;
+               ret = -1;
+               goto out;
        }
 
-       return do_store_stash(&obj, stash_msg, quiet);
+       ret = do_store_stash(&obj, stash_msg, quiet);
+
+out:
+       object_context_release(&dummy);
+       return ret;
 }
 
 static void add_pathspecs(struct strvec *args,

diff --git a/list-objects-filter.c b/list-objects-filter.c
index 4346f8da4560fd518d73db7969faa9127e302090..c95ec3509ae08a96224104094af8c4ed3955bb63 100644 (file)
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -542,6 +542,8 @@ static void filter_sparse_oid__init(
        filter->filter_data = d;
        filter->filter_object_fn = filter_sparse;
        filter->free_fn = filter_sparse_free;
+
+       object_context_release(&oc);
 }
 
 /*

diff --git a/object-name.c b/object-name.c
index 523af6f64f33512d1d7587bf0ce96cbe23b63b94..0471fafc98d5af51ad39b0e7e836128a451c6162 100644 (file)
--- a/object-name.c
+++ b/object-name.c
@@ -1757,6 +1757,11 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
        return check_refname_format(sb->buf, 0);
 }
 
+void object_context_release(struct object_context *ctx)
+{
+       free(ctx->path);
+}
+
 /*
  * This is like "get_oid_basic()", except it allows "object ID expressions",
  * notably "xyz^" for "parent of xyz"
@@ -1764,7 +1769,9 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 int repo_get_oid(struct repository *r, const char *name, struct object_id *oid)
 {
        struct object_context unused;
-       return get_oid_with_context(r, name, 0, oid, &unused);
+       int ret = get_oid_with_context(r, name, 0, oid, &unused);
+       object_context_release(&unused);
+       return ret;
 }
 
 /*
@@ -1802,8 +1809,10 @@ int repo_get_oid_committish(struct repository *r,
                            struct object_id *oid)
 {
        struct object_context unused;
-       return get_oid_with_context(r, name, GET_OID_COMMITTISH,
-                                   oid, &unused);
+       int ret = get_oid_with_context(r, name, GET_OID_COMMITTISH,
+                                      oid, &unused);
+       object_context_release(&unused);
+       return ret;
 }
 
 int repo_get_oid_treeish(struct repository *r,
@@ -1811,8 +1820,10 @@ int repo_get_oid_treeish(struct repository *r,
                         struct object_id *oid)
 {
        struct object_context unused;
-       return get_oid_with_context(r, name, GET_OID_TREEISH,
-                                   oid, &unused);
+       int ret = get_oid_with_context(r, name, GET_OID_TREEISH,
+                                      oid, &unused);
+       object_context_release(&unused);
+       return ret;
 }
 
 int repo_get_oid_commit(struct repository *r,
@@ -1820,8 +1831,10 @@ int repo_get_oid_commit(struct repository *r,
                        struct object_id *oid)
 {
        struct object_context unused;
-       return get_oid_with_context(r, name, GET_OID_COMMIT,
-                                   oid, &unused);
+       int ret = get_oid_with_context(r, name, GET_OID_COMMIT,
+                                      oid, &unused);
+       object_context_release(&unused);
+       return ret;
 }
 
 int repo_get_oid_tree(struct repository *r,
@@ -1829,8 +1842,10 @@ int repo_get_oid_tree(struct repository *r,
                      struct object_id *oid)
 {
        struct object_context unused;
-       return get_oid_with_context(r, name, GET_OID_TREE,
-                                   oid, &unused);
+       int ret = get_oid_with_context(r, name, GET_OID_TREE,
+                                      oid, &unused);
+       object_context_release(&unused);
+       return ret;
 }
 
 int repo_get_oid_blob(struct repository *r,
@@ -1838,8 +1853,10 @@ int repo_get_oid_blob(struct repository *r,
                      struct object_id *oid)
 {
        struct object_context unused;
-       return get_oid_with_context(r, name, GET_OID_BLOB,
-                                   oid, &unused);
+       int ret = get_oid_with_context(r, name, GET_OID_BLOB,
+                                      oid, &unused);
+       object_context_release(&unused);
+       return ret;
 }
 
 /* Must be called only when object_name:filename doesn't exist. */
@@ -2117,6 +2134,7 @@ void maybe_die_on_misspelt_object_name(struct repository *r,
        struct object_id oid;
        get_oid_with_context_1(r, name, GET_OID_ONLY_TO_DIE | GET_OID_QUIETLY,
                               prefix, &oid, &oc);
+       object_context_release(&oc);
 }
 
 enum get_oid_result get_oid_with_context(struct repository *repo,

diff --git a/object-name.h b/object-name.h
index 064ddc97d1fe991c98272802c9eff65c8445a49b..8dba4a47a470119a2a8a7f278bdf22f0058d92b2 100644 (file)
--- a/object-name.h
+++ b/object-name.h
@@ -22,6 +22,8 @@ struct object_context {
        char *path;
 };
 
+void object_context_release(struct object_context *ctx);
+
 /*
  * Return an abbreviated sha1 unique within this repository's object database.
  * The result will be at least `len` characters long, and will be NUL

diff --git a/revision.c b/revision.c
index 75e71bcaea8db39d18ef94031bd0c14f2dd02c53..82c0aadb42ce984bcae223fd569e85e7023968a3 100644 (file)
--- a/revision.c
+++ b/revision.c
@@ -2130,30 +2130,26 @@ static int handle_dotdot(const char *arg,
                         struct rev_info *revs, int flags,
                         int cant_be_filename)
 {
-       struct object_context a_oc, b_oc;
+       struct object_context a_oc = {0}, b_oc = {0};
        char *dotdot = strstr(arg, "..");
        int ret;
 
        if (!dotdot)
                return -1;
 
-       memset(&a_oc, 0, sizeof(a_oc));
-       memset(&b_oc, 0, sizeof(b_oc));
-
        *dotdot = '\0';
        ret = handle_dotdot_1(arg, dotdot, revs, flags, cant_be_filename,
                              &a_oc, &b_oc);
        *dotdot = '.';
 
-       free(a_oc.path);
-       free(b_oc.path);
-
+       object_context_release(&a_oc);
+       object_context_release(&b_oc);
        return ret;
 }
 
 static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)
 {
-       struct object_context oc;
+       struct object_context oc = {0};
        char *mark;
        struct object *object;
        struct object_id oid;
@@ -2161,6 +2157,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
        const char *arg = arg_;
        int cant_be_filename = revarg_opt & REVARG_CANNOT_BE_FILENAME;
        unsigned get_sha1_flags = GET_OID_RECORD_PATH;
+       int ret;
 
        flags = flags & UNINTERESTING ? flags | BOTTOM : flags & ~BOTTOM;
 
@@ -2169,17 +2166,22 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
                 * Just ".."?  That is not a range but the
                 * pathspec for the parent directory.
                 */
-               return -1;
+               ret = -1;
+               goto out;
        }
 
-       if (!handle_dotdot(arg, revs, flags, revarg_opt))
-               return 0;
+       if (!handle_dotdot(arg, revs, flags, revarg_opt)) {
+               ret = 0;
+               goto out;
+       }
 
        mark = strstr(arg, "^@");
        if (mark && !mark[2]) {
                *mark = 0;
-               if (add_parents_only(revs, arg, flags, 0))
-                       return 0;
+               if (add_parents_only(revs, arg, flags, 0)) {
+                       ret = 0;
+                       goto out;
+               }
                *mark = '^';
        }
        mark = strstr(arg, "^!");
@@ -2194,8 +2196,10 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
 
                if (mark[2]) {
                        if (strtol_i(mark + 2, 10, &exclude_parent) ||
-                           exclude_parent < 1)
-                               return -1;
+                           exclude_parent < 1) {
+                               ret = -1;
+                               goto out;
+                       }
                }
 
                *mark = 0;
@@ -2217,17 +2221,25 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
         * should error out if we can't even get an oid, as
         * `--missing=print` should be able to report missing oids.
         */
-       if (get_oid_with_context(revs->repo, arg, get_sha1_flags, &oid, &oc))
-               return revs->ignore_missing ? 0 : -1;
+       if (get_oid_with_context(revs->repo, arg, get_sha1_flags, &oid, &oc)) {
+               ret = revs->ignore_missing ? 0 : -1;
+               goto out;
+       }
        if (!cant_be_filename)
                verify_non_filename(revs->prefix, arg);
        object = get_reference(revs, arg, &oid, flags ^ local_flags);
-       if (!object)
-               return (revs->ignore_missing || revs->do_not_die_on_missing_objects) ? 0 : -1;
+       if (!object) {
+               ret = (revs->ignore_missing || revs->do_not_die_on_missing_objects) ? 0 : -1;
+               goto out;
+       }
        add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);
        add_pending_object_with_path(revs, object, arg, oc.mode, oc.path);
-       free(oc.path);
-       return 0;
+
+       ret = 0;
+
+out:
+       object_context_release(&oc);
+       return ret;
 }
 
 int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
@@ -3062,6 +3074,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
                        diagnose_missing_default(revs->def);
                object = get_reference(revs, revs->def, &oid, 0);
                add_pending_object_with_mode(revs, object, revs->def, oc.mode);
+               object_context_release(&oc);
        }
 
        /* Did the user ask for any diff output? Run the diff! */

diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh
index cd5c20fe51b3727035d56245c17568030f87d455..d984200c1733b50ff346e01910e4e3b5a3db331b 100755 (executable)
--- a/t/t7012-skip-worktree-writing.sh
+++ b/t/t7012-skip-worktree-writing.sh
@@ -5,6 +5,7 @@
 
 test_description='test worktree writing operations when skip-worktree is used'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '