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

"Jeff Hostetler via GitGitGadget" [off-list ref] writes:

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.

On Wed, Sep 15, 2021 at 02:01:39PM -0700, Junio C Hamano wrote:

"Jeff Hostetler via GitGitGadget" [off-list ref] writes:

quoted

From: Jeff Hostetler <redacted>

Do not leak the thread name (contained within the thread context) when
a thread terminates.

Signed-off-by: Jeff Hostetler <redacted>
---
 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.

It looks like we do not change the contents of the thread_name buffer
anywhere. I assume that we store the strbuf in struct tr2tls_thread_ctx
because we do some string formatting in tr2tls_create_self().

But there we could easily say ctx->thread_name = strbuf_release(&buf).

More concerning to me is that we use signed integers to keep track of nr
and alloc of array_us_start. But both of these are separate issues and
don't need to be dealt with here.

Thanks,
Taylor