Fix a bug that caused "make check" to fail: MissingCompiler_Case

was reporting an unexpected exit status: 139 instead of 110.
This was caused by a seg fault in distccd, deep in the bowels of
vsnprintf(), which appears to be due to calling vsnprintf() twice
on the same va_list.  The fix is to use va_copy() in src/trace.c.

Of course it's never quite as easy as that.  va_copy() exists
only in C99, not in C89.  Some implementations have __va_copy()
but not va_copy().  So we need to autoconf it.  There was
already an autoconf test for this, but only for __va_copy.
I've moved the code which defined VA_COPY from snprintf.c to
a new header file va_copy.h, and added a VA_COPY_END macro.

Also, fix another bug that I noticed at the same time:
snprintf.c was using va_copy(), but was not matching each
call to va_copy with a corresponding call to va_end(),
as required by the C99 standard.



git-svn-id: http://distcc.googlecode.com/svn/trunk@559 01de4be4-8c4a-0410-9132-4925637da917

4 files changed, 66 insertions, 17 deletions

diff --git a/configure.ac b/configure.ac
index 4c433ae..0bdd4e9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -383,12 +383,19 @@ AC_TRY_COMPILE([#define func(a, b...) do { } while (0)],
  AC_DEFINE(HAVE_VARARG_MACROS, , [Define if your cpp has vararg macros])],
 [AC_MSG_RESULT(no)])
 
-AC_CACHE_CHECK([for __va_copy],samba_cv_HAVE_VA_COPY,[
+AC_CACHE_CHECK([for va_copy],dcc_cv_HAVE_VA_COPY,[
+AC_TRY_LINK([#include <stdarg.h>
+va_list ap1,ap2;], [va_copy(ap1,ap2);],
+dcc_cv_HAVE_VA_COPY=yes,dcc_cv_HAVE_VA_COPY=no)])
+if test x"$dcc_cv_HAVE_VA_COPY" = x"yes"; then
+    AC_DEFINE(HAVE_VA_COPY,1,[Whether va_copy() is available])
+fi
+AC_CACHE_CHECK([for __va_copy],dcc_cv_HAVE_UNDERSCORE_UNDERSCORE_VA_COPY,[
 AC_TRY_LINK([#include <stdarg.h>
 va_list ap1,ap2;], [__va_copy(ap1,ap2);],
-samba_cv_HAVE_VA_COPY=yes,samba_cv_HAVE_VA_COPY=no)])
-if test x"$samba_cv_HAVE_VA_COPY" = x"yes"; then
-    AC_DEFINE(HAVE_VA_COPY,1,[Whether __va_copy() is available])
+dcc_cv_HAVE_UNDERSCORE_UNDERSCORE_VA_COPY=yes,dcc_cv_HAVE_UNDERSCORE_UNDERSCORE_VA_COPY=no)])
+if test x"$dcc_cv_HAVE_UNDERSCORE_UNDERSCORE_VA_COPY" = x"yes"; then
+    AC_DEFINE(HAVE_UNDERSCORE_UNDERSCORE_VA_COPY,1,[Whether __va_copy() is available])
 fi
 
 AC_CACHE_CHECK([for C99 vsnprintf],rsync_cv_HAVE_C99_VSNPRINTF,[
diff --git a/src/snprintf.c b/src/snprintf.c
index 541e939..9402422 100644
--- a/src/snprintf.c
+++ b/src/snprintf.c
@@ -134,6 +134,7 @@
 #else
 
 #include "snprintf.h"
+#include "va_copy.h"
 
 #ifdef HAVE_LONG_DOUBLE
 #define LDOUBLE long double
@@ -147,14 +148,6 @@
 #define LLONG long
 #endif
 
-#ifndef VA_COPY
-#ifdef HAVE_VA_COPY
-#define VA_COPY(dest, src) __va_copy(dest, src)
-#else
-#define VA_COPY(dest, src) (dest) = (src)
-#endif /* ! HAVE_VA_COPY */
-#endif /* ndef VA_COPY */
-
 /* yes this really must be a ||. Don't muck with this (tridge) */
 #if !defined(HAVE_VSNPRINTF) || !defined(HAVE_C99_VSNPRINTF)
 
@@ -464,6 +457,8 @@ static size_t dopr(char *buffer, size_t maxlen, const char *format, va_list args
             buffer[maxlen - 1] = '\0';
     }
 
+    VA_COPY_END(args);
+
     return currlen;
 }
 
@@ -854,16 +849,16 @@ static void dopr_outch(char *buffer, size_t *currlen, size_t maxlen, char c)
     va_list ap2;
 
     VA_COPY(ap2, ap);
-
     ret = vsnprintf(NULL, 0, format, ap2);
+    VA_COPY_END(ap2);
     if (ret <= 0) return ret;
 
     (*ptr) = (char *)malloc(ret+1);
     if (!*ptr) return -1;
 
     VA_COPY(ap2, ap);
-
     ret = vsnprintf(*ptr, ret+1, format, ap2);
+    VA_COPY_END(ap2);
 
     return ret;
 }
diff --git a/src/trace.c b/src/trace.c
index 9614175..f23f94f 100644
--- a/src/trace.c
+++ b/src/trace.c
@@ -41,6 +41,7 @@
 
 #include "trace.h"
 #include "snprintf.h"
+#include "va_copy.h"
 
 struct rs_logger_list {
     rs_logger_fn               *fn;
@@ -201,7 +202,6 @@ static void rs_lazy_default(void)
         rs_add_logger(rs_logger_file, RS_LOG_WARNING, NULL, STDERR_FILENO);
 }
 
-
 /* Heart of the matter */
 static void
 rs_log_va(int flags, char const *caller_fn_name, char const *fmt, va_list va)
@@ -215,9 +215,16 @@ rs_log_va(int flags, char const *caller_fn_name, char const *fmt, va_list va)
 
     if (level <= rs_trace_level)
       for (l = logger_list; l; l = l->next)
-          if (level <= l->max_level)
+          if (level <= l->max_level) {
+              /* We need to use va_copy() here, because functions like vsprintf
+               * may destructively modify their va_list argument, but we need
+               * to ensure that it's still valid next time around the loop. */
+              va_list copied_va;
+              VA_COPY(copied_va, va);
               l->fn(flags, caller_fn_name,
-                    fmt, va, l->private_ptr, l->private_int);
+                    fmt, copied_va, l->private_ptr, l->private_int);
+              VA_COPY_END(copied_va);
+          }
 }
 
 
diff --git a/src/va_copy.h b/src/va_copy.h
new file mode 100644
index 0000000..9403c83
--- /dev/null
+++ b/src/va_copy.h
@@ -0,0 +1,40 @@
+/* -*- c-file-style: "java"; indent-tabs-mode: nil; tab-width: 4; fill-column: 78 -*-
+ * Copyright 2007 Google Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
+ * USA.
+ */
+#ifndef VA_COPY_H
+#define VA_COPY_H
+
+#include <stdarg.h>
+
+#ifdef HAVE_VA_COPY
+  /* C99: use va_copy(), and match it with calls to va_end(). */
+  #define VA_COPY(dest, src)      va_copy(dest, src)
+  #define VA_COPY_END(dest)       va_end(dest)
+#elif defined(HAVE_UNDERSCORE_UNDERSCORE_VA_COPY)
+  /* Earlier drafts of the C99 standard used __va_copy(). */
+  #define VA_COPY(dest, src)      __va_copy(dest, src)
+  #define VA_COPY_END(dest)       va_end(dest)
+#else
+  /* Pre-C99: the best we can do is to assume that va_list
+     values can be freely copied.  This works on most (but
+     not all) pre-C99 C implementations. */
+  #define VA_COPY(dest, src)      ((dest) = (src), (void) 0)
+  #define VA_COPY_END(dest)       ((void) 0)
+#endif
+
+#endif /* VA_COPY_H */