Re: [PATCH 1/7] trace2: fix memory leak of thread name

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 15, 2021, 21:01 UTC
Message-ID: <xmqqa6kdwo24.fsf@gitster.g>
In-Reply-To: <5f557caee004f22cee33e8753063f0315459d7e1.1631738177.git.gitgitgadget@gmail.com>

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 22 quoted lines

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Do not leak the thread name (contained within the thread context) when
> a thread terminates.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  trace2/tr2_tls.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
> index 067c23755fb..7da94aba522 100644
> --- a/trace2/tr2_tls.c
> +++ b/trace2/tr2_tls.c
> @@ -95,6 +95,7 @@ void tr2tls_unset_self(void)
>  
>  	pthread_setspecific(tr2tls_key, NULL);
>  
> +	strbuf_release(&ctx->thread_name);
>  	free(ctx->array_us_start);
>  	free(ctx);
>  }

So the idea is create allocates a new instance, and unset is to release the resource held by it?

This is not a problem in _this_ patch but more about the base code that is being fixed here, but using strbuf as thread_name member sends a strong signal that it is designed to be inexpensive to change thread_name after a context is created by create_self. If it is not the case, the member should be "const char *", which may be computed using a temporary strbuf in create_self() and taken from the strbuf with strbuf_detach(), I would think.

Thanks.