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