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

Re: [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset

From
Elijah Newren <newren@gmail.com>
Date
Oct 4, 2021, 13:45 UTC
Message-ID
<CABPp-BH4ubjJ98Nvgp2iyKxmU9X+ypw4m1o=iL9Z4vSNZ-QTDw@mail.gmail.com>
In-Reply-To
<patch-04.10-9d62edd35bf-20211004T002226Z-avarab@gmail.com>
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 23 quoted lines
>
> Change the clear_unpack_trees_porcelain() to be like a *_release()
> function, not a *_reset() (in strbuf.c terms). Let's move the only API
> user that relied on the latter to doing its own
> unpack_trees_options_init(). See the commit that introduced
> unpack_trees_options_init() for details on the control flow involved
> here.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  merge-recursive.c | 1 +
>  unpack-trees.c    | 1 -
>  2 files changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index d24a4903f1d..a77f66b006c 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -442,6 +442,7 @@ static void unpack_trees_finish(struct merge_options *opt)
>  {
>         discard_index(&opt->priv->orig_index);
>         clear_unpack_trees_porcelain(&opt->priv->unpack_opts);
> +       unpack_trees_options_init(&opt->priv->unpack_opts);

This is wrong. It suggests that unpack_opts is used after unpack_trees_finish() (other than an outer merge first calling unpack_trees_start() again), which can only serve to greatly confuse future readers. Drop this hunk.

Show 12 quoted lines
>  }
>
>  static int save_files_dirs(const struct object_id *oid,
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 94767d3f96f..e7365322e82 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -197,7 +197,6 @@ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
>  {
>         strvec_clear(&opts->msgs_to_free);
>         dir_clear(&opts->dir);
> -       memset(opts->msgs, 0, sizeof(opts->msgs));

This seems like a very dangerous change. You want to leave opts->msgs pointing at freed memory?

Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 9 of 31 in “unpack-trees & dir APIs: fix memory leaks”
  1. 00/10 unpack-trees & dir APIs: fix memory leaksÆvar Arnfjörð Bjarmason, Oct 4, 2021
  2. 02/10 merge-recursive.c: call a new unpack_trees_options_init() functionÆvar Arnfjörð Bjarmason, Oct 4, 2021
  3. Elijah NewrenOct 4, 2021
  4. Ævar Arnfjörð BjarmasonOct 4, 2021
  5. Elijah NewrenOct 4, 2021
  6. 04/10 unpack-trees API: don't have clear_unpack_trees_porcelain() resetÆvar Arnfjörð Bjarmason, Oct 4, 2021
  7. Phillip WoodOct 4, 2021
  8. Ævar Arnfjörð BjarmasonOct 4, 2021
  9. Elijah NewrenOct 4, 2021
  10. Ævar Arnfjörð BjarmasonOct 4, 2021
  11. Elijah NewrenOct 4, 2021
  12. 09/10 merge.c: avoid duplicate unpack_trees_options_release() codeÆvar Arnfjörð Bjarmason, Oct 4, 2021
  13. Elijah NewrenOct 4, 2021
  14. Ævar Arnfjörð BjarmasonOct 4, 2021
  15. 03/10 unpack-trees.[ch]: embed "dir" in "struct unpack_trees_options"Ævar Arnfjörð Bjarmason, Oct 4, 2021
  16. Elijah NewrenOct 4, 2021
  17. 01/10 unpack-trees.[ch]: define and use a UNPACK_TREES_OPTIONS_INITÆvar Arnfjörð Bjarmason, Oct 4, 2021
  18. 05/10 dir.[ch]: make DIR_INIT mandatoryÆvar Arnfjörð Bjarmason, Oct 4, 2021
  19. Elijah NewrenOct 4, 2021
  20. 06/10 dir.c: get rid of lazy initializationÆvar Arnfjörð Bjarmason, Oct 4, 2021
  21. Elijah NewrenOct 4, 2021
  22. 07/10 unpack-trees API: rename clear_unpack_trees_porcelain()Ævar Arnfjörð Bjarmason, Oct 4, 2021
  23. Phillip WoodOct 4, 2021
  24. Ævar Arnfjörð BjarmasonOct 4, 2021
  25. Elijah NewrenOct 4, 2021
  26. 08/10 unpack-trees: don't leak memory in verify_clean_subdirectory()Ævar Arnfjörð Bjarmason, Oct 4, 2021
  27. Elijah NewrenOct 4, 2021
  28. 10/10 built-ins: plug memory leaks with unpack_trees_options_release()Ævar Arnfjörð Bjarmason, Oct 4, 2021
  29. Elijah NewrenOct 4, 2021
  30. Ævar Arnfjörð BjarmasonOct 4, 2021
  31. Elijah NewrenOct 4, 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.