| Message ID | 20210620151204.19260-13-andrzej@ahunt.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix all leaks in tests t0002-t0099: Part 2 | expand |
On Sun, Jun 20, 2021 at 8:15 AM <andrzej@ahunt.org> wrote: > > From: Andrzej Hunt <ajrhunt@google.com> > > 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) > 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 <andrzej@ahunt.org> > --- > 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.
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;