git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Elijah Newren <newren@gmail.com>
Date
Jun 21, 2021, 20:34 UTC
Message-ID
<CABPp-BH_35UkaXhaBeo_SVPcyRk=OuENpGr+L3Jkycz6RNh1LQ@mail.gmail.com>
In-Reply-To
<20210620151204.19260-2-andrzej@ahunt.org>
On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote:
Show 138 quoted lines
>
> 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.

Previous: andrzej@ahunt.orgNext: andrzej@ahunt.org
Message 3 of 51 in “Fix all leaks in tests t0002-t0099: Part 2”
  1. 00/12 Fix all leaks in tests t0002-t0099: Part 2andrzej@ahunt.org, Jun 20, 2021
  2. 01/12 fmt-merge-msg: free newly allocated temporary strings when doneandrzej@ahunt.org, Jun 20, 2021
  3. Elijah NewrenJun 21, 2021
  4. 02/12 environment: move strbuf into block to plug leakandrzej@ahunt.org, Jun 20, 2021
  5. Elijah NewrenJun 21, 2021
  6. René ScharfeJun 26, 2021
  7. 03/12 builtin/submodule--helper: release unused strbuf to avoid leakandrzej@ahunt.org, Jun 20, 2021
  8. 04/12 builtin/for-each-repo: remove unnecessary argv copy to plug leakandrzej@ahunt.org, Jun 20, 2021
  9. Elijah NewrenJun 21, 2021
  10. 05/12 diffcore-rename: move old_dir/new_dir definition to plug leakandrzej@ahunt.org, Jun 20, 2021
  11. Elijah NewrenJun 21, 2021
  12. 06/12 ref-filter: also free head for ATOM_HEAD to avoid leakandrzej@ahunt.org, Jun 20, 2021
  13. Elijah NewrenJun 21, 2021
  14. 07/12 read-cache: call diff_setup_done to avoid leakandrzej@ahunt.org, Jun 20, 2021
  15. Elijah NewrenJun 21, 2021
  16. 08/12 convert: release strbuf to avoid leakandrzej@ahunt.org, Jun 20, 2021
  17. Elijah NewrenJun 21, 2021
  18. 09/12 builtin/mv: free or UNLEAK multiple pointers at end of cmd_mvandrzej@ahunt.org, Jun 20, 2021
  19. 10/12 builtin/merge: free found_ref when doneandrzej@ahunt.org, Jun 20, 2021
  20. Elijah NewrenJun 21, 2021
  21. 11/12 builtin/rebase: fix options.strategy memory lifecycleandrzej@ahunt.org, Jun 20, 2021
  22. Phillip WoodJun 20, 2021
  23. Elijah NewrenJun 21, 2021
  24. Phillip WoodJun 22, 2021
  25. Andrzej HuntJul 25, 2021
  26. Phillip WoodJul 27, 2021
  27. 12/12 reset: clear_unpack_trees_porcelain to plug leakandrzej@ahunt.org, Jun 20, 2021
  28. Elijah NewrenJun 21, 2021
  29. Elijah NewrenJun 21, 2021
  30. Andrzej HuntJul 25, 2021
  31. Christian CouderJul 26, 2021
  32. 00/12 Fix all leaks in tests t0002-t0099: Part 2andrzej@ahunt.org, Jul 25, 2021
  33. 01/12 fmt-merge-msg: free newly allocated temporary strings when doneandrzej@ahunt.org, Jul 25, 2021
  34. Junio C HamanoJul 26, 2021
  35. 02/12 environment: move strbuf into block to plug leakandrzej@ahunt.org, Jul 25, 2021
  36. 03/12 builtin/submodule--helper: release unused strbuf to avoid leakandrzej@ahunt.org, Jul 25, 2021
  37. 04/12 builtin/for-each-repo: remove unnecessary argv copy to plug leakandrzej@ahunt.org, Jul 25, 2021
  38. Junio C HamanoJul 26, 2021
  39. 06/12 ref-filter: also free head for ATOM_HEAD to avoid leakandrzej@ahunt.org, Jul 25, 2021
  40. Junio C HamanoJul 26, 2021
  41. 07/12 read-cache: call diff_setup_done to avoid leakandrzej@ahunt.org, Jul 25, 2021
  42. Junio C HamanoJul 26, 2021
  43. 05/12 diffcore-rename: move old_dir/new_dir definition to plug leakandrzej@ahunt.org, Jul 25, 2021
  44. Junio C HamanoJul 26, 2021
  45. 09/12 builtin/mv: free or UNLEAK multiple pointers at end of cmd_mvandrzej@ahunt.org, Jul 25, 2021
  46. 08/12 convert: release strbuf to avoid leakandrzej@ahunt.org, Jul 25, 2021
  47. Junio C HamanoJul 26, 2021
  48. 10/12 builtin/merge: free found_ref when doneandrzej@ahunt.org, Jul 25, 2021
  49. 12/12 reset: clear_unpack_trees_porcelain to plug leakandrzej@ahunt.org, Jul 25, 2021
  50. 11/12 builtin/rebase: fix options.strategy memory lifecycleandrzej@ahunt.org, Jul 25, 2021
  51. Junio C HamanoJul 26, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.