Re: [PATCH 05/12] diffcore-rename: move old_dir/new_dir definition to plug leak

From: Elijah Newren <newren@gmail.com>
Date: Jun 21, 2021, 14:01 UTC
Message-ID: <CABPp-BHvr1Y0XxXfEavhOKkrSf9s6S5H8dXBZy0kR5CTohmuUQ@mail.gmail.com>
In-Reply-To: <20210620151204.19260-6-andrzej@ahunt.org>

On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote:

Show 11 quoted lines

>
> 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)

Show 41 quoted lines

> 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.

Show 35 quoted lines

> 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.