Re: [PATCH] trace2: tolerate failed timestamp formatting

From: Derrick Stolee <stolee@gmail.com>
Date: Jul 18, 2026, 15:01 UTC
Message-ID: <c8d443a5-3cfb-4752-8716-cf0d8fadd9d3@gmail.com>
In-Reply-To: <alpXW5U6sndZtgqV@com-79390>

On 7/17/2026 12:24 PM, Taylor Blau wrote:

Show 17 quoted lines

> On Wed, Jul 15, 2026 at 04:12:11PM +0000, Derrick Stolee via GitGitGadget wrote:
>> This change removes all uses of xsnprintf() from the trace2/ directory.
>> There are two uses of xstrdup() that could be considered for removal,
>> but they only die() on out-of-memory errors instead of formatting
>> issues. I chose to leave those in place for now.
> 
> I may be missing some Git for Windows context, but I dug into this a
> little and I'm not sure 'gettimeofday()' is the culprit...
> 
> In my understanding Git for Windows's 'gettext.h' appears[1] to redirect
> the 'vsnprintf()' inside 'xsnprintf()' to 'libintl_vsnprintf()'. In this
> case, we have seven '%' placeholders. Gettext can store only six plus
> its end marker inline, so parsing the seventh causes an allocation
> before any timestamp values are read.
> 
> A failure there would produce the observed -1, after which 'xsnprintf()'
> dies and trace2 can recurse.

With this perspective, the issue is that gettext is doing dynamic allocation and getting a failure there, which explains the transient nature. This is an interesting idea, and a more likely "application side" error. I'm still curious why this is creeping up for the first time in this burst, since nothing has changed in the application, to my knowledge.

Show 9 quoted lines

> I think that also explains why calling 'snprintf()' directly helps.
> tr2_tbuf.c doesn't include gettext.h, so I think it bypasses libintl. If
> I'm reading compat/mingw.c correctly, 'gettimeofday()' fills tv and
> always returns zero [2], making the zero-initialization unrelated.
> 
> Would it make more sense to fix the xsnprintf()/libintl boundary and
> treat Trace2 reentrancy separately? I still can't explain why the
> allocation failed, so there may be another GfW-specific piece I’m
> missing.

I think that your suggested change has merits and should be pursued. I'll explore it a bit to confirm.

The other justification I'd like to make in my patch is that the xsnprintf() calls die() and the trace2 machinery should be die()-free whenever possible. Solving both possible causes is likely the right long-term approach.

Thanks, -Stolee