| Message ID | 20210620151204.19260-2-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> > > 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.
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; }