From: Derrick Stolee <stolee@gmail.com>
Date: Sat, 18 Jul 2026 15:01:02 GMT
Subject: Re: [PATCH] trace2: tolerate failed timestamp formatting
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:
> 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. 
> 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