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

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

[Michal Rokos]

This PATCH is NOT intended to be merged (yet).

Some systems (namely HPUX) return -1 when maxsize in vsnprintf() is reached. 
So replace that broken vsnprintf() with our own that returns correct value 
upon overflow.

Could anybody give it some testing since I don't know how many broken systems 
are out there?

If anybody could think of some better define than BROKEN_VSNPRINTF, I'm all 
ears.

Linux is OK, HPUX is detected to be broken. On HPUX (11.23) test suite with 
defined BROKEN_VSNPRINTF fails in 8 *.sh testsuites, without it it fails in 
140 *.sh testsuites (out of 241).

Signed-off-by: Michal Rokos <michal.rokos@nextsoft.cz>

diff --git a/Makefile b/Makefile
index ca5aad9..a1dbf1d 100644
--- a/Makefile
+++ b/Makefile
@@ -3,6 +3,10 @@ all::
 
 # Define V=1 to have a more verbose compile.
 #
+# Define BROKEN_VSNPRINTF if your are on a system which vsnprintf() return
+# something else (typically -1) instead of number of characters which would
+# have been written to the final string if enough space had been available.
+#
 # Define FREAD_READS_DIRECTORIES if your are on a system which succeeds
 # when attempting to read from an fopen'ed directory.
 #
@@ -526,6 +530,7 @@ ifeq ($(uname_S),HP-UX)
 	NO_UNSETENV = YesPlease
 	NO_HSTRERROR = YesPlease
 	NO_SYS_SELECT_H = YesPlease
+	BROKEN_VSNPRINTF = UnfortunatelyYes
 endif
 ifneq (,$(findstring arm,$(uname_M)))
 	ARM_SHA1 = YesPlease
@@ -629,6 +634,10 @@ endif
 ifdef NO_C99_FORMAT
 	BASIC_CFLAGS += -DNO_C99_FORMAT
 endif
+ifdef BROKEN_VSNPRINTF
+	COMPAT_CFLAGS += -DBROKEN_VSNPRINTF
+	COMPAT_OBJS += compat/vsnprintf.o
+endif
 ifdef FREAD_READS_DIRECTORIES
 	COMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES
 	COMPAT_OBJS += compat/fopen.o
diff --git a/config.mak.in b/config.mak.in
index ee6c33d..aaa98c6 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -46,3 +46,4 @@ NO_MKDTEMP=@NO_MKDTEMP@
 NO_ICONV=@NO_ICONV@
 OLD_ICONV=@OLD_ICONV@
 NO_DEFLATE_BOUND=@NO_DEFLATE_BOUND@
+BROKEN_VSNPRINTF=@BROKEN_VSNPRINTF@
diff --git a/configure.ac b/configure.ac
index 85d7ef5..d4bb2b3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -326,6 +326,37 @@ else
 	NO_C99_FORMAT=
 fi
 AC_SUBST(NO_C99_FORMAT)
+#
+# Define BROKEN_VSNPRINTF if your are on a system which vsnprintf() return
+# something else (typically -1) instead of number of characters which would
+# have been written to the final string if enough space had been available.
+AC_CACHE_CHECK([whether vsnprintf() is broken],
+ [ac_cv_broken_vsnprintf],
+[
+AC_RUN_IFELSE(
+	[AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT
+		#include "stdarg.h"
+
+		int test_vsnprintf(char *str, size_t maxsize, const char *format, ...)
+		{
+		  int ret;
+		  va_list ap;
+		  va_start(ap, format);
+		  ret = vsnprintf(str, maxsize, format, ap);
+		  va_end(ap);
+                  return ret;
+		}],
+		[[char buf[1];
+		  if (test_vsnprintf(buf, 1, "%s", "12345") != 5) return 1]])],
+	[ac_cv_broken_vsnprintf=no],
+	[ac_cv_broken_vsnprintf=yes])
+])
+if test $ac_cv_broken_vsnprintf = yes; then
+	BROKEN_VSNPRINTF=UnfortunatelyYes
+else
+	BROKEN_VSNPRINTF=
+fi
+AC_SUBST(BROKEN_VSNPRINTF)
 
 
 ## Checks for library functions.
diff --git a/git-compat-util.h b/git-compat-util.h
index 2a40703..5c392f8 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -209,6 +209,12 @@ void *gitmemmem(const void *haystack, size_t haystacklen,
 extern FILE *git_fopen(const char*, const char*);
 #endif
 
+#ifdef BROKEN_VSNPRINTF
+#define vsnprintf git_vsnprintf
+extern int git_vsnprintf(char *str, size_t maxsize,
+                         const char *format, va_list ap);
+#endif
+
 #ifdef __GLIBC_PREREQ
 #if __GLIBC_PREREQ(2, 1)
 #define HAVE_STRCHRNUL
diff --git a/dev/null b/compat/vsnprintf.c
new file mode 100644
index 0000000..263e00e
--- /dev/null
+++ b/compat/vsnprintf.c
@@ -0,0 +1,20 @@
+#include "../git-compat-util.h"
+
+#undef vsnprintf
+int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap);
+{
+	int ret = vsnprintf(s, maxsize, format, ap);
+	if (ret != -1 ) return ret;
+
+	s = NULL;
+	while ( ret == -1 )
+	{
+		maxsize = (maxsize*3)/2;
+		s = realloc(s, maxsize);
+		if (! s) return -1;
+		ret = vsnprintf(s, maxsize, format, ap);
+	}
+	free(s);
+	return ret;
+}
+

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

[Johannes Schindelin]

Hi,

On Tue, 4 Mar 2008, Michal Rokos wrote:

> This PATCH is NOT intended to be merged (yet).

See also

http://article.gmane.org/gmane.comp.version-control.git/75280/match=vsnprintf

Ciao,
Dscho

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

[Finn Arne Gangstad]

On Tue, Mar 04, 2008 at 02:59:28PM +0100, Michal Rokos wrote:

> +	while ( ret == -1 )
> +	{
> +		maxsize = (maxsize*3)/2;
> +		s = realloc(s, maxsize);
> +		if (! s) return -1;
> +		ret = vsnprintf(s, maxsize, format, ap);  /* <--- UNSAFE! */
> +	}

This is not generally safe, you cannot call vsnprintf multiple times
with the same ap on all architectures. You need va_copy (or __va_copy,
or VA_COPY, differs a bit between different architectures, especially
one the ones with a broken vsnprintf I guess..)

- Finn Arne

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

[Morten Welinder]

That leaks in the out-of-memory case.  Not terribly important, but still.

Morten

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

[Johannes Sixt]

Finn Arne Gangstad schrieb:
> On Tue, Mar 04, 2008 at 02:59:28PM +0100, Michal Rokos wrote:
> 
>> +	while ( ret == -1 )
>> +	{
>> +		maxsize = (maxsize*3)/2;
>> +		s = realloc(s, maxsize);
>> +		if (! s) return -1;
>> +		ret = vsnprintf(s, maxsize, format, ap);  /* <--- UNSAFE! */
>> +	}
> 
> This is not generally safe, you cannot call vsnprintf multiple times
> with the same ap on all architectures. You need va_copy (or __va_copy,
> or VA_COPY, differs a bit between different architectures, especially
> one the ones with a broken vsnprintf I guess..)

True. But...

This replacement of vsnprintf will not be needed on all architectures, but
only on some. And on these we can test in advance whether we can get away
without va_copy (et.al.). A note next to the configuration setting about
this would be in order, I think.

Furthermore, on systems where vsnprintf is broken in this way, va_copy is
likely not available.

-- Hannes

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

[Johannes Sixt]

Michal Rokos schrieb:
> If anybody could think of some better define than BROKEN_VSNPRINTF, I'm all 
> ears.

CLUELESS_OVERFLOWN_VSNPRINTF?

Because we have the same issue on Windows where vsnprintf returns -1 on
overflow.

But there is also another complication: The size parameter of the system's
vsnprintf must not count the trailing NUL, i.e. the buffer must actually
have space for one extra byte, whereas the POSIX version must count NUL.
Can you check whether your vsnprintf has this flaw, too? If it doesn't, we
would need another configuration variable so that we can distinguish these
two kinds of brokenness.

-- Hannes

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

[Wayne Davison]

On Tue, Mar 04, 2008 at 05:28:57PM +0100, Johannes Sixt wrote:
> But there is also another complication: The size parameter of the system's
> vsnprintf must not count the trailing NUL, i.e. the buffer must actually
> have space for one extra byte, whereas the POSIX version must count NUL.

FYI, rsync's configure script has a check for both those problems.  It
sets HAVE_C99_VSNPRINTF if vsnprintf() works right.  If that fails,
rsync uses its own lib/snprintf.c implementation (that I believe comes
from Samba).  That file could be included in git, if desired.

..wayne..

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

[Michal Rokos]

Hello,

On Tuesday 04 March 2008 17:28:57 Johannes Sixt wrote:
> Michal Rokos schrieb:
> > If anybody could think of some better define than BROKEN_VSNPRINTF, I'm
> > all ears.
>
> CLUELESS_OVERFLOWN_VSNPRINTF?

Heh. My first one was VSNPRINTF_RETURNS_MINUS_1...

> Because we have the same issue on Windows where vsnprintf returns -1 on
> overflow.
>
> But there is also another complication: The size parameter of the system's
> vsnprintf must not count the trailing NUL, i.e. the buffer must actually
> have space for one extra byte, whereas the POSIX version must count NUL.
> Can you check whether your vsnprintf has this flaw, too? If it doesn't, we
> would need another configuration variable so that we can distinguish these
> two kinds of brokenness.

I did my homework and prepared this:

#include "stdio.h"
#include "stdarg.h"

int test_vsnprintf(char *str, size_t maxsize, const char *format, ...)
{
  int ret;
  va_list ap;

  va_start(ap, format);
  ret = vsnprintf(str, maxsize, format, ap);
  va_end(ap);

  return ret;
}

int main(void)
{
  char buf[10];
  int ret;

  ret = test_vsnprintf(buf, 1, "%s", "12345");
  printf("case1: %d\n", ret);

  ret = test_vsnprintf(buf, 5, "%s", "12345");
  printf("case2: %d\n", ret);

  ret = test_vsnprintf(buf, 6, "%s", "12345");
  printf("case3: %d\n", ret);

  ret = test_vsnprintf(buf, 10, "%s", "12345");
  printf("case4: %d\n", ret);

  return 0;
}

which returns:
Linux 2.6.25-rc3-mr i686
case1: 5
case2: 5
case3: 5
case4: 5

HP-UX B.11.11 9000/800
case1: -1
case2: -1
case3: 5
case4: 5

HP-UX B.11.23 ia64
case1: -1
case2: -1
case3: 5
case4: 5

So HPUX impl is the same as a Windows one. So we can share the replacement.
Please note that there's no va_copy() on HPUX.

I think that Tru64 has the same issue, but I have no HW to test it on now.

Could somebody else try to run testcase above on some other OSes?

Thanks

Michal

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

[Jeff King]

On Wed, Mar 05, 2008 at 09:37:39AM +0100, Michal Rokos wrote:

> Could somebody else try to run testcase above on some other OSes?

SunOS 5.8 correctly returns '5' in each case.

-Peff

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

[Johannes Sixt]

Michal Rokos schrieb:
> HP-UX B.11.11 9000/800
> case1: -1
> case2: -1
> case3: 5
> case4: 5
> 
> HP-UX B.11.23 ia64
> case1: -1
> case2: -1
> case3: 5
> case4: 5
> 
> So HPUX impl is the same as a Windows one. So we can share the replacement.
> Please note that there's no va_copy() on HPUX.

It's not the same on Windows, which returns:

case1: -1
case2: 5
case3: 5
case4: 5

IOW, HPUX et.al. take the *size* of the buffer, whereas Windows takes the
maximal number of characters to write excluding NUL.

Which means that we need another configuration variable on Windows:

  -DSNPRINTF_SIZE_CORR=1

and

#ifndef SNPRINTF_SIZE_CORR
#define SNPRINTF_SIZE_CORR 0
#endif

in the replacement implementation. But I can do this myself in the course
of the MinGW port.

BTW, this is not only an issue of vsnprintf, but also of snprintf!

-- Hannes

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

[Mike Ralphson]

On 05/03/2008, Michal Rokos <michal.rokos@nextsoft.cz> wrote:
>  Could somebody else try to run testcase above on some other OSes?

AIX isis 3 5 00C0FEDC4C00
case1: 5
case2: 5
case3: 5
case4: 5

<faints/>

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

[Robert Haines]

> Could somebody else try to run testcase above on some other OSes?


Mac OS X Darwin 8.11.1 i386 (Tiger)

case1: 5
case2: 5
case3: 5
case4: 5

Cheers,
Rob

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

[Michal Rokos]

Hello,

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.

Signed-off-by: Michal Rokos <michal.rokos@nextsoft.cz>

diff --git a/Makefile b/Makefile
index ca5aad9..49d5ab6 100644
--- a/Makefile
+++ b/Makefile
@@ -3,6 +3,14 @@ all::
 
 # Define V=1 to have a more verbose compile.
 #
+# 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.
+#
 # Define FREAD_READS_DIRECTORIES if your are on a system which succeeds
 # when attempting to read from an fopen'ed directory.
 #
@@ -629,6 +637,14 @@ endif
 ifdef NO_C99_FORMAT
 	BASIC_CFLAGS += -DNO_C99_FORMAT
 endif
+ifdef SNPRINTF_RETURNS_BOGUS
+	COMPAT_CFLAGS += -DSNPRINTF_RETURNS_BOGUS
+	COMPAT_OBJS += compat/snprintf.o
+endif
+ifdef VSNPRINTF_RETURNS_BOGUS
+	COMPAT_CFLAGS += -DVSNPRINTF_RETURNS_BOGUS
+	COMPAT_OBJS += compat/snprintf.o
+endif
 ifdef FREAD_READS_DIRECTORIES
 	COMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES
 	COMPAT_OBJS += compat/fopen.o
diff --git a/config.mak.in b/config.mak.in
index ee6c33d..a10a4af 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -46,3 +46,5 @@ NO_MKDTEMP=@NO_MKDTEMP@
 NO_ICONV=@NO_ICONV@
 OLD_ICONV=@OLD_ICONV@
 NO_DEFLATE_BOUND=@NO_DEFLATE_BOUND@
+SNPRINTF_RETURNS_BOGUS=@SNPRINTF_RETURNS_BOGUS@
+VSNPRINTF_RETURNS_BOGUS=@VSNPRINTF_RETURNS_BOGUS@
diff --git a/configure.ac b/configure.ac
index 85d7ef5..b902888 100644
--- a/configure.ac
+++ b/configure.ac
@@ -326,6 +326,57 @@ else
 	NO_C99_FORMAT=
 fi
 AC_SUBST(NO_C99_FORMAT)
+#
+# 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.
+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]])],
+	[ac_cv_snprintf_returns_bogus=no],
+	[ac_cv_snprintf_returns_bogus=yes])
+])
+if test $ac_cv_snprintf_returns_bogus = yes; then
+	SNPRINTF_RETURNS_BOGUS=UnfortunatelyYes
+else
+	SNPRINTF_RETURNS_BOGUS=
+fi
+AC_SUBST(SNPRINTF_RETURNS_BOGUS)
+#
+# 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.
+AC_CACHE_CHECK([whether vsnprintf() returns bogus],
+ [ac_cv_vsnprintf_returns_bogus],
+[
+AC_RUN_IFELSE(
+	[AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT
+		#include "stdarg.h"
+
+		int test_vsnprintf(char *str, size_t maxsize, const char *format, ...)
+		{
+		  int ret;
+		  va_list ap;
+		  va_start(ap, format);
+		  ret = vsnprintf(str, maxsize, format, ap);
+		  va_end(ap);
+		  return ret;
+		}],
+		[[char buf[1];
+		  if (test_vsnprintf(buf, 1, "%s", "12345") != 5) return 1]])],
+	[ac_cv_vsnprintf_returns_bogus=no],
+	[ac_cv_vsnprintf_returns_bogus=yes])
+])
+if test $ac_cv_vsnprintf_returns_bogus = yes; then
+	VSNPRINTF_RETURNS_BOGUS=UnfortunatelyYes
+else
+	VSNPRINTF_RETURNS_BOGUS=
+fi
+AC_SUBST(VSNPRINTF_RETURNS_BOGUS)
 
 
 ## Checks for library functions.
diff --git a/git-compat-util.h b/git-compat-util.h
index 2a40703..6618c08 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -209,6 +209,18 @@ void *gitmemmem(const void *haystack, size_t haystacklen,
 extern FILE *git_fopen(const char*, const char*);
 #endif
 
+#ifdef SNPRINTF_RETURNS_BOGUS
+#define snprintf git_snprintf
+extern int git_snprintf(char *str, size_t maxsize,
+                        const char *format, ...);
+#endif
+
+#ifdef VSNPRINTF_RETURNS_BOGUS
+#define vsnprintf git_vsnprintf
+extern int git_vsnprintf(char *str, size_t maxsize,
+                         const char *format, va_list ap);
+#endif
+
 #ifdef __GLIBC_PREREQ
 #if __GLIBC_PREREQ(2, 1)
 #define HAVE_STRCHRNUL
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;
+
+   s = NULL;
+   size = maxsize;
+   while ( ret == -1 )
+   {
+      size *= 4;
+      s = realloc(s, size);
+      if (! s) return -1;
+      ret = vsnprintf(s, size, format, ap);
+   }
+   if (str && maxsize > 0) memcpy(str, s, maxsize);
+   free(s);
+   return ret;
+}
+
+int git_snprintf(char *str, size_t maxsize, const char *format, ...)
+{
+   va_list ap;
+   int ret;
+
+   va_start(ap, format);
+   ret = git_vsnprintf(str, maxsize, format, ap);
+   va_end(ap);
+
+   return ret;
+}
+



-- 
Michal Rokos

NextSoft s.r.o.
Vyskočilova 1/1410
140 21 Praha 4
phone:  +420 267 224 311
fax:    +420 267 224 307
mobile: +420 736 646 591
e-mail: michal.rokos@nextsoft.cz

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

[Michal Rokos]

Hello,

On Wednesday 05 March 2008 11:35:25 Robert Haines wrote:
> > Could somebody else try to run testcase above on some other OSes?

thank you all for testing on SunOS, Windows, AIX, and MacOS!

Michal

-- 
Michal Rokos

NextSoft s.r.o.
Vyskočilova 1/1410
140 21 Praha 4
phone:  +420 267 224 311
fax:    +420 267 224 307
mobile: +420 736 646 591
e-mail: michal.rokos@nextsoft.cz

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

[Johannes Sixt]

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

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

[Michal Rokos]

Hello,

On Wednesday 05 March 2008 15:28:48 Johannes Sixt wrote:
> > 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.

Did some more tests. HPUX (v)snprintf() writes chars just well, but returns 
bogus. So no need to copy data explicitly here as well. I'm too quick 
on "Send" button and slow in testing; sorry.

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

I don't know. Right now I'm aware of 2 OSes that return bogus - HPUX and 
Windows. I the rest of bugus OSes is the same, 1 config is enough.

Thank you for your comments; is this better?

diff --git a/Makefile b/Makefile
index ca5aad9..6af7132 100644
--- a/Makefile
+++ b/Makefile
@@ -3,6 +3,10 @@ all::
 
 # Define V=1 to have a more verbose compile.
 #
+# Define SNPRINTF_RETURNS_BOGUS if your are on a system which snprintf()
+# or vsnprintf() returns -1 instead of number of characters which would
+# have been written to the final string if enough space had been available.
+#
 # Define FREAD_READS_DIRECTORIES if your are on a system which succeeds
 # when attempting to read from an fopen'ed directory.
 #
@@ -629,6 +633,10 @@ endif
 ifdef NO_C99_FORMAT
 	BASIC_CFLAGS += -DNO_C99_FORMAT
 endif
+ifdef SNPRINTF_RETURNS_BOGUS
+	COMPAT_CFLAGS += -DSNPRINTF_RETURNS_BOGUS
+	COMPAT_OBJS += compat/snprintf.o
+endif
 ifdef FREAD_READS_DIRECTORIES
 	COMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES
 	COMPAT_OBJS += compat/fopen.o
diff --git a/config.mak.in b/config.mak.in
index ee6c33d..8e1cd5f 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -46,3 +46,4 @@ NO_MKDTEMP=@NO_MKDTEMP@
 NO_ICONV=@NO_ICONV@
 OLD_ICONV=@OLD_ICONV@
 NO_DEFLATE_BOUND=@NO_DEFLATE_BOUND@
+SNPRINTF_RETURNS_BOGUS=@SNPRINTF_RETURNS_BOGUS@
diff --git a/configure.ac b/configure.ac
index 85d7ef5..a3bbfa6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -326,6 +326,38 @@ else
 	NO_C99_FORMAT=
 fi
 AC_SUBST(NO_C99_FORMAT)
+#
+# Define SNPRINTF_RETURNS_BOGUS if your are on a system which snprintf()
+# or vsnprintf() returns -1 instead of number of characters which would
+# have been written to the final string if enough space had been available.
+AC_CACHE_CHECK([whether snprintf() and/or vsnprintf() return bogus],
+ [ac_cv_snprintf_returns_bogus],
+[
+AC_RUN_IFELSE(
+	[AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT
+		#include "stdarg.h"
+
+		int test_vsnprintf(char *str, size_t maxsize, const char *format, ...)
+		{
+		  int ret;
+		  va_list ap;
+		  va_start(ap, format);
+		  ret = vsnprintf(str, maxsize, format, ap);
+		  va_end(ap);
+		  return ret;
+		}],
+		[[char buf[1];
+		  if (test_vsnprintf(buf, 1, "%s", "12345") != 5) return 1;
+		  if (snprintf(buf, 1, "%s", "12345") != 5) return 1]])],
+	[ac_cv_snprintf_returns_bogus=no],
+	[ac_cv_snprintf_returns_bogus=yes])
+])
+if test $ac_cv_snprintf_returns_bogus = yes; then
+	SNPRINTF_RETURNS_BOGUS=UnfortunatelyYes
+else
+	SNPRINTF_RETURNS_BOGUS=
+fi
+AC_SUBST(SNPRINTF_RETURNS_BOGUS)
 
 
 ## Checks for library functions.
diff --git a/git-compat-util.h b/git-compat-util.h
index 2a40703..0aa04eb 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -209,6 +209,15 @@ void *gitmemmem(const void *haystack, size_t haystacklen,
 extern FILE *git_fopen(const char*, const char*);
 #endif
 
+#ifdef SNPRINTF_RETURNS_BOGUS
+#define snprintf git_snprintf
+extern int git_snprintf(char *str, size_t maxsize,
+                        const char *format, ...);
+#define vsnprintf git_vsnprintf
+extern int git_vsnprintf(char *str, size_t maxsize,
+                         const char *format, va_list ap);
+#endif
+
 #ifdef __GLIBC_PREREQ
 #if __GLIBC_PREREQ(2, 1)
 #define HAVE_STRCHRNUL
diff --git a/dev/null b/compat/snprintf.c
new file mode 100644
index 0000000..9f30b22
--- /dev/null
+++ b/compat/snprintf.c
@@ -0,0 +1,39 @@
+#include "../git-compat-util.h"
+
+#undef vsnprintf
+int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
+{
+	char *s;
+
+	int ret = vsnprintf(str, maxsize, format, ap);
+	if (ret != -1 )
+		return ret;
+
+	s = NULL;
+
+	while (ret == -1) {
+		maxsize *= 4;
+		str = realloc(s, maxsize);
+		if (! str) {
+			free(s);
+			return -1;
+		}
+		s = str;
+		ret = vsnprintf(str, maxsize, format, ap);
+	}
+	free(s);
+	return ret;
+}
+
+int git_snprintf(char *str, size_t maxsize, const char *format, ...)
+{
+	va_list ap;
+	int ret;
+
+	va_start(ap, format);
+	ret = git_vsnprintf(str, maxsize, format, ap);
+	va_end(ap);
+
+	return ret;
+}
+

-- 
Michal Rokos

NextSoft s.r.o.
Vyskočilova 1/1410
140 21 Praha 4
phone:  +420 267 224 311
fax:    +420 267 224 307
mobile: +420 736 646 591
e-mail: michal.rokos@nextsoft.cz

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

[Johannes Sixt]

Michal Rokos schrieb:
> Thank you for your comments; is this better?

Better, but still not there. See below. The configure test looks fine, but
I can't test it.

Finally, please make this a proper patch with Signed-off-by for Junio to
pick up.

> +#undef vsnprintf
> +int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
> +{
> +	char *s;
> +
> +	int ret = vsnprintf(str, maxsize, format, ap);
> +	if (ret != -1 )
> +		return ret;
> +
> +	s = NULL;
> +

What if maxsize == 0? Insert here:

	if (maxsize < 250)
		maxsize = 250;

> +	while (ret == -1) {
> +		maxsize *= 4;
> +		str = realloc(s, maxsize);
> +		if (! str) {
> +			free(s);
> +			return -1;
> +		}

ret == -1 at this time, so:

		if (!str)
			break;

Hm?

> +		s = str;
> +		ret = vsnprintf(str, maxsize, format, ap);
> +	}
> +	free(s);
> +	return ret;
> +}

-- Hannes

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

[Michal Rokos]

Hi,

On Wednesday 05 March 2008 16:22:11 Johannes Sixt wrote:
> Michal Rokos schrieb:
> > Thank you for your comments; is this better?
>
> Better, but still not there. See below. The configure test looks fine, but
> I can't test it.
>
> Finally, please make this a proper patch with Signed-off-by for Junio to
> pick up.

Did that. Thank you for your review.

> What if maxsize == 0? Insert here:
>
> 	if (maxsize < 250)
> 		maxsize = 250;

I've just used 128 which I hope is OK - it's multiplied 4 * anyway.

Michal

-- 
Michal Rokos

NextSoft s.r.o.
Vyskočilova 1/1410
140 21 Praha 4
phone:  +420 267 224 311
fax:    +420 267 224 307
mobile: +420 736 646 591
e-mail: michal.rokos@nextsoft.cz

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

[Wayne Davison]

On Wed, Mar 05, 2008 at 04:00:06PM +0100, Michal Rokos wrote:
> +		[[char buf[1];
> +		  if (test_vsnprintf(buf, 1, "%s", "12345") != 5) return 1;
> +		  if (snprintf(buf, 1, "%s", "12345") != 5) return 1]])],

I'd suggest using a longer buf, requesting a longer length (e.g. 3
instead of 1), and then making sure that the resulting buf is right
(e.g. "12" instead of "123").

		[[char buf[6];
		  if (test_vsnprintf(buf, 3, "%s", "12345") != 5
		   || strcmp(buf, "12") != 0) return 1;
		  if (snprintf(buf, 3, "%s", "12345") != 5
		   || strcmp(buf, "12") != 0) return 1]])],

Then, set a define that snprintf is bogus and use a version of
snprintf() based on this instead:

http://rsync.samba.org/ftp/unpacked/rsync/lib/snprintf.c

That defines rsync_snprintf() and rsync_vsnprintf() functions (which
could be renamed for git).  Then, in a global .h file, add something
like this:

#if !defined HAVE_VSNPRINTF || !defined HAVE_C99_VSNPRINTF
#define vsnprintf rsync_vsnprintf
int vsnprintf(char *str, size_t count, const char *fmt, va_list args);
#endif

#if !defined HAVE_SNPRINTF || !defined HAVE_C99_VSNPRINTF
#define snprintf rsync_snprintf
int snprintf(char *str, size_t count, const char *fmt,...);
#endif

Just be sure to put those that after the various system includes so that
they are not adversely affected.

..wayne..

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

[Johannes Sixt]

Wayne Davison schrieb:
> Then, set a define that snprintf is bogus and use a version of
> snprintf() based on this instead:
> 
> http://rsync.samba.org/ftp/unpacked/rsync/lib/snprintf.c
> 
> That defines rsync_snprintf() and rsync_vsnprintf() functions (which
> could be renamed for git). 

No.

If one *printf function is replaced, all of them must be replaced. The
reason is that, eg. on Windows we don't have %llu, but we have %I64u. So
depending on which flavor of *printf is called, we would have to supply
the one format (rsync_printf: %llu) or the other format (fprintf et al:
%I64u).

-- Hannes

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

[Junio C Hamano]

Wayne Davison <wayne@opencoder.net> writes:

> I'd suggest using a longer buf, requesting a longer length (e.g. 3
> instead of 1), and then making sure that the resulting buf is right
> (e.g. "12" instead of "123").
>
> 		[[char buf[6];
> 		  if (test_vsnprintf(buf, 3, "%s", "12345") != 5
> 		   || strcmp(buf, "12") != 0) return 1;
> 		  if (snprintf(buf, 3, "%s", "12345") != 5
> 		   || strcmp(buf, "12") != 0) return 1]])],

That sounds like a sensible thing to do.

Other than that, the patch in

    Message-Id: <200803051646.13343.michal.rokos@nextsoft.cz>

looked Ok to me.

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

[Wayne Davison]

On Wed, Mar 05, 2008 at 05:04:12PM +0100, Johannes Sixt wrote:
> If one *printf function is replaced, all of them must be replaced. The
> reason is that, eg. on Windows we don't have %llu, but we have %I64u.

Or the compatibility function would need to know which escapes to
emulate (or even emulate a super-set of escapes, as long as they don't
conflict).  If the calling code already has a way to know which escapes
to use, then the compatibility code could easily be customized to handle
the right ones in the same way.

If the decision is made to not use a compatibility snprintf library,
then configure will need a separate test for the limit being off by one,
and then a fixing function would need to pass through limit-1.

..wayne..

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

[Michal Rokos]

Hello,

I've managed to run testcase on
Tru64 OSF1 <hostname> V5.1 2650 alpha
and it returns
case1: 0
case2: 4
case3: 5
case4: 5
which is sad since our vsnprintf() cannot detect this ill behaviour.

Michal

-- 
Michal Rokos

NextSoft s.r.o.
Vyskočilova 1/1410
140 21 Praha 4
phone:  +420 267 224 311
fax:    +420 267 224 307
mobile: +420 736 646 591
e-mail: michal.rokos@nextsoft.cz

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

[Johannes Sixt]

Michal Rokos schrieb:
> Hello,
> 
> I've managed to run testcase on
> Tru64 OSF1 <hostname> V5.1 2650 alpha
> and it returns
> case1: 0
> case2: 4
> case3: 5
> case4: 5
> which is sad since our vsnprintf() cannot detect this ill behaviour.

Ugh! We'd need to run the loop until we get a return value that is not
negative and less than the size specified...

-- Hannes

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

[Michal Rokos]

On Monday 10 March 2008 10:28:53 Johannes Sixt wrote:
> Michal Rokos schrieb:
> > Hello,
> >
> > I've managed to run testcase on
> > Tru64 OSF1 <hostname> V5.1 2650 alpha
> > and it returns
> > case1: 0
> > case2: 4
> > case3: 5
> > case4: 5
> > which is sad since our vsnprintf() cannot detect this ill behaviour.
>
> Ugh! We'd need to run the loop until we get a return value that is not
> negative and less than the size specified...

Hmm, I'm afraid we have to grow our buffer size until 2 successive runs return 
the same value and that value is not -1.

But - do we want to fix it now, when we don't even know that else would be 
needed for Tru64 port? There's even no Tru64 paragraph in Makefile yet.

Michal

PS: I have no ambition to port git to Tru64 since that test machine is 
accessible via friend of my friend - so I have to IM him the code and he 
returns value :)

-- 
Michal Rokos

NextSoft s.r.o.
Vyskočilova 1/1410
140 21 Praha 4
phone:  +420 267 224 311
fax:    +420 267 224 307
mobile: +420 736 646 591
e-mail: michal.rokos@nextsoft.cz

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

[Johannes Sixt]

Michal Rokos schrieb:
> But - do we want to fix it now, when we don't even know that else would be 
> needed for Tru64 port? There's even no Tru64 paragraph in Makefile yet.
> 
> Michal
> 
> PS: I have no ambition to port git to Tru64 since that test machine is 
> accessible via friend of my friend - so I have to IM him the code and he 
> returns value :)

It's not your itch nor is it not mine. Someone else has to come along...

-- Hannes