{"thread":{"id":"64698","subject":"[PATCH] fsmonitor: fix khash memory leak in do_handle_client","startedAt":"2025-12-30T12:42:58Z","lastAt":"2026-01-04T02:19:21Z","messageCount":6,"participants":["Paul Tarjan via GitGitGadget","René Scharfe","Junio C Hamano","Paul Tarjan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"532827","messageId":"pull.2148.git.git.1767098576384.gitgitgadget@gmail.com","threadId":"64698","inReplyTo":null,"subject":"[PATCH] fsmonitor: fix khash memory leak in do_handle_client","fromName":"Paul Tarjan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-12-30T12:42:56Z","receivedAt":"2025-12-30T12:42:58Z","isPatch":true,"sender":{"key":"paul@paultarjan.com","avatar":null},"body":"From: Paul Tarjan \n\nThe do_handle_client() function allocates a khash table to de-duplicate\npathnames when responding to client requests. Two issues existed:\n\n1. kh_release_str() was used instead of kh_destroy_str(). The release\n function only frees internal arrays (flags, keys, vals) but not the\n struct itself (allocated by kh_init_str via xcalloc). This caused a\n 40-byte leak per request.\n\n2. The khash was freed mid-function rather than in the cleanup section,\n so if the worker thread was interrupted before reaching that point\n during daemon shutdown, the memory would leak.\n\nFix both issues by:\n- Initializing shown = NULL at declaration\n- Using kh_destroy_str() which handles NULL and frees both internal\n arrays and the struct itself\n- Moving the cleanup to the cleanup section so it runs on all exit paths\n\nSigned-off-by: Claude \n---\n fsmonitor: fix khash memory leak in do_handle_client\n \n The do_handle_client() function allocates a khash table to de-duplicate\n pathnames when responding to client requests. Two issues existed:\n \n 1. kh_release_str() was used instead of kh_destroy_str(). The release\n function only frees internal arrays (flags, keys, vals) but not the\n struct itself (allocated by kh_init_str via xcalloc). This caused a\n 40-byte leak per request.\n \n 2. The khash was freed mid-function rather than in the cleanup section,\n so if the worker thread was interrupted before reaching that point\n during daemon shutdown, the memory would leak.\n \n Fix both issues by:\n \n * Initializing shown = NULL at declaration\n * Using kh_destroy_str() which handles NULL and frees both internal\n arrays and the struct itself\n * Moving the cleanup to the cleanup section so it runs on all exit\n paths\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2148%2Fptarjan%2Fclaude%2Ffix-fsmonitor-hashmap-leak-gfDCU-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2148/ptarjan/claude/fix-fsmonitor-hashmap-leak-gfDCU-v1\nPull-Request: https://github.com/git/git/pull/2148\n\n builtin/fsmonitor--daemon.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\nindex 242c594646..bc4571938c 100644\n--- a/builtin/fsmonitor--daemon.c\n+++ b/builtin/fsmonitor--daemon.c\n@@ -671,7 +671,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,\n \tconst struct fsmonitor_batch *batch;\n \tstruct fsmonitor_batch *remainder = NULL;\n \tintmax_t count = 0, duplicates = 0;\n-\tkh_str_t *shown;\n+\tkh_str_t *shown = NULL;\n \tint hash_ret;\n \tint do_trivial = 0;\n \tint do_flush = 0;\n@@ -909,8 +909,6 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,\n \t\ttotal_response_len += payload.len;\n \t}\n \n-\tkh_release_str(shown);\n-\n \tpthread_mutex_lock(&state->main_lock);\n \n \tif (token_data->client_ref_count > 0)\n@@ -954,6 +952,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,\n \ttrace2_data_intmax(\"fsmonitor\", the_repository, \"response/count/duplicates\", duplicates);\n \n cleanup:\n+\tkh_destroy_str(shown);\n \tstrbuf_release(&response_token);\n \tstrbuf_release(&requested_token_id);\n \tstrbuf_release(&payload);\n\nbase-commit: 7c7698a654a7a0031f65b0ab0c1c4e438e95df60\n-- \ngitgitgadget\n"},{"id":"532864","messageId":"ebb877bb-c86f-4ca1-b7b7-b236fb95848e@web.de","threadId":"64698","inReplyTo":"pull.2148.git.git.1767098576384.gitgitgadget@gmail.com","subject":"Re: [PATCH] fsmonitor: fix khash memory leak in do_handle_client","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-12-31T08:37:24Z","receivedAt":"2025-12-31T08:37:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 12/30/25 1:42 PM, Paul Tarjan via GitGitGadget wrote:\n> From: Paul Tarjan \n> \n> The do_handle_client() function allocates a khash table to de-duplicate\n> pathnames when responding to client requests. Two issues existed:\n> \n> 1. kh_release_str() was used instead of kh_destroy_str(). The release\n> function only frees internal arrays (flags, keys, vals) but not the\n> struct itself (allocated by kh_init_str via xcalloc). This caused a\n> 40-byte leak per request.\n> \n> 2. The khash was freed mid-function rather than in the cleanup section,\n> so if the worker thread was interrupted before reaching that point\n> during daemon shutdown, the memory would leak.\n> \n> Fix both issues by:\n> - Initializing shown = NULL at declaration\n> - Using kh_destroy_str() which handles NULL and frees both internal\n> arrays and the struct itself\n> - Moving the cleanup to the cleanup section so it runs on all exit paths\n> \n> Signed-off-by: Claude \n> ---\n> fsmonitor: fix khash memory leak in do_handle_client\n> \n> The do_handle_client() function allocates a khash table to de-duplicate\n> pathnames when responding to client requests. Two issues existed:\n> \n> 1. kh_release_str() was used instead of kh_destroy_str(). The release\n> function only frees internal arrays (flags, keys, vals) but not the\n> struct itself (allocated by kh_init_str via xcalloc). This caused a\n> 40-byte leak per request.\n\nMakes sense.\n\n> \n> 2. The khash was freed mid-function rather than in the cleanup section,\n> so if the worker thread was interrupted before reaching that point\n> during daemon shutdown, the memory would leak.\n\nReally? Would an interrupted thread even reach its cleanup section?\n\n> \n> Fix both issues by:\n> \n> * Initializing shown = NULL at declaration\n> * Using kh_destroy_str() which handles NULL and frees both internal\n> arrays and the struct itself\n> * Moving the cleanup to the cleanup section so it runs on all exit\n> paths\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2148%2Fptarjan%2Fclaude%2Ffix-fsmonitor-hashmap-leak-gfDCU-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2148/ptarjan/claude/fix-fsmonitor-hashmap-leak-gfDCU-v1\n> Pull-Request: https://github.com/git/git/pull/2148\n> \n> builtin/fsmonitor--daemon.c | 5 ++---\n> 1 file changed, 2 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\n> index 242c594646..bc4571938c 100644\n> --- a/builtin/fsmonitor--daemon.c\n> +++ b/builtin/fsmonitor--daemon.c\n> @@ -671,7 +671,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,\n> \tconst struct fsmonitor_batch *batch;\n> \tstruct fsmonitor_batch *remainder = NULL;\n> \tintmax_t count = 0, duplicates = 0;\n> -\tkh_str_t *shown;\n> +\tkh_str_t *shown = NULL;\n> \tint hash_ret;\n> \tint do_trivial = 0;\n> \tint do_flush = 0;\n> @@ -909,8 +909,6 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,\n> \t\ttotal_response_len += payload.len;\n> \t}\n> \n> -\tkh_release_str(shown);\n> -\n> \tpthread_mutex_lock(&state->main_lock);\n> \n> \tif (token_data->client_ref_count > 0)\n> @@ -954,6 +952,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,\n> \ttrace2_data_intmax(\"fsmonitor\", the_repository, \"response/count/duplicates\", duplicates);\n> \n> cleanup:\n> +\tkh_destroy_str(shown);\n> \tstrbuf_release(&response_token);\n> \tstrbuf_release(&requested_token_id);\n> \tstrbuf_release(&payload);\n> \n> base-commit: 7c7698a654a7a0031f65b0ab0c1c4e438e95df60\n\n"},{"id":"532865","messageId":"pull.2148.v2.git.git.1767191943962.gitgitgadget@gmail.com","threadId":"64698","inReplyTo":"pull.2148.git.git.1767098576384.gitgitgadget@gmail.com","subject":"[PATCH v2] fsmonitor: fix khash memory leak in do_handle_client","fromName":"Paul Tarjan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-12-31T14:39:03Z","receivedAt":"2025-12-31T14:39:07Z","isPatch":true,"sender":{"key":"paul@paultarjan.com","avatar":null},"body":"From: Paul Tarjan \n\nThe do_handle_client() function allocates a khash table to de-duplicate\npathnames when responding to client requests. However, kh_release_str()\nwas used instead of kh_destroy_str(). The release function only frees\ninternal arrays (flags, keys, vals) but not the struct itself, which is\nallocated by kh_init_str() via xcalloc. This caused a 40-byte leak per\nclient request.\n\nFix by using kh_destroy_str() which properly frees both internal arrays\nand the struct itself. Also move the cleanup to the cleanup section and\ninitialize shown to NULL so that kh_destroy_str() is safe to call on all\nexit paths.\n\nSigned-off-by: Paul Tarjan \n---\n fsmonitor: fix khash memory leak in do_handle_client\n \n The do_handle_client() function allocates a khash table to de-duplicate\n pathnames when responding to client requests. However, kh_release_str()\n was used instead of kh_destroy_str(). The release function only frees\n internal arrays (flags, keys, vals) but not the struct itself, which is\n allocated by kh_init_str() via xcalloc. This caused a 40-byte leak per\n client request.\n \n Fix by using kh_destroy_str() which properly frees both internal arrays\n and the struct itself. Also move the cleanup to the cleanup section and\n initialize shown to NULL so that kh_destroy_str() is safe to call on all\n exit paths.\n \n Changes since v1:\n \n * Removed incorrect claim about interrupted threads reaching cleanup\n * Simplified commit message to focus on the real bug (kh_release vs\n kh_destroy)\n \n Signed-off-by: Paul Tarjan github@paulisageek.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2148%2Fptarjan%2Fclaude%2Ffix-fsmonitor-hashmap-leak-gfDCU-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2148/ptarjan/claude/fix-fsmonitor-hashmap-leak-gfDCU-v2\nPull-Request: https://github.com/git/git/pull/2148\n\nRange-diff vs v1:\n\n 1: 18670d3230 ! 1: 0a754c7f09 fsmonitor: fix khash memory leak in do_handle_client\n @@ Commit message\n fsmonitor: fix khash memory leak in do_handle_client\n \n The do_handle_client() function allocates a khash table to de-duplicate\n - pathnames when responding to client requests. Two issues existed:\n + pathnames when responding to client requests. However, kh_release_str()\n + was used instead of kh_destroy_str(). The release function only frees\n + internal arrays (flags, keys, vals) but not the struct itself, which is\n + allocated by kh_init_str() via xcalloc. This caused a 40-byte leak per\n + client request.\n \n - 1. kh_release_str() was used instead of kh_destroy_str(). The release\n - function only frees internal arrays (flags, keys, vals) but not the\n - struct itself (allocated by kh_init_str via xcalloc). This caused a\n - 40-byte leak per request.\n + Fix by using kh_destroy_str() which properly frees both internal arrays\n + and the struct itself. Also move the cleanup to the cleanup section and\n + initialize shown to NULL so that kh_destroy_str() is safe to call on all\n + exit paths.\n \n - 2. The khash was freed mid-function rather than in the cleanup section,\n - so if the worker thread was interrupted before reaching that point\n - during daemon shutdown, the memory would leak.\n -\n - Fix both issues by:\n - - Initializing shown = NULL at declaration\n - - Using kh_destroy_str() which handles NULL and frees both internal\n - arrays and the struct itself\n - - Moving the cleanup to the cleanup section so it runs on all exit paths\n -\n - Signed-off-by: Claude \n + Signed-off-by: Paul Tarjan \n \n ## builtin/fsmonitor--daemon.c ##\n @@ builtin/fsmonitor--daemon.c: static int do_handle_client(struct fsmonitor_daemon_state *state,\n\n\n builtin/fsmonitor--daemon.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\nindex 242c594646..bc4571938c 100644\n--- a/builtin/fsmonitor--daemon.c\n+++ b/builtin/fsmonitor--daemon.c\n@@ -671,7 +671,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,\n \tconst struct fsmonitor_batch *batch;\n \tstruct fsmonitor_batch *remainder = NULL;\n \tintmax_t count = 0, duplicates = 0;\n-\tkh_str_t *shown;\n+\tkh_str_t *shown = NULL;\n \tint hash_ret;\n \tint do_trivial = 0;\n \tint do_flush = 0;\n@@ -909,8 +909,6 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,\n \t\ttotal_response_len += payload.len;\n \t}\n \n-\tkh_release_str(shown);\n-\n \tpthread_mutex_lock(&state->main_lock);\n \n \tif (token_data->client_ref_count > 0)\n@@ -954,6 +952,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,\n \ttrace2_data_intmax(\"fsmonitor\", the_repository, \"response/count/duplicates\", duplicates);\n \n cleanup:\n+\tkh_destroy_str(shown);\n \tstrbuf_release(&response_token);\n \tstrbuf_release(&requested_token_id);\n \tstrbuf_release(&payload);\n\nbase-commit: 7c7698a654a7a0031f65b0ab0c1c4e438e95df60\n-- \ngitgitgadget\n"},{"id":"532893","messageId":"xmqqbjjdc4d0.fsf@gitster.g","threadId":"64698","inReplyTo":"pull.2148.v2.git.git.1767191943962.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] fsmonitor: fix khash memory leak in do_handle_client","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-01T23:14:51Z","receivedAt":"2026-01-01T23:14:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Paul Tarjan via GitGitGadget\" writes:\n\n> From: Paul Tarjan \n>\n> The do_handle_client() function allocates a khash table to de-duplicate\n> pathnames when responding to client requests. However, kh_release_str()\n> was used instead of kh_destroy_str(). The release function only frees\n> internal arrays (flags, keys, vals) but not the struct itself, which is\n> allocated by kh_init_str() via xcalloc. This caused a 40-byte leak per\n> client request.\n>\n> Fix by using kh_destroy_str() which properly frees both internal arrays\n> and the struct itself. Also move the cleanup to the cleanup section and\n> initialize shown to NULL so that kh_destroy_str() is safe to call on all\n> exit paths.\n>\n> Signed-off-by: Paul Tarjan \n> ---\n\nThis is already in v4 of the other larger fsmonitor-linux patch,\nright?\n\nThanks.\n"},{"id":"532897","messageId":"CALvWuB6MW8g3Vd1-gKkZa_r86o0-t3gGazLSvsoBO4m6jjubeQ@mail.gmail.com","threadId":"64698","inReplyTo":"xmqqbjjdc4d0.fsf@gitster.g","subject":"Re: [PATCH v2] fsmonitor: fix khash memory leak in do_handle_client","fromName":"Paul Tarjan","fromEmail":"paul@paultarjan.com","sentAt":"2026-01-02T01:24:27Z","receivedAt":"2026-01-02T01:24:39Z","isPatch":true,"sender":{"key":"paul@paultarjan.com","avatar":null},"body":"On Thu, Jan 1, 2026 at 1:14 PM Junio C Hamano wrote:\n>\n> \"Paul Tarjan via GitGitGadget\" writes:\n>\n> > From: Paul Tarjan \n> >\n> > The do_handle_client() function allocates a khash table to de-duplicate\n> > pathnames when responding to client requests. However, kh_release_str()\n> > was used instead of kh_destroy_str(). The release function only frees\n> > internal arrays (flags, keys, vals) but not the struct itself, which is\n> > allocated by kh_init_str() via xcalloc. This caused a 40-byte leak per\n> > client request.\n> >\n> > Fix by using kh_destroy_str() which properly frees both internal arrays\n> > and the struct itself. Also move the cleanup to the cleanup section and\n> > initialize shown to NULL so that kh_destroy_str() is safe to call on all\n> > exit paths.\n> >\n> > Signed-off-by: Paul Tarjan \n> > ---\n>\n> This is already in v4 of the other larger fsmonitor-linux patch,\n> right?\n\nCorrect. I sent it separately since it is currently a bug in existing\ncode and you might want to merge it sooner.\n\n>\n> Thanks.\n>\n"},{"id":"532961","messageId":"xmqq344mqfvd.fsf@gitster.g","threadId":"64698","inReplyTo":"CALvWuB6MW8g3Vd1-gKkZa_r86o0-t3gGazLSvsoBO4m6jjubeQ@mail.gmail.com","subject":"Re: [PATCH v2] fsmonitor: fix khash memory leak in do_handle_client","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-04T02:19:18Z","receivedAt":"2026-01-04T02:19:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tarjan writes:\n\n> On Thu, Jan 1, 2026 at 1:14 PM Junio C Hamano wrote:\n>>\n>> \"Paul Tarjan via GitGitGadget\" writes:\n>>\n>> > From: Paul Tarjan \n>> >\n>> > The do_handle_client() function allocates a khash table to de-duplicate\n>> > pathnames when responding to client requests. However, kh_release_str()\n>> > was used instead of kh_destroy_str(). The release function only frees\n>> > internal arrays (flags, keys, vals) but not the struct itself, which is\n>> > allocated by kh_init_str() via xcalloc. This caused a 40-byte leak per\n>> > client request.\n>> >\n>> > Fix by using kh_destroy_str() which properly frees both internal arrays\n>> > and the struct itself. Also move the cleanup to the cleanup section and\n>> > initialize shown to NULL so that kh_destroy_str() is safe to call on all\n>> > exit paths.\n>> >\n>> > Signed-off-by: Paul Tarjan \n>> > ---\n>>\n>> This is already in v4 of the other larger fsmonitor-linux patch,\n>> right?\n>\n> Correct. I sent it separately since it is currently a bug in existing\n> code and you might want to merge it sooner.\n\nThanks.\n"}]}