Use va_copy() on all platforms; fixed a dangerous low-level bug (#160)

To improve cross-compilation support and to simplify code, rely on C++11
cstdarg header instead of ./configure-time va_copy() detection.

Using ./configure-time detection for va_copy() is dangerous because when
it does not work (e.g., during a poorly configured cross-compilation
attempt), Squid may crash if va_copy() was needed but was not detected.

See also: Bug 4821 and bug 753.

Also found and fixed a low-level bug: StoreEntry::vappendf() was not
using va_copy() because store.cc lacked VA_COPY #defines. The affected
code (900+ callers!) is used for cache manager responses and Gopher
gateway response compilation. If any of those calls required a buffer
larger than 4KB, the lack of those va_copy() calls could lead to crashes
and/or data corruption issues on platforms where va_copy() is required.

diff --git a/acinclude/os-deps.m4 b/acinclude/os-deps.m4
index c1cad524bc594c007626cc7ba4d6e2387f5a09bd..2f25c1609aac2b08021bb117c4f82bddc030eea8 100644 (file)
--- a/acinclude/os-deps.m4
+++ b/acinclude/os-deps.m4
@@ -36,62 +36,6 @@ fi
 
 ]) dnl SQUID_CHECK_FUNC_STRNSTR
 
-dnl check that va_copy is implemented and works
-dnl sets squid_cv_func_va_copy and defines HAVE_VA_COPY
-AC_DEFUN([SQUID_CHECK_FUNC_VACOPY],[
-
-# check that the system provides a functional va_copy call
-
-AH_TEMPLATE(HAVE_VA_COPY, [The system implements a functional va_copy() ])
-AC_CACHE_CHECK(if va_copy is implemented, squid_cv_func_va_copy,
-  AC_RUN_IFELSE([AC_LANG_SOURCE([[
-      #include <stdarg.h>
-      #include <stdlib.h>
-      int f (int i, ...) {
-         va_list args1, args2;
-         va_start (args1, i);
-         va_copy (args2, args1);
-         if (va_arg (args2, int) != 42 || va_arg (args1, int) != 42)
-            return 1;
-         va_end (args1); va_end (args2);
-         return 0;
-      }
-      int main(int argc, char **argv) { return f (0, 42); }
-      ]])],[squid_cv_func_va_copy="yes"],[squid_cv_func_va_copy="no"],[:])
-)
-if test "$squid_cv_func_va_copy" = "yes" ; then
-  AC_DEFINE(HAVE_VA_COPY, 1)
-fi
-
-]) dnl SQUID_CHECK_FUNC_VACOPY
-
-dnl same sa SQUID_CHECK_FUNC_VACOPY, but checks __va_copy
-dnl sets squid_cv_func___va_copy, and defines HAVE___VA_COPY
-AC_DEFUN([SQUID_CHECK_FUNC___VACOPY],[
-
-AH_TEMPLATE(HAVE___VA_COPY,[Some systems have __va_copy instead of va_copy])
-AC_CACHE_CHECK(if __va_copy is implemented, squid_cv_func___va_copy,
-  AC_RUN_IFELSE([AC_LANG_SOURCE([[
-      #include <stdarg.h>
-      #include <stdlib.h>
-      int f (int i, ...) {
-         va_list args1, args2;
-         va_start (args1, i);
-         __va_copy (args2, args1);
-         if (va_arg (args2, int) != 42 || va_arg (args1, int) != 42)
-            return 1;
-         va_end (args1); va_end (args2);
-         return 0;
-      }
-      int main(int argc, char **argv) { return f (0, 42); }
-      ]])],[squid_cv_func___va_copy="yes"],[squid_cv_func___va_copy="no"],[:])
-)
-if test "$squid_cv_func___va_copy" = "yes" ; then
-  AC_DEFINE(HAVE___VA_COPY, 1)
-fi
-]) dnl SQUID_CHECK_FUNC___VACOPY
-
-
 dnl check that epoll actually works
 dnl sets squid_cv_epoll_works to "yes" or "no"
 AC_DEFUN([SQUID_CHECK_EPOLL],[

diff --git a/configure.ac b/configure.ac
index f9e744f65d8793d8e8efd772937b55057faf4def..5347cbf9a7f8f4d4e55753f4f6238fe52b2e1528 100644 (file)
--- a/configure.ac
+++ b/configure.ac
@@ -3489,8 +3489,6 @@ AC_CHECK_TYPE(struct sockaddr_un,AC_DEFINE(HAVE_SOCKADDR_UN,1,[The system provid
 ])
 
 SQUID_CHECK_FUNC_STRNSTR
-SQUID_CHECK_FUNC_VACOPY
-SQUID_CHECK_FUNC___VACOPY
 
 dnl IP-Filter support requires ipf header files. These aren't
 dnl installed by default, so we need to check for them

diff --git a/src/MemBuf.cc b/src/MemBuf.cc
index 08e98f3c1ec5059b07c9bf0f4d51d93e7dd32106..38fbff6743366aa44ab8aa40dc5115be5012fd94 100644 (file)
--- a/src/MemBuf.cc
+++ b/src/MemBuf.cc
@@ -76,15 +76,6 @@
 #include "MemBuf.h"
 #include "profiler/Profiler.h"
 
-#ifdef VA_COPY
-#undef VA_COPY
-#endif
-#if defined HAVE_VA_COPY
-#define VA_COPY va_copy
-#elif defined HAVE___VA_COPY
-#define VA_COPY __va_copy
-#endif
-
 /* local constants */
 
 /* default values for buffer sizes, used by memBufDefInit */
@@ -268,10 +259,6 @@ void MemBuf::terminate()
 void
 MemBuf::vappendf(const char *fmt, va_list vargs)
 {
-#ifdef VA_COPY
-    va_list ap;
-#endif
-
     int sz = 0;
     assert(fmt);
     assert(buf);
@@ -282,18 +269,15 @@ MemBuf::vappendf(const char *fmt, va_list vargs)
         mb_size_t free_space = capacity - size;
         /* put as much as we can */
 
-#ifdef VA_COPY
         /* Fix of bug 753r. The value of vargs is undefined
          * after vsnprintf() returns. Make a copy of vargs
          * incase we loop around and call vsnprintf() again.
          */
-        VA_COPY(ap,vargs);
+        va_list ap;
+        va_copy(ap,vargs);
         sz = vsnprintf(buf + size, free_space, fmt, ap);
         va_end(ap);
-#else /* VA_COPY */
 
-        sz = vsnprintf(buf + size, free_space, fmt, vargs);
-#endif /*VA_COPY*/
         /* check for possible overflow */
         /* snprintf on Linuz returns -1 on overflows */
         /* snprintf on FreeBSD returns at least free_space on overflows */

diff --git a/src/MemStore.cc b/src/MemStore.cc
index 64835ef13d0147cbdddcef66039766f99748b701..f058730c312202c56beb32637a2481e61e22ca33 100644 (file)
--- a/src/MemStore.cc
+++ b/src/MemStore.cc
@@ -108,14 +108,10 @@ void
 ShmWriter::vappendf(const char *fmt, va_list ap)
 {
     SBuf vaBuf;
-#if defined(VA_COPY)
     va_list apCopy;
-    VA_COPY(apCopy, ap);
+    va_copy(apCopy, ap);
     vaBuf.vappendf(fmt, apCopy);
     va_end(apCopy);
-#else
-    vaBuf.vappendf(fmt, ap);
-#endif
     append(vaBuf.rawContent(), vaBuf.length());
 }
 

diff --git a/src/acl/external/eDirectory_userip/ext_edirectory_userip_acl.cc b/src/acl/external/eDirectory_userip/ext_edirectory_userip_acl.cc
index 94a41c6acf07cae53d44398d27d9deeb628d983f..a7e31dbb0d334b379e6694ba330b139833a1d66d 100644 (file)
--- a/src/acl/external/eDirectory_userip/ext_edirectory_userip_acl.cc
+++ b/src/acl/external/eDirectory_userip/ext_edirectory_userip_acl.cc
@@ -53,7 +53,6 @@
 #include <cctype>
 #include <cerrno>
 #include <csignal>
-#include <cstdarg>
 #include <cstdlib>
 #include <cstring>
 #include <ctime>

diff --git a/src/acl/external/time_quota/ext_time_quota_acl.cc b/src/acl/external/time_quota/ext_time_quota_acl.cc
index e96cb216f968d41ffbd932a0af2032cc8f9e926d..764fa862e06c14f9a680e862290df6622051eb2b 100644 (file)
--- a/src/acl/external/time_quota/ext_time_quota_acl.cc
+++ b/src/acl/external/time_quota/ext_time_quota_acl.cc
@@ -29,7 +29,6 @@
 #include "squid.h"
 #include "helper/protocol_defines.h"
 
-#include <cstdarg>
 #include <cstdlib>
 #include <cstring>
 #include <ctime>

diff --git a/src/sbuf/SBuf.cc b/src/sbuf/SBuf.cc
index d72fa02216d5aacff7478a105305245a7366c50c..494e8c5f4bd791e47ed2c000ece257f4069f9a9a 100644 (file)
--- a/src/sbuf/SBuf.cc
+++ b/src/sbuf/SBuf.cc
@@ -19,15 +19,6 @@
 #include <iostream>
 #include <sstream>
 
-#ifdef VA_COPY
-#undef VA_COPY
-#endif
-#if defined HAVE_VA_COPY
-#define VA_COPY va_copy
-#elif defined HAVE___VA_COPY
-#define VA_COPY __va_copy
-#endif
-
 InstanceIdDefinitions(SBuf, "SBuf");
 
 SBufStats SBuf::stats;
@@ -266,14 +257,10 @@ SBuf::vappendf(const char *fmt, va_list vargs)
     size_type requiredSpaceEstimate = strlen(fmt)*2;
 
     char *space = rawSpace(requiredSpaceEstimate);
-#ifdef VA_COPY
     va_list ap;
-    VA_COPY(ap, vargs);
+    va_copy(ap, vargs);
     sz = vsnprintf(space, spaceSize(), fmt, ap);
     va_end(ap);
-#else
-    sz = vsnprintf(space, spaceSize(), fmt, vargs);
-#endif
     Must2(sz >= 0, "vsnprintf() output error");
 
     /* check for possible overflow */

diff --git a/src/sbuf/SBuf.h b/src/sbuf/SBuf.h
index be943649cb6efbd6ed0dc9048f11592df66df64f..607d22a2066dbe21647f60b8ca0eec9f76d58e7a 100644 (file)
--- a/src/sbuf/SBuf.h
+++ b/src/sbuf/SBuf.h
@@ -20,7 +20,6 @@
 #include "sbuf/Stats.h"
 
 #include <climits>
-#include <cstdarg>
 #include <iosfwd>
 #include <iterator>
 #if HAVE_UNISTD_H

diff --git a/src/store.cc b/src/store.cc
index a1f57dee813d6909758d09d12524f303c6902439..2272da704942aff9b3b2024097ae15000efdbc34 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -859,26 +859,18 @@ StoreEntry::vappendf(const char *fmt, va_list vargs)
     *buf = 0;
     int x;
 
-#ifdef VA_COPY
-    va_args ap;
+    va_list ap;
     /* Fix of bug 753r. The value of vargs is undefined
      * after vsnprintf() returns. Make a copy of vargs
      * incase we loop around and call vsnprintf() again.
      */
-    VA_COPY(ap,vargs);
+    va_copy(ap,vargs);
     errno = 0;
     if ((x = vsnprintf(buf, sizeof(buf), fmt, ap)) < 0) {
         fatal(xstrerr(errno));
         return;
     }
     va_end(ap);
-#else /* VA_COPY */
-    errno = 0;
-    if ((x = vsnprintf(buf, sizeof(buf), fmt, vargs)) < 0) {
-        fatal(xstrerr(errno));
-        return;
-    }
-#endif /*VA_COPY*/
 
     if (x < static_cast<int>(sizeof(buf))) {
         append(buf, x);