On Sun, Jun 20, 2021 at 8:14 AM <andrzej@xxxxxxxxx> wrote:
>
> From: Andrzej Hunt <ajrhunt@xxxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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.
