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

[Taylor Blau <me@ttaylorr.com>]

On Wed, Sep 15, 2021 at 02:01:39PM -0700, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.

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