Re: [PATCH 12/12] reset: clear_unpack_trees_porcelain to plug leak
From: Elijah Newren <hidden>
Date: 2021-06-21 21:45:17
On Sun, Jun 20, 2021 at 8:15 AM [off-list ref] wrote:
From: Andrzej Hunt <redacted> setup_unpack_trees_porcelain() populates various fields on unpack_tree_opts, we need to call clear_unpack_trees_porcelain() to avoid leaking them. Specifically, we used to leak unpack_tree_opts.msgs_to_free. We have to do this in leave_reset_head because there are multiple scenarios where unpack_tree_opts has already been configured, followed by a 'goto leave_reset_head'. But we can also 'goto leave_reset_head' prior to having initialised unpack_tree_opts via memset(..., 0, ...). Therefore we also move unpack_tree_opts initialisation to the start of reset_head(), and convert it to use brace initialisation - which guarantees that we can never clear an unitialised unpack_tree_opts.
I think you mean either "uninitialized" or "uninitialised" (missing an 'in' in the spelling)
quoted hunk ↗ jump to hunk
clear_unpack_tree_opts() is always safe to call as long as unpack_tree_opts is at least zero-initialised, i.e. it does not depend on a previous call to setup_unpack_trees_porcelain(). LSAN output from t0021: Direct leak of 192 byte(s) in 1 object(s) allocated from: #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 #1 0xa721e5 in xrealloc wrapper.c:126:8 #2 0x9f7861 in strvec_push_nodup strvec.c:19:2 #3 0x9f7861 in strvec_pushf strvec.c:39:2 #4 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3 #5 0x97e011 in reset_head reset.c:53:2 #6 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9 #7 0x4ce83e in run_builtin git.c:475:11 #8 0x4ccafe in handle_builtin git.c:729:3 #9 0x4cb01c in run_argv git.c:818:4 #10 0x4cb01c in cmd_main git.c:949:19 #11 0x6b3f3d in main common-main.c:52:11 #12 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349) Indirect leak of 147 byte(s) in 1 object(s) allocated from: #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 #1 0xa721e5 in xrealloc wrapper.c:126:8 #2 0x9e8d54 in strbuf_grow strbuf.c:98:2 #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3 #4 0x9f7774 in strvec_pushf strvec.c:36:2 #5 0xa43e14 in setup_unpack_trees_porcelain unpack-trees.c:129:3 #6 0x97e011 in reset_head reset.c:53:2 #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9 #8 0x4ce83e in run_builtin git.c:475:11 #9 0x4ccafe in handle_builtin git.c:729:3 #10 0x4cb01c in run_argv git.c:818:4 #11 0x4cb01c in cmd_main git.c:949:19 #12 0x6b3f3d in main common-main.c:52:11 #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349) Indirect leak of 134 byte(s) in 1 object(s) allocated from: #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 #1 0xa721e5 in xrealloc wrapper.c:126:8 #2 0x9e8d54 in strbuf_grow strbuf.c:98:2 #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3 #4 0x9f7774 in strvec_pushf strvec.c:36:2 #5 0xa43fe4 in setup_unpack_trees_porcelain unpack-trees.c:168:3 #6 0x97e011 in reset_head reset.c:53:2 #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9 #8 0x4ce83e in run_builtin git.c:475:11 #9 0x4ccafe in handle_builtin git.c:729:3 #10 0x4cb01c in run_argv git.c:818:4 #11 0x4cb01c in cmd_main git.c:949:19 #12 0x6b3f3d in main common-main.c:52:11 #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349) Indirect leak of 130 byte(s) in 1 object(s) allocated from: #0 0x49ab49 in realloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3 #1 0xa721e5 in xrealloc wrapper.c:126:8 #2 0x9e8d54 in strbuf_grow strbuf.c:98:2 #3 0x9e8d54 in strbuf_vaddf strbuf.c:401:3 #4 0x9f7774 in strvec_pushf strvec.c:36:2 #5 0xa43f20 in setup_unpack_trees_porcelain unpack-trees.c:150:3 #6 0x97e011 in reset_head reset.c:53:2 #7 0x61dfa5 in cmd_rebase builtin/rebase.c:1991:9 #8 0x4ce83e in run_builtin git.c:475:11 #9 0x4ccafe in handle_builtin git.c:729:3 #10 0x4cb01c in run_argv git.c:818:4 #11 0x4cb01c in cmd_main git.c:949:19 #12 0x6b3f3d in main common-main.c:52:11 #13 0x7fa8addf3349 in __libc_start_main (/lib64/libc.so.6+0x24349) SUMMARY: AddressSanitizer: 603 byte(s) leaked in 4 allocation(s). Signed-off-by: Andrzej Hunt <redacted> --- reset.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)diff --git a/reset.c b/reset.c index 4bea758053..79310ae071 100644 --- a/reset.c +++ b/reset.c@@ -21,7 +21,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action, struct object_id head_oid; struct tree_desc desc[2] = { { NULL }, { NULL } }; struct lock_file lock = LOCK_INIT; - struct unpack_trees_options unpack_tree_opts; + struct unpack_trees_options unpack_tree_opts = { 0 }; struct tree *tree; const char *reflog_action; struct strbuf msg = STRBUF_INIT;@@ -49,7 +49,6 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action, if (refs_only) goto reset_head_refs; - memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts)); setup_unpack_trees_porcelain(&unpack_tree_opts, action); unpack_tree_opts.head_idx = 1; unpack_tree_opts.src_index = r->index;@@ -134,6 +133,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action, leave_reset_head: strbuf_release(&msg); rollback_lock_file(&lock); + clear_unpack_trees_porcelain(&unpack_tree_opts); while (nr) free((void *)desc[--nr].buffer); return ret; --2.26.2
Nice catch, and nice explanation. I think we probably have several similar problems throughout the code base; a quick grep (`git grep -e struct.unpack_trees_options -e clear_unpack_trees_porcelain`) suggests there are several places that clear_unpack_trees_porcelain() is probably missing and which could likely use your struct initialization trick as well.