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

[Johannes Sixt <j.sixt@viscovery.net>]

Michal Rokos schrieb:
> 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.

> +# 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.

> +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.

> 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.)

> +   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