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

Re: [PATCH 07/12] read-cache: call diff_setup_done to avoid leak

From
Elijah Newren <newren@gmail.com>
Date
Jun 21, 2021, 21:17 UTC
Message-ID
<CABPp-BGBk8qT+ApEkaDMF4zqK5ZeW07fTjQUuNz6c6z=oQd2eQ@mail.gmail.com>
In-Reply-To
<20210620151204.19260-8-andrzej@ahunt.org>
On Sun, Jun 20, 2021 at 8:15 AM <andrzej@ahunt.org> wrote:
Show 8 quoted lines
>
> From: Andrzej Hunt <ajrhunt@google.com>
>
> repo_diff_setup() calls through to diff.c's static prep_parse_options(),
> which in  turn allocates a new array into diff_opts.parseopts.
> diff_setup_done() is responsible for freeing that array, and has the
> benefit of verifying diff_opts too - hence we add a call to
> diff_setup_done() to avoid leaking parseopts.

Should the documentation near the top of diff.h also point out that part of the purpose of diff_setup_done() is to free some memory?

Show 71 quoted lines
> Output from the leak as found while running t0090 with LSAN:
>
> Direct leak of 7120 byte(s) in 1 object(s) allocated from:
>     #0 0x49a82d in malloc ../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
>     #1 0xa8bf89 in do_xmalloc wrapper.c:41:8
>     #2 0x7a7bae in prep_parse_options diff.c:5636:2
>     #3 0x7a7bae in repo_diff_setup diff.c:4611:2
>     #4 0x93716c in repo_index_has_changes read-cache.c:2518:3
>     #5 0x872233 in unclean merge-ort-wrappers.c:12:14
>     #6 0x872233 in merge_ort_recursive merge-ort-wrappers.c:53:6
>     #7 0x5d5b11 in try_merge_strategy builtin/merge.c:752:12
>     #8 0x5d0b6b in cmd_merge builtin/merge.c:1666:9
>     #9 0x4ce83e in run_builtin git.c:475:11
>     #10 0x4ccafe in handle_builtin git.c:729:3
>     #11 0x4cb01c in run_argv git.c:818:4
>     #12 0x4cb01c in cmd_main git.c:949:19
>     #13 0x6bdc2d in main common-main.c:52:11
>     #14 0x7f551eb51349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>
> SUMMARY: AddressSanitizer: 7120 byte(s) leaked in 1 allocation(s)
>
> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
> ---
>  read-cache.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index 77961a3885..212d604dd3 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2487,37 +2487,38 @@ int unmerged_index(const struct index_state *istate)
>  int repo_index_has_changes(struct repository *repo,
>                            struct tree *tree,
>                            struct strbuf *sb)
>  {
>         struct index_state *istate = repo->index;
>         struct object_id cmp;
>         int i;
>
>         if (tree)
>                 cmp = tree->object.oid;
>         if (tree || !get_oid_tree("HEAD", &cmp)) {
>                 struct diff_options opt;
>
>                 repo_diff_setup(repo, &opt);
>                 opt.flags.exit_with_status = 1;
>                 if (!sb)
>                         opt.flags.quick = 1;
> +               diff_setup_done(&opt);
>                 do_diff_cache(&cmp, &opt);
>                 diffcore_std(&opt);
>                 for (i = 0; sb && i < diff_queued_diff.nr; i++) {
>                         if (i)
>                                 strbuf_addch(sb, ' ');
>                         strbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);
>                 }
>                 diff_flush(&opt);
>                 return opt.flags.has_changes != 0;
>         } else {
>                 /* TODO: audit for interaction with sparse-index. */
>                 ensure_full_index(istate);
>                 for (i = 0; sb && i < istate->cache_nr; i++) {
>                         if (i)
>                                 strbuf_addch(sb, ' ');
>                         strbuf_addstr(sb, istate->cache[i]->name);
>                 }
>                 return !!istate->cache_nr;
>         }
>  }
> --
> 2.26.2

Patch makes sense; a quick `git grep -e repo_diff_setup -e diff_setup_done` doesn't flag any other areas of the code as having the same bug.

Previous: andrzej@ahunt.orgNext: andrzej@ahunt.org
Message 15 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.