From: René Scharfe Date: Wed, 31 Dec 2025 08:37:24 GMT Subject: Re: [PATCH] fsmonitor: fix khash memory leak in do_handle_client Message-ID: In-Reply-To: On 12/30/25 1:42 PM, Paul Tarjan via GitGitGadget wrote: > From: Paul Tarjan > > The do_handle_client() function allocates a khash table to de-duplicate > pathnames when responding to client requests. Two issues existed: > > 1. kh_release_str() was used instead of kh_destroy_str(). The release > function only frees internal arrays (flags, keys, vals) but not the > struct itself (allocated by kh_init_str via xcalloc). This caused a > 40-byte leak per request. > > 2. The khash was freed mid-function rather than in the cleanup section, > so if the worker thread was interrupted before reaching that point > during daemon shutdown, the memory would leak. > > Fix both issues by: > - Initializing shown = NULL at declaration > - Using kh_destroy_str() which handles NULL and frees both internal > arrays and the struct itself > - Moving the cleanup to the cleanup section so it runs on all exit paths > > Signed-off-by: Claude > --- > fsmonitor: fix khash memory leak in do_handle_client > > The do_handle_client() function allocates a khash table to de-duplicate > pathnames when responding to client requests. Two issues existed: > > 1. kh_release_str() was used instead of kh_destroy_str(). The release > function only frees internal arrays (flags, keys, vals) but not the > struct itself (allocated by kh_init_str via xcalloc). This caused a > 40-byte leak per request. Makes sense. > > 2. The khash was freed mid-function rather than in the cleanup section, > so if the worker thread was interrupted before reaching that point > during daemon shutdown, the memory would leak. Really? Would an interrupted thread even reach its cleanup section? > > Fix both issues by: > > * Initializing shown = NULL at declaration > * Using kh_destroy_str() which handles NULL and frees both internal > arrays and the struct itself > * Moving the cleanup to the cleanup section so it runs on all exit > paths > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2148%2Fptarjan%2Fclaude%2Ffix-fsmonitor-hashmap-leak-gfDCU-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2148/ptarjan/claude/fix-fsmonitor-hashmap-leak-gfDCU-v1 > Pull-Request: https://github.com/git/git/pull/2148 > > builtin/fsmonitor--daemon.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c > index 242c594646..bc4571938c 100644 > --- a/builtin/fsmonitor--daemon.c > +++ b/builtin/fsmonitor--daemon.c > @@ -671,7 +671,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state, > const struct fsmonitor_batch *batch; > struct fsmonitor_batch *remainder = NULL; > intmax_t count = 0, duplicates = 0; > - kh_str_t *shown; > + kh_str_t *shown = NULL; > int hash_ret; > int do_trivial = 0; > int do_flush = 0; > @@ -909,8 +909,6 @@ static int do_handle_client(struct fsmonitor_daemon_state *state, > total_response_len += payload.len; > } > > - kh_release_str(shown); > - > pthread_mutex_lock(&state->main_lock); > > if (token_data->client_ref_count > 0) > @@ -954,6 +952,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state, > trace2_data_intmax("fsmonitor", the_repository, "response/count/duplicates", duplicates); > > cleanup: > + kh_destroy_str(shown); > strbuf_release(&response_token); > strbuf_release(&requested_token_id); > strbuf_release(&payload); > > base-commit: 7c7698a654a7a0031f65b0ab0c1c4e438e95df60