threads / patch / 12493

patchAdd compat/vsnprintf.c for systems that returns -1 on maxsize reached

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

## tl;dr

26 messages between Mar 4, 2008 and Mar 10, 2008. Diffs are folded; open one to read it.

replies: 25people: 10as markdown or json

Michal Rokos· Mar 4, 2008, 13:59 UTC · lore
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>
Show changes to 6 files +67 −0

Makefile, config.mak.in, configure.ac, git-compat-util.h, dev/null, compat/vsnprintf.c

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;
+}
+
Johannes Schindelin· Mar 4, 2008, 14:09 UTC · re: Michal Rokos · lore

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

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

Finn Arne Gangstad· Mar 4, 2008, 14:09 UTC · re: Michal Rokos · lore

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

On Tue, Mar 04, 2008 at 02:59:28PM +0100, Michal Rokos wrote:
Show 7 quoted lines
> +	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
Johannes Sixt· Mar 4, 2008, 16:19 UTC · re: Finn Arne Gangstad · lore

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

Finn Arne Gangstad schrieb:
Show 14 quoted lines
> 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
Johannes Sixt· Mar 4, 2008, 16:28 UTC · re: Michal Rokos · lore

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

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
Wayne Davison· Mar 4, 2008, 23:51 UTC · re: Johannes Sixt · lore

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

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..
Michal Rokos· Mar 5, 2008, 08:37 UTC · re: Johannes Sixt · lore

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

Hello,
On Tuesday 04 March 2008 17:28:57 Johannes Sixt wrote:
Show 5 quoted lines
> 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...
Show 9 quoted lines
> 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
Jeff King· Mar 5, 2008, 08:44 UTC · re: Michal Rokos · lore

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

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
Johannes Sixt· Mar 5, 2008, 09:18 UTC · re: Michal Rokos · lore

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

Michal Rokos schrieb:
Show 14 quoted lines
> 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
Michal Rokos· Mar 5, 2008, 13:55 UTC · re: Johannes Sixt · lore

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

Hello,
On Wednesday 05 March 2008 10:18:10 Johannes Sixt wrote:
Show 7 quoted lines
> 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>
Show changes to 6 files +118 −0

Makefile, config.mak.in, configure.ac, git-compat-util.h, dev/null, compat/snprintf.c

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
Johannes Sixt· Mar 5, 2008, 14:28 UTC · re: Michal Rokos · lore

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

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
Michal Rokos· Mar 5, 2008, 15:00 UTC · re: Johannes Sixt · lore

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

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.

Show 11 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:

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?
Show changes to 6 files +89 −0

Makefile, config.mak.in, configure.ac, git-compat-util.h, dev/null, compat/snprintf.c

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
Johannes Sixt· Mar 5, 2008, 15:22 UTC · re: Michal Rokos · lore

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

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.

Show 11 quoted lines
> +#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;
Show 7 quoted lines
> +	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?
Show 6 quoted lines
> +		s = str;
> +		ret = vsnprintf(str, maxsize, format, ap);
> +	}
> +	free(s);
> +	return ret;
> +}
-- Hannes
Michal Rokos· Mar 5, 2008, 15:48 UTC · re: Johannes Sixt · lore

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

Hi,
On Wednesday 05 March 2008 16:22:11 Johannes Sixt wrote:
Show 8 quoted lines
> 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
Wayne Davison· Mar 5, 2008, 15:54 UTC · re: Michal Rokos · lore

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

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..
Johannes Sixt· Mar 5, 2008, 16:04 UTC · re: Wayne Davison · lore

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

Wayne Davison schrieb:
Show 7 quoted lines
> 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
Wayne Davison· Mar 5, 2008, 22:33 UTC · re: Johannes Sixt · lore

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

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..
Junio C Hamano· Mar 5, 2008, 21:05 UTC · re: Wayne Davison · lore

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

Wayne Davison <wayne@opencoder.net> writes:
Show 9 quoted lines
> 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.
Mike Ralphson· Mar 5, 2008, 09:22 UTC · re: Michal Rokos · lore

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

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/>
Robert Haines· Mar 5, 2008, 10:35 UTC · re: Michal Rokos · lore

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

> 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

Michal Rokos· Mar 5, 2008, 13:58 UTC · re: Robert Haines · lore

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

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
Michal Rokos· Mar 10, 2008, 08:59 UTC · re: Johannes Sixt · lore

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

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
Johannes Sixt· Mar 10, 2008, 09:28 UTC · re: Michal Rokos · lore

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

Michal Rokos schrieb:
Show 10 quoted lines
> 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
Michal Rokos· Mar 10, 2008, 09:47 UTC · re: Johannes Sixt · lore

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

On Monday 10 March 2008 10:28:53 Johannes Sixt wrote:
Show 14 quoted lines
> 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
Johannes Sixt· Mar 10, 2008, 10:05 UTC · re: Michal Rokos · lore

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

Michal Rokos schrieb:
Show 8 quoted lines
> 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

← back to recent threads