{"thread":{"id":"63369","subject":"[PATCH] hashmap: ensure hashmaps are reusable after hashmap_clear()","startedAt":"2025-04-29T15:47:46Z","lastAt":"2025-04-29T16:55:21Z","messageCount":2,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"516961","messageId":"pull.1911.git.1745941663160.gitgitgadget@gmail.com","threadId":"63369","inReplyTo":null,"subject":"[PATCH] hashmap: ensure hashmaps are reusable after hashmap_clear()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-04-29T15:47:43Z","receivedAt":"2025-04-29T15:47:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nIn the series merged at bf0a430f70b5 (Merge branch 'en/strmap',\n2020-11-21), strmap was built on top of hashmap and hashmap was extended\nin a few ways to support strmap and be more generally useful.  One of\nthe extensions was that hashmap_partial_clear() was introduced to allow\nreuse of the hashmap without freeing the table.  Peff believed that it\nalso made sense to introduce a hashmap_clear() which freed everything\nwhile allowing reuse.\n\nI added hashmap_clear(), but in doing so, overlooked the fact that for\na hashmap to be reusable, it needs a defined cmpfn and data (the\nHASHMAP_INIT macro requires these fields as parameters, for example).\nSo, if we want the hashmap to be reusable, we shouldn't zero out those\nfields.  We probably also shouldn't zero out do_count_items.  (We could\nzero out grow_at and shrink_at, but whether we zero those or not is\nirrelevant as they'll be automatically updated whenever a new entry is\ninserted.)\n\nSince clearing is associated with freeing map->table, and the only thing\nrequired for consistency after freeing map->table is zeroing tablesize\nand private_size, let's only zero those fields out.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    hashmap: ensure hashmaps are reusable after hashmap_clear()\n    \n    Ran into a NULL pointer dereference of cmpfn a few months ago when\n    trying to reuse one of {strmap, strset, strintmap} (don't remember which\n    one) after calling the relevant ${TYPE}_clear() variant, and tracked the\n    NULL pointer back to hashmap_clear(). Turned out to not be relevant to\n    those patches because I ended up not needing to reuse the map after all,\n    but I kept a note to myself to send in a fix.\n    \n    I was surprised this wasn't a bug we were already hitting somewhere, but\n    I looked through the codebase and it appears that the only time we\n    attempt to reuse a hashmap after clearing is when we specifically use\n    hashmap_partial_clear(). So, this is just a latent bug waiting as a trap\n    for someone.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1911%2Fnewren%2Ffix-hashmap-clear-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1911/newren/fix-hashmap-clear-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1911\n\n hashmap.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/hashmap.c b/hashmap.c\nindex ee45ef00852..a711377853f 100644\n--- a/hashmap.c\n+++ b/hashmap.c\n@@ -205,8 +205,9 @@ void hashmap_clear_(struct hashmap *map, ssize_t entry_offset)\n \t\treturn;\n \tif (entry_offset >= 0)  /* called by hashmap_clear_and_free */\n \t\tfree_individual_entries(map, entry_offset);\n-\tfree(map->table);\n-\tmemset(map, 0, sizeof(*map));\n+\tFREE_AND_NULL(map->table);\n+\tmap->tablesize = 0;\n+\tmap->private_size = 0;\n }\n \n struct hashmap_entry *hashmap_get(const struct hashmap *map,\n\nbase-commit: f65182a99e545d2f2bc22e6c1c2da192133b16a3\n-- \ngitgitgadget\n"},{"id":"516967","messageId":"xmqqwmb26h6x.fsf@gitster.g","threadId":"63369","inReplyTo":"pull.1911.git.1745941663160.gitgitgadget@gmail.com","subject":"Re: [PATCH] hashmap: ensure hashmaps are reusable after hashmap_clear()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-29T16:55:18Z","receivedAt":"2025-04-29T16:55:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Elijah Newren <newren@gmail.com>\n>\n> In the series merged at bf0a430f70b5 (Merge branch 'en/strmap',\n> 2020-11-21), strmap was built on top of hashmap and hashmap was extended\n> in a few ways to support strmap and be more generally useful.  One of\n> the extensions was that hashmap_partial_clear() was introduced to allow\n> reuse of the hashmap without freeing the table.  Peff believed that it\n> also made sense to introduce a hashmap_clear() which freed everything\n> while allowing reuse.\n>\n> I added hashmap_clear(), but in doing so, overlooked the fact that for\n> a hashmap to be reusable, it needs a defined cmpfn and data (the\n> HASHMAP_INIT macro requires these fields as parameters, for example).\n> So, if we want the hashmap to be reusable, we shouldn't zero out those\n> fields.  We probably also shouldn't zero out do_count_items.  (We could\n> zero out grow_at and shrink_at, but whether we zero those or not is\n> irrelevant as they'll be automatically updated whenever a new entry is\n> inserted.)\n>\n> Since clearing is associated with freeing map->table, and the only thing\n> required for consistency after freeing map->table is zeroing tablesize\n> and private_size, let's only zero those fields out.\n\nMakes sense.  Thanks for finding and fixing.\n\nI do not think we want to patch all the way down to Git 2.30, ...\n\n> diff --git a/hashmap.c b/hashmap.c\n> index ee45ef00852..a711377853f 100644\n> --- a/hashmap.c\n> +++ b/hashmap.c\n> @@ -205,8 +205,9 @@ void hashmap_clear_(struct hashmap *map, ssize_t entry_offset)\n>  \t\treturn;\n>  \tif (entry_offset >= 0)  /* called by hashmap_clear_and_free */\n>  \t\tfree_individual_entries(map, entry_offset);\n> -\tfree(map->table);\n> -\tmemset(map, 0, sizeof(*map));\n> +\tFREE_AND_NULL(map->table);\n> +\tmap->tablesize = 0;\n> +\tmap->private_size = 0;\n>  }\n>  \n>  struct hashmap_entry *hashmap_get(const struct hashmap *map,\n>\n> base-commit: f65182a99e545d2f2bc22e6c1c2da192133b16a3\n\n... but this part of the code has been fairly quiet and the patch\napplies very cleanly.  So I'll apply on top of bf0a430f7 and merge\nthe result---anybody maintaining Git for their LTS distro can then\nmerge it to their favorite ancient maintenance track ;-)\n\nThanks.\n\n\n\n"}]}