git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: Breakage (?) in configure and git_vsnprintf()

From
Brandon Casey <casey@nrlssc.navy.mil>
Date
Dec 12, 2011, 23:25 UTC
Message-ID
<KDO2EaM6TnHQ6KGYqmkErS6Nq_qZc7b4J1aClSFkQwb60rk3bMZqU3iiY0O2LRaJukqzzBU6djUrNHGC505_oYXFc1BUezUo8EtdKcjEOSE@cipher.nrlssc.navy.mil>
In-Reply-To
<20111212142550.GA23875@sigill.intra.peff.net>
FYI: all tests passed on IRIX (not that I suspected otherwise).
-Brandon
On 12/12/2011 08:25 AM, Jeff King wrote:
Show 69 quoted lines
> On Mon, Dec 12, 2011 at 10:30:27AM +0100, Andreas Schwab wrote:
> 
>> Jeff King <peff@peff.net> writes:
>>
>>>  	if (maxsize > 0) {
>>> -		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
>>> +		va_copy(cp, ap);
>>> +		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
>>
>> You also need to call va_end on the copy.
> 
> Silly me. Thanks for catching.
> 
> Junio, here's a corrected version.
> 
> -- >8 --
> Subject: [PATCH] compat/snprintf: don't look at va_list twice
> 
> If you define SNPRINTF_RETURNS_BOGUS, we use a special
> git_vsnprintf wrapper assumes that vsnprintf returns "-1"
> instead of the number of characters that you would need to
> store the result.
> 
> To do this, it invokes vsnprintf multiple times, growing a
> heap buffer until we have enough space to hold the result.
> However, this means we evaluate the va_list parameter
> multiple times, which is generally a bad thing (it may be
> modified by calls to vsnprintf, yielding undefined
> behavior).
> 
> Instead, we must va_copy it and hand the copy to vsnprintf,
> so we always have a pristine va_list.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  compat/snprintf.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/compat/snprintf.c b/compat/snprintf.c
> index e1e0e75..42ea1ac 100644
> --- a/compat/snprintf.c
> +++ b/compat/snprintf.c
> @@ -19,11 +19,14 @@
>  #undef vsnprintf
>  int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
>  {
> +	va_list cp;
>  	char *s;
>  	int ret = -1;
>  
>  	if (maxsize > 0) {
> -		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
> +		va_copy(cp, ap);
> +		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
> +		va_end(cp);
>  		if (ret == maxsize-1)
>  			ret = -1;
>  		/* Windows does not NUL-terminate if result fills buffer */
> @@ -42,7 +45,9 @@ int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
>  		if (! str)
>  			break;
>  		s = str;
> -		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
> +		va_copy(cp, ap);
> +		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
> +		va_end(cp);
>  		if (ret == maxsize-1)
>  			ret = -1;
>  	}
Previous: Jeff KingNext: Michael Haggerty
Message 8 of 10 in “Breakage (?) in configure and git_vsnprintf()”
  1. Michael HaggertyDec 11, 2011
  2. Jeff KingDec 12, 2011
  3. Johannes SixtDec 12, 2011
  4. Jeff KingDec 12, 2011
  5. Junio C HamanoDec 12, 2011
  6. Andreas SchwabDec 12, 2011
  7. Jeff KingDec 12, 2011
  8. Brandon CaseyDec 12, 2011
  9. Michael HaggertyDec 12, 2011
  10. Jonathan NiederDec 12, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.