Re: [PATCH] Add compat/vsnprintf.c for systems that returns -1 on maxsize reached

From: Johannes Sixt <j.sixt@viscovery.net>
Date: Mar 5, 2008, 14:28 UTC
Message-ID: <47CEAE20.1030707@viscovery.net>
In-Reply-To: <200803051455.57148.michal.rokos@nextsoft.cz>

Michal Rokos schrieb:

Show 13 quoted lines

> On Wednesday 05 March 2008 10:18:10 Johannes Sixt wrote:
>> It's not the same on Windows, which returns:
>> case1: -1
>> case2: 5
>> case3: 5
>> case4: 5
>>
>> BTW, this is not only an issue of vsnprintf, but also of snprintf!
> 
> Hmm, HPUX has the same issue for snprint() as is for vsnprintf().
> 
> Do you think that following patch suffices your needs. Please note that it 
> actually copies data to str.

... in the case where the buffer is too small? This won't be a problem for our users.

Show 7 quoted lines

> +# Define SNPRINTF_RETURNS_BOGUS if your are on a system which snprintf()
> +# returns -1 instead of number of characters which would have been written
> +# to the final string if enough space had been available.
> +#
> +# Define VSNPRINTF_RETURNS_BOGUS if your are on a system which vsnprintf()
> +# returns -1 instead of number of characters which would have been written
> +# to the final string if enough space had been available.

We don't need two configuration variables. I think we can assume that if vsnprintf is broken, then snprintf will be broken, too:

# Define SNPRINTF_RETURNS_BOGUS if your are on a system which snprintf() # and vsnprintf() return -1 instead of number of characters that would # have been written to the final string if enough space had been # available.

Show 7 quoted lines

> +AC_CACHE_CHECK([whether snprintf() returns bogus],
> + [ac_cv_snprintf_returns_bogus],
> +[
> +AC_RUN_IFELSE(
> +	[AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT],
> +		[[char buf[1];
> +		  if (snprintf(bug, 1, "%s", "12345") != 5) return 1]])],

                               ^^^
buf?

Are you trying to test for the second bogus behavior on Windows? Don't do it! I've thought about it for 5 minutes, but I can't come up with a simple test that would detect its odd behavior.

Show 16 quoted lines

> diff --git a/dev/null b/compat/snprintf.c
> new file mode 100644
> index 0000000..bc0d37c
> --- /dev/null
> +++ b/compat/snprintf.c
> @@ -0,0 +1,37 @@
> +#include "../git-compat-util.h"
> +
> +#undef vsnprintf
> +int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
> +{
> +   char *s;
> +   int size;
> +
> +   int ret = vsnprintf(str, maxsize, format, ap);
> +   if (ret != -1 ) return ret;

'return' goes on its own line. Indentation is one tabstop, not two spaces. Thank you.

> +
> +   s = NULL;

You could reuse str here.

> +   size = maxsize;

We are trying to find a suitably long buffer in a loop. We should spend as few cycles as possible. Therefore, my implementation used a minimum of 250*4 for the first try just in case the caller had a long string to construct. (And it protects against maxsize == 0.)

Show 5 quoted lines

> +   while ( ret == -1 )
> +   {
> +      size *= 4;
> +      s = realloc(s, size);
> +      if (! s) return -1;

Could you avoid the memory leak on this error path?

> +      ret = vsnprintf(s, size, format, ap);
> +   }
> +   if (str && maxsize > 0) memcpy(str, s, maxsize);

Why this?

> +   free(s);
> +   return ret;
> +}

-- Hannes