Re: [PATCH] fsmonitor: fix khash memory leak in do_handle_client
From: René Scharfe <hidden>
Date: 2025-12-31 08:37:40
On 12/30/25 1:42 PM, Paul Tarjan via GitGitGadget wrote:
From: Paul Tarjan <redacted>
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 <redacted>
---
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?
quoted hunk ↗ jump to hunk
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