From: René Scharfe <l.s.r@web.de>
Date: Wed, 16 Sep 2026 06:33:19 GMT
Subject: Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting
Message-ID: <bf0351d8-05fe-4f77-958a-2ac59495029c@web.de>
In-Reply-To: <20260916042312.35891-1-yqtian668@gmail.com>

On 9/16/26 6:23 AM, Yongqiang Tian wrote:
> During WinANSI initialization, duplicate_handle() reports the handle
> when DuplicateHandle() fails:
> 
>     die_lasterr("DuplicateHandle(%li) failed", ...);
> 
> die_lasterr() collects the formatting arguments in a va_list, but
> passes that va_list to die_errno() as an ordinary variadic argument.
> die_errno() consequently formats the representation of the va_list
> instead of the supplied handle, producing an incorrect fatal message.

Good find!

> The other current callers pass fixed strings and are unaffected.
> 
> Git does not provide a va_list-taking variant of die_errno(), so format
> the caller's arguments separately with strbuf_vaddf(). This consumes the
> original va_list correctly and produces the complete diagnostic prefix,
> including the handle supplied by duplicate_handle().
> 
> Save GetLastError() before formatting because calls made while growing
> the strbuf may change the thread's Windows error value. Convert the
> saved value to errno only after formatting, then pass the completed
> message to die_errno() through a literal "%s". This prevents any percent
> characters in the formatted message from being interpreted a second
> time, while allowing die_errno() to append the corresponding system
> error and terminate as before.

That all makes sense, but is quite complicated.  die_errno() itself uses
a fixed-size buffer to avoid heap allocation, for robustness and to
avoid changing errno.  How about turning die_lasterr() into a macro for
the same reasons?

	#define die_lasterr(...) do { \
		errno = err_win_to_posix(GetLastError()); \
		die_errno(__VA_ARGS__); \
	} while (0)

> The updated compat/winansi.c compiles with MinGW GCC 13. A Win64 probe
> under Wine prints a value derived from the va_list before this change
> and the supplied integer afterward.
> 
> Signed-off-by: Yongqiang Tian <yqtian668@gmail.com>
> ---
>  compat/winansi.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/compat/winansi.c b/compat/winansi.c
> index 3ce190093..5547192a2 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -7,6 +7,7 @@
>  #define DISABLE_SIGN_COMPARE_WARNINGS
>  
>  #include "../git-compat-util.h"
> +#include "../strbuf.h"
>  #include <wingdi.h>
>  #include <winreg.h>
>  #include "win32.h"
> @@ -438,11 +439,15 @@ static void winansi_exit(void)
>  
>  static void die_lasterr(const char *fmt, ...)
>  {
> +	DWORD err = GetLastError();
> +	struct strbuf message = STRBUF_INIT;
>  	va_list params;
> +
>  	va_start(params, fmt);
> -	errno = err_win_to_posix(GetLastError());
> -	die_errno(fmt, params);
> +	strbuf_vaddf(&message, fmt, params);
>  	va_end(params);
> +	errno = err_win_to_posix(err);
> +	die_errno("%s", message.buf);
>  }
>  
>  #undef dup2