Re: [PATCH] trace2: intercept all common signals

From: Jeff King <peff@peff.net>
Date: May 23, 2024, 09:36 UTC
Message-ID: <20240523093643.GG1306938@coredump.intra.peff.net>
In-Reply-To: <xmqqwmntra3f.fsf@gitster.g>

On Thu, May 16, 2024 at 09:32:36AM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> >   - the opposite approach might be: stop using any allocating functions
> >     in the trace2 code. There's a certain simplicity there, even for
> >     non-signal functions, that we know we're just touching a few
> >     fixed-size buffers, and you can never create a weird DoS by tweaking
> >     the tracing code. But it would mean rewriting a lot of it (including
> >     json formatting stuff) without many of our usual strbuf niceties.
> >
> >     This is more or less the approach we take with error(), die(), etc,
> >     which are built on vreportf() and its fixed buffer.
> 
> Would another approach be to add various trace2 functions that use
> strbuf() allocation a way to tell if they are called from a signal
> handing codepath, and punt (by doing nothing if needed, but
> hopefully we have enough slop in the buffer to say "hey we got
> interrupted so no more detailed report for you, sorry") if that is
> the case?

We do use that "in_signal" flag in other handlers. E.g., when run-command avoids calling free() in a signal, and the tempfile code avoids using stdio. But in the case of these trace functions, I think they'd all need to be rewritten to avoid strbufs. That message formatting is the whole point, and there is no way to have a strbuf which truncates rather than growing (though it is something we've discussed).

So I think we either need to rip strbufs out of most of trace2, or let these signal paths re-implement the formatting in a super-simple way.

-Peff