On Wed, Sep 15, 2021 at 02:01:39PM -0700, Junio C Hamano wrote: > "Jeff Hostetler via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > > > From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx> > > > > Do not leak the thread name (contained within the thread context) when > > a thread terminates. > > > > Signed-off-by: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx> > > --- > > 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
