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

Re: [PATCH] clear_pattern_list(): clear embedded hashmaps

From
Elijah Newren <newren@gmail.com>
Date
Aug 17, 2020, 17:22 UTC
Message-ID
<CABPp-BGUBPJCW3-GrVvom5CwfauY-m5ZCLJmKP=P=fXWm=Kepg@mail.gmail.com>
In-Reply-To
<CABPp-BGrxaY-ObzuJ60TDMfQD+crq+TTnYE03T=KVmG5mEf-5A@mail.gmail.com>
On Mon, Aug 17, 2020 at 9:55 AM Elijah Newren <newren@gmail.com> wrote:
Show 35 quoted lines
>
> Hi,
>
> On Fri, Aug 14, 2020 at 5:23 AM Jeff King <peff@peff.net> wrote:
> >
> > Commit 96cc8ab531 (sparse-checkout: use hashmaps for cone patterns,
> > 2019-11-21) added some auxiliary hashmaps to the pattern_list struct,
> > but they're leaked when clear_pattern_list() is called.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > I have no idea how often this leak triggers in practice. I just noticed
> > it while poking at LSan output (which we remain depressingly far
> > from getting a clean run on).
> >
> >  dir.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/dir.c b/dir.c
> > index fe64be30ed..9411b94e9b 100644
> > --- a/dir.c
> > +++ b/dir.c
> > @@ -916,6 +916,8 @@ void clear_pattern_list(struct pattern_list *pl)
> >                 free(pl->patterns[i]);
> >         free(pl->patterns);
> >         free(pl->filebuf);
> > +       hashmap_free_entries(&pl->recursive_hashmap, struct pattern_entry, ent);
> > +       hashmap_free_entries(&pl->parent_hashmap, struct pattern_entry, ent);
>
> This clears up the hash entries, but continues to leak the hash table.
> Since you submitted first, can you fix this to use hashmap_free_()
> instead, as per
> https://lore.kernel.org/git/932741d7598ca2934dbca40f715ba2d3819fcc51.1597561152.git.gitgitgadget@gmail.com/?
>  Then I'll rebase my series on yours and drop my first patch (since
> it'll then be identical).

Nevermind, I got confused once again by the name. hashmap_free_entries() doesn't mean just free the entries, it means free what hashmap_free() would plus all the entries, i.e. do what hashmap_free() *should* *have* *been* defined to do. Such a confusing API. And hashmap_free() really perplexes me -- it seems like a function that can't possibly be useful; it's sole purpose seems to be a trap for the unwary.

Previous: Elijah NewrenNext: Jeff King
Message 4 of 5 in “clear_pattern_list(): clear embedded hashmaps”
  1. clear_pattern_list(): clear embedded hashmapsJeff King, Aug 14, 2020
  2. Derrick StoleeAug 14, 2020
  3. Elijah NewrenAug 17, 2020
  4. Elijah NewrenAug 17, 2020
  5. Jeff KingAug 17, 2020

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.