Re: [PATCH v6 01/10] fsmonitor: fix khash memory leak in do_handle_client
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 25, 2026, 21:01 UTC
- Message-ID
- <xmqq4in4a673.fsf@gitster.g>
- In-Reply-To
- <4d4dec8fa161926e6f6ac822aff0db35353705eb.1772050636.git.gitgitgadget@gmail.com>
"Paul Tarjan via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 12 quoted lines
> From: Paul Tarjan <github@paulisageek.com> > > The `shown` kh_str_t was freed with kh_release_str() at a point in > the code only reachable in the non-trivial response path. When the > client receives a trivial response, the code jumps to the `cleanup` > label, skipping the kh_release_str() call entirely and leaking the > hash table. > > Fix this by initializing `shown` to NULL and moving the cleanup to the > `cleanup` label using kh_destroy_str(), which is safe to call on NULL. > This ensures the hash table is freed regardless of which code path is > taken.
Makes perfect sense, the changes to the code matches the description, and the difference between kh_release_* and kh_destroy_* in khash.h is exactly as described in the log message.
Perfect. I wish all the patches posted here are as easy to review as this one ;-)
Show 35 quoted lines
> Signed-off-by: Paul Tarjan <github@paulisageek.com>
> ---
> 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);