Thread (44 messages) 44 messages, 5 authors, 2021-07-27

Re: [PATCH 07/12] read-cache: call diff_setup_done to avoid leak

From: Elijah Newren <hidden>
Date: 2021-06-21 21:17:36

On Sun, Jun 20, 2021 at 8:15 AM [off-list ref] wrote:
From: Andrzej Hunt <redacted>

repo_diff_setup() calls through to diff.c's static prep_parse_options(),
which in  turn allocates a new array into diff_opts.parseopts.
diff_setup_done() is responsible for freeing that array, and has the
benefit of verifying diff_opts too - hence we add a call to
diff_setup_done() to avoid leaking parseopts.
Should the documentation near the top of diff.h also point out that
part of the purpose of diff_setup_done() is to free some memory?
quoted hunk ↗ jump to hunk
Output from the leak as found while running t0090 with LSAN:

Direct leak of 7120 byte(s) in 1 object(s) allocated from:
    #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0xa8bf89 in do_xmalloc wrapper.c:41:8
    #2 0x7a7bae in prep_parse_options diff.c:5636:2
    #3 0x7a7bae in repo_diff_setup diff.c:4611:2
    #4 0x93716c in repo_index_has_changes read-cache.c:2518:3
    #5 0x872233 in unclean merge-ort-wrappers.c:12:14
    #6 0x872233 in merge_ort_recursive merge-ort-wrappers.c:53:6
    #7 0x5d5b11 in try_merge_strategy builtin/merge.c:752:12
    #8 0x5d0b6b in cmd_merge builtin/merge.c:1666:9
    #9 0x4ce83e in run_builtin git.c:475:11
    #10 0x4ccafe in handle_builtin git.c:729:3
    #11 0x4cb01c in run_argv git.c:818:4
    #12 0x4cb01c in cmd_main git.c:949:19
    #13 0x6bdc2d in main common-main.c:52:11
    #14 0x7f551eb51349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 7120 byte(s) leaked in 1 allocation(s)

Signed-off-by: Andrzej Hunt <redacted>
---
 read-cache.c | 1 +
 1 file changed, 1 insertion(+)
diff --git a/read-cache.c b/read-cache.c
index 77961a3885..212d604dd3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2487,37 +2487,38 @@ int unmerged_index(const struct index_state *istate)
 int repo_index_has_changes(struct repository *repo,
                           struct tree *tree,
                           struct strbuf *sb)
 {
        struct index_state *istate = repo->index;
        struct object_id cmp;
        int i;

        if (tree)
                cmp = tree->object.oid;
        if (tree || !get_oid_tree("HEAD", &cmp)) {
                struct diff_options opt;

                repo_diff_setup(repo, &opt);
                opt.flags.exit_with_status = 1;
                if (!sb)
                        opt.flags.quick = 1;
+               diff_setup_done(&opt);
                do_diff_cache(&cmp, &opt);
                diffcore_std(&opt);
                for (i = 0; sb && i < diff_queued_diff.nr; i++) {
                        if (i)
                                strbuf_addch(sb, ' ');
                        strbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);
                }
                diff_flush(&opt);
                return opt.flags.has_changes != 0;
        } else {
                /* TODO: audit for interaction with sparse-index. */
                ensure_full_index(istate);
                for (i = 0; sb && i < istate->cache_nr; i++) {
                        if (i)
                                strbuf_addch(sb, ' ');
                        strbuf_addstr(sb, istate->cache[i]->name);
                }
                return !!istate->cache_nr;
        }
 }
--
2.26.2
Patch makes sense; a quick `git grep -e repo_diff_setup -e
diff_setup_done` doesn't flag any other areas of the code as having
the same bug.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help