Re: [PATCH 01/12] fmt-merge-msg: free newly allocated temporary strings when done

[Elijah Newren <newren@gmail.com>]

On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote:
>
> From: Andrzej Hunt <ajrhunt@google.com>
>
> origin starts off pointing to somewhere within line, which is owned by
> the caller. Later we might allocate a new string using xmemdupz() or
> xstrfmt(). To avoid leaking these new strings, we introduce a to_free
> pointer - which allows us to safely free the newly allocated string when
> we're done (we cannot just free origin directly as it might still be
> pointing to line).
>
> LSAN output from t0090:
>
> Direct leak of 8 byte(s) in 1 object(s) allocated from:
>     #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
>     #1 0xa71f49 in do_xmalloc wrapper.c:41:8
>     #2 0xa720b0 in do_xmallocz wrapper.c:75:8
>     #3 0xa720b0 in xmallocz wrapper.c:83:9
>     #4 0xa720b0 in xmemdupz wrapper.c:99:16
>     #5 0x8092ba in handle_line fmt-merge-msg.c:187:23
>     #6 0x8092ba in fmt_merge_msg fmt-merge-msg.c:666:7
>     #7 0x5ce2e6 in prepare_merge_message builtin/merge.c:1119:2
>     #8 0x5ce2e6 in collect_parents builtin/merge.c:1215:3
>     #9 0x5c9c1e in cmd_merge builtin/merge.c:1454:16
>     #10 0x4ce83e in run_builtin git.c:475:11
>     #11 0x4ccafe in handle_builtin git.c:729:3
>     #12 0x4cb01c in run_argv git.c:818:4
>     #13 0x4cb01c in cmd_main git.c:949:19
>     #14 0x6b3fad in main common-main.c:52:11
>     #15 0x7fb929620349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> SUMMARY: AddressSanitizer: 8 byte(s) leaked in 1 allocation(s).
>
> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>  fmt-merge-msg.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
> index 0f66818e0f..b969dc6ebb 100644
> --- a/fmt-merge-msg.c
> +++ b/fmt-merge-msg.c
> @@ -105,90 +105,92 @@ static void add_merge_parent(struct merge_parents *table,
>  static int handle_line(char *line, struct merge_parents *merge_parents)
>  {
>         int i, len = strlen(line);
>         struct origin_data *origin_data;
>         char *src;
>         const char *origin, *tag_name;
> +       char *to_free = NULL;
>         struct src_data *src_data;
>         struct string_list_item *item;
>         int pulling_head = 0;
>         struct object_id oid;
>         const unsigned hexsz = the_hash_algo->hexsz;
>
>         if (len < hexsz + 3 || line[hexsz] != '\t')
>                 return 1;
>
>         if (starts_with(line + hexsz + 1, "not-for-merge"))
>                 return 0;
>
>         if (line[hexsz + 1] != '\t')
>                 return 2;
>
>         i = get_oid_hex(line, &oid);
>         if (i)
>                 return 3;
>
>         if (!find_merge_parent(merge_parents, &oid, NULL))
>                 return 0; /* subsumed by other parents */
>
>         CALLOC_ARRAY(origin_data, 1);
>         oidcpy(&origin_data->oid, &oid);
>
>         if (line[len - 1] == '\n')
>                 line[len - 1] = 0;
>         line += hexsz + 2;
>
>         /*
>          * At this point, line points at the beginning of comment e.g.
>          * "branch 'frotz' of git://that/repository.git".
>          * Find the repository name and point it with src.
>          */
>         src = strstr(line, " of ");
>         if (src) {
>                 *src = 0;
>                 src += 4;
>                 pulling_head = 0;
>         } else {
>                 src = line;
>                 pulling_head = 1;
>         }
>
>         item = unsorted_string_list_lookup(&srcs, src);
>         if (!item) {
>                 item = string_list_append(&srcs, src);
>                 item->util = xcalloc(1, sizeof(struct src_data));
>                 init_src_data(item->util);
>         }
>         src_data = item->util;
>
>         if (pulling_head) {
>                 origin = src;
>                 src_data->head_status |= 1;
>         } else if (skip_prefix(line, "branch ", &origin)) {
>                 origin_data->is_local_branch = 1;
>                 string_list_append(&src_data->branch, origin);
>                 src_data->head_status |= 2;
>         } else if (skip_prefix(line, "tag ", &tag_name)) {
>                 origin = line;
>                 string_list_append(&src_data->tag, tag_name);
>                 src_data->head_status |= 2;
>         } else if (skip_prefix(line, "remote-tracking branch ", &origin)) {
>                 string_list_append(&src_data->r_branch, origin);
>                 src_data->head_status |= 2;
>         } else {
>                 origin = src;
>                 string_list_append(&src_data->generic, line);
>                 src_data->head_status |= 2;
>         }
>
>         if (!strcmp(".", src) || !strcmp(src, origin)) {
>                 int len = strlen(origin);
>                 if (origin[0] == '\'' && origin[len - 1] == '\'')
> -                       origin = xmemdupz(origin + 1, len - 2);
> +                       origin = to_free = xmemdupz(origin + 1, len - 2);
>         } else
> -               origin = xstrfmt("%s of %s", origin, src);
> +               origin = to_free = xstrfmt("%s of %s", origin, src);
>         if (strcmp(".", src))
>                 origin_data->is_local_branch = 0;
>         string_list_append(&origins, origin)->util = origin_data;
> +       free(to_free);
>         return 0;
>  }
>
> --
> 2.26.2

Makes sense.  The extended diff context makes this patch easier to
read and verify too; thanks.