| Message ID | 20210620151204.19260-6-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:14 AM <andrzej@ahunt.org> wrote: > > From: Andrzej Hunt <ajrhunt@google.com> > > old_dir/new_dir are free()'d at the end of update_dir_rename_counts, > however if we return early we'll never free those strings. Therefore > we should move all new allocations after the possible early return, > avoiding a leak. > > This seems like a fairly recent leak, that started happening since the > early-return was added in: > 1ad69eb0dc (diffcore-rename: compute dir_rename_counts in stages, 2021-02-27) The entire function was added relatively recently, just a few commits before that one at 0c4fd732f0 ("Move computation of dir_rename_count from merge-ort to diffcore-rename", 2021-02-27) > LSAN output from t0022: > > Direct leak of 7 byte(s) in 1 object(s) allocated from: > #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3 > #1 0xa71e48 in xstrdup wrapper.c:29:14 > #2 0x7db9c7 in update_dir_rename_counts diffcore-rename.c:464:12 > #3 0x7db6ae in find_renames diffcore-rename.c:1062:3 > #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18 > #5 0x7b4cfc in diffcore_std diff.c:6705:4 > #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2 > #7 0x856574 in log_tree_diff log-tree.c:955:3 > #8 0x856574 in log_tree_commit log-tree.c:986:10 > #9 0x9a9c67 in print_commit_summary sequencer.c:1329:7 > #10 0x52e623 in cmd_commit builtin/commit.c:1862:3 > #11 0x4ce83e in run_builtin git.c:475:11 > #12 0x4ccafe in handle_builtin git.c:729:3 > #13 0x4cb01c in run_argv git.c:818:4 > #14 0x4cb01c in cmd_main git.c:949:19 > #15 0x6b3f3d in main common-main.c:52:11 > #16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349) > > Direct leak of 7 byte(s) in 1 object(s) allocated from: > #0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3 > #1 0xa71e48 in xstrdup wrapper.c:29:14 > #2 0x7db9bc in update_dir_rename_counts diffcore-rename.c:463:12 > #3 0x7db6ae in find_renames diffcore-rename.c:1062:3 > #4 0x7d76c3 in diffcore_rename_extended diffcore-rename.c:1472:18 > #5 0x7b4cfc in diffcore_std diff.c:6705:4 > #6 0x855e46 in log_tree_diff_flush log-tree.c:846:2 > #7 0x856574 in log_tree_diff log-tree.c:955:3 > #8 0x856574 in log_tree_commit log-tree.c:986:10 > #9 0x9a9c67 in print_commit_summary sequencer.c:1329:7 > #10 0x52e623 in cmd_commit builtin/commit.c:1862:3 > #11 0x4ce83e in run_builtin git.c:475:11 > #12 0x4ccafe in handle_builtin git.c:729:3 > #13 0x4cb01c in run_argv git.c:818:4 > #14 0x4cb01c in cmd_main git.c:949:19 > #15 0x6b3f3d in main common-main.c:52:11 > #16 0x7fe397c7a349 in __libc_start_main (/lib64/libc.so.6+0x24349) > > SUMMARY: AddressSanitizer: 14 byte(s) leaked in 2 allocation(s). I ran this code under valgrind with specific tests, but apparently not under enough different cases. Thanks for catching this. > Signed-off-by: Andrzej Hunt <andrzej@ahunt.org> > --- > diffcore-rename.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/diffcore-rename.c b/diffcore-rename.c > index 3375e24659..f7c728fe47 100644 > --- a/diffcore-rename.c > +++ b/diffcore-rename.c > @@ -455,9 +455,9 @@ static void update_dir_rename_counts(struct dir_rename_info *info, > const char *oldname, > const char *newname) > { > - char *old_dir = xstrdup(oldname); > - char *new_dir = xstrdup(newname); > - char new_dir_first_char = new_dir[0]; > + char *old_dir; > + char *new_dir; > + const char new_dir_first_char = newname[0]; > int first_time_in_loop = 1; > > if (!info->setup) > @@ -482,6 +482,10 @@ static void update_dir_rename_counts(struct dir_rename_info *info, > */ > return; > > + > + old_dir = xstrdup(oldname); > + new_dir = xstrdup(newname); > + > while (1) { > int drd_flag = NOT_RELEVANT; > > -- > 2.26.2 The patch is correct.
diff --git a/diffcore-rename.c b/diffcore-rename.c index 3375e24659..f7c728fe47 100644 --- a/diffcore-rename.c +++ b/diffcore-rename.c @@ -455,9 +455,9 @@ static void update_dir_rename_counts(struct dir_rename_info *info, const char *oldname, const char *newname) { - char *old_dir = xstrdup(oldname); - char *new_dir = xstrdup(newname); - char new_dir_first_char = new_dir[0]; + char *old_dir; + char *new_dir; + const char new_dir_first_char = newname[0]; int first_time_in_loop = 1; if (!info->setup) @@ -482,6 +482,10 @@ static void update_dir_rename_counts(struct dir_rename_info *info, */ return; + + old_dir = xstrdup(oldname); + new_dir = xstrdup(newname); + while (1) { int drd_flag = NOT_RELEVANT;