From: Amos Jeffries Date: Thu, 1 Mar 2018 23:48:28 +0000 (+1300) Subject: Use va_copy() on all platforms; fixed a dangerous low-level bug (#160) X-Git-Tag: SQUID_4_0_24~2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e40e3c89a0e6b2b76fd6756242867c30aa055d7e;p=thirdparty%2Fsquid.git 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 c1cad524bc..2f25c1609a 100644 --- 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 - #include - 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 - #include - 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 2c66ad0c74..258ad50592 100644 --- a/configure.ac +++ b/configure.ac @@ -3526,8 +3526,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 08e98f3c1e..38fbff6743 100644 --- 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 06b253fc96..23ffaad4f2 100644 --- 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 7dead69ebb..dfbfeb0120 100644 --- 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 #include #include -#include #include #include #include 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 a2ba3afe73..c6d500d4c1 100644 --- 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 #include #include #include diff --git a/src/sbuf/SBuf.cc b/src/sbuf/SBuf.cc index 46c55cf656..853b84e628 100644 --- a/src/sbuf/SBuf.cc +++ b/src/sbuf/SBuf.cc @@ -19,15 +19,6 @@ #include #include -#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 13aa524886..add7a727c4 100644 --- a/src/sbuf/SBuf.h +++ b/src/sbuf/SBuf.h @@ -20,7 +20,6 @@ #include "sbuf/Stats.h" #include -#include #include #include #if HAVE_UNISTD_H diff --git a/src/store.cc b/src/store.cc index 8e681ca514..33c5d4f172 100644 --- 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(sizeof(buf))) { append(buf, x);