THRIFT-3873: fix various compiler warnings and overflow errors
THRIFT-3847: change VERSION to PACKAGE_VERSION to avoid conflicts with third party or OS headers
This closes #1128
diff --git a/lib/cpp/src/thrift/protocol/THeaderProtocol.h b/lib/cpp/src/thrift/protocol/THeaderProtocol.h
index e7d4bd6..b01bfb6 100644
--- a/lib/cpp/src/thrift/protocol/THeaderProtocol.h
+++ b/lib/cpp/src/thrift/protocol/THeaderProtocol.h
@@ -46,7 +46,7 @@
explicit THeaderProtocol(const boost::shared_ptr<TTransport>& trans,
uint16_t protoId = T_COMPACT_PROTOCOL)
: TVirtualProtocol<THeaderProtocol>(boost::shared_ptr<TTransport>(new THeaderTransport(trans))),
- trans_(boost::dynamic_pointer_cast<THeaderTransport>(this->getTransport())),
+ trans_(boost::dynamic_pointer_cast<THeaderTransport>(getTransport())),
protoId_(protoId) {
trans_->setProtocolId(protoId);
resetProtocol();
@@ -57,7 +57,7 @@
uint16_t protoId = T_COMPACT_PROTOCOL)
: TVirtualProtocol<THeaderProtocol>(
boost::shared_ptr<TTransport>(new THeaderTransport(inTrans, outTrans))),
- trans_(boost::dynamic_pointer_cast<THeaderTransport>(this->getTransport())),
+ trans_(boost::dynamic_pointer_cast<THeaderTransport>(getTransport())),
protoId_(protoId) {
trans_->setProtocolId(protoId);
resetProtocol();
diff --git a/lib/cpp/src/thrift/protocol/TJSONProtocol.cpp b/lib/cpp/src/thrift/protocol/TJSONProtocol.cpp
index d3ec722..431e7c4 100644
--- a/lib/cpp/src/thrift/protocol/TJSONProtocol.cpp
+++ b/lib/cpp/src/thrift/protocol/TJSONProtocol.cpp
@@ -524,7 +524,7 @@
std::string doubleToString(double d) {
std::ostringstream str;
str.imbue(std::locale::classic());
- const int max_digits10 = 2 + std::numeric_limits<double>::digits10;
+ const std::streamsize max_digits10 = 2 + std::numeric_limits<double>::digits10;
str.precision(max_digits10);
str << d;
return str.str();
diff --git a/lib/cpp/src/thrift/server/TNonblockingServer.cpp b/lib/cpp/src/thrift/server/TNonblockingServer.cpp
index ccc37a2..537770b 100644
--- a/lib/cpp/src/thrift/server/TNonblockingServer.cpp
+++ b/lib/cpp/src/thrift/server/TNonblockingServer.cpp
@@ -17,8 +17,6 @@
* under the License.
*/
-#define __STDC_FORMAT_MACROS
-
#include <thrift/thrift-config.h>
#include <thrift/server/TNonblockingServer.h>
@@ -64,13 +62,12 @@
#define AF_LOCAL AF_UNIX
#endif
-#if !defined(PRIu32)
-#define PRIu32 "I32u"
-#define PRIu64 "I64u"
+#ifdef HAVE_INTTYPES_H
+#include <inttypes.h>
#endif
-#if defined(_WIN32) && (_WIN32_WINNT < 0x0600)
- #define AI_ADDRCONFIG 0x0400
+#ifdef HAVE_STDINT_H
+#include <stdint.h>
#endif
namespace apache {
diff --git a/lib/cpp/src/thrift/transport/THeaderTransport.cpp b/lib/cpp/src/thrift/transport/THeaderTransport.cpp
index b1fe923..f222910 100644
--- a/lib/cpp/src/thrift/transport/THeaderTransport.cpp
+++ b/lib/cpp/src/thrift/transport/THeaderTransport.cpp
@@ -23,11 +23,11 @@
#include <thrift/protocol/TBinaryProtocol.h>
#include <thrift/protocol/TCompactProtocol.h>
+#include <limits>
#include <utility>
-#include <cassert>
#include <string>
-#include <zlib.h>
#include <string.h>
+#include <zlib.h>
using std::map;
using boost::shared_ptr;
@@ -172,7 +172,7 @@
* Reads a string from ptr, taking care not to reach headerBoundary
* Advances ptr on success
*
- * @param str output string
+ * @param str output string
* @throws CORRUPTED_DATA if size of string exceeds boundary
*/
void THeaderTransport::readString(uint8_t*& ptr,
@@ -198,7 +198,10 @@
uint8_t* ptr = reinterpret_cast<uint8_t*>(rBuf_.get() + 10);
// Catch integer overflow, check for reasonable header size
- assert(headerSize < 16384);
+ if (headerSize >= 16384) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header size is unreasonable");
+ }
headerSize *= 4;
const uint8_t* const headerBoundary = ptr + headerSize;
if (headerSize > sz) {
@@ -252,7 +255,7 @@
}
// Untransform the data section. rBuf will contain result.
- untransform(data, sz - (data - rBuf_.get())); // ignore header in size calc
+ untransform(data, safe_numeric_cast<uint32_t>(static_cast<ptrdiff_t>(sz) - (data - rBuf_.get())));
}
void THeaderTransport::untransform(uint8_t* ptr, uint32_t sz) {
@@ -375,7 +378,7 @@
}
uint32_t THeaderTransport::getWriteBytes() {
- return wBase_ - wBuf_.get();
+ return safe_numeric_cast<uint32_t>(wBase_ - wBuf_.get());
}
/**
@@ -384,7 +387,7 @@
* Automatically advances ptr to after the written portion
*/
void THeaderTransport::writeString(uint8_t*& ptr, const string& str) {
- uint32_t strLen = str.length();
+ int32_t strLen = safe_numeric_cast<int32_t>(str.length());
ptr += writeVarint32(strLen, ptr);
memcpy(ptr, str.c_str(), strLen); // no need to write \0
ptr += strLen;
@@ -394,7 +397,7 @@
writeHeaders_[key] = value;
}
-size_t THeaderTransport::getMaxWriteHeadersSize() const {
+uint32_t THeaderTransport::getMaxWriteHeadersSize() const {
size_t maxWriteHeadersSize = 0;
THeaderTransport::StringToStringMap::const_iterator it;
for (it = writeHeaders_.begin(); it != writeHeaders_.end(); ++it) {
@@ -402,7 +405,7 @@
// 2 varints32 + the strings themselves
maxWriteHeadersSize += 5 + 5 + (it->first).length() + (it->second).length();
}
- return maxWriteHeadersSize;
+ return safe_numeric_cast<uint32_t>(maxWriteHeadersSize);
}
void THeaderTransport::clearHeaders() {
@@ -431,7 +434,7 @@
if (clientType == THRIFT_HEADER_CLIENT_TYPE) {
// header size will need to be updated at the end because of varints.
// Make it big enough here for max varint size, plus 4 for padding.
- int headerSize = (2 + getNumTransforms()) * THRIFT_MAX_VARINT32_BYTES + 4;
+ uint32_t headerSize = (2 + getNumTransforms()) * THRIFT_MAX_VARINT32_BYTES + 4;
// add approximate size of info headers
headerSize += getMaxWriteHeadersSize();
@@ -479,11 +482,11 @@
// write info headers
// for now only write kv-headers
- uint16_t headerCount = writeHeaders_.size();
+ int32_t headerCount = safe_numeric_cast<int32_t>(writeHeaders_.size());
if (headerCount > 0) {
pkt += writeVarint32(infoIdType::KEYVALUE, pkt);
// Write key-value headers count
- pkt += writeVarint32(headerCount, pkt);
+ pkt += writeVarint32(static_cast<int32_t>(headerCount), pkt);
// Write info headers
map<string, string>::const_iterator it;
for (it = writeHeaders_.begin(); it != writeHeaders_.end(); ++it) {
@@ -494,7 +497,7 @@
}
// Fixups after varint size calculations
- headerSize = (pkt - headerStart);
+ headerSize = safe_numeric_cast<uint32_t>(pkt - headerStart);
uint8_t padding = 4 - (headerSize % 4);
headerSize += padding;
@@ -504,8 +507,13 @@
}
// Pkt size
+ ptrdiff_t szHbp = (headerStart - pktStart - 4);
+ if (static_cast<uint64_t>(szHbp) > static_cast<uint64_t>(std::numeric_limits<uint32_t>().max()) - (headerSize + haveBytes)) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "Header section size is unreasonable");
+ }
szHbo = headerSize + haveBytes // thrift header + payload
- + (headerStart - pktStart - 4); // common header section
+ + static_cast<uint32_t>(szHbp); // common header section
headerSizeN = htons(headerSize / 4);
memcpy(headerSizePtr, &headerSizeN, sizeof(headerSizeN));
diff --git a/lib/cpp/src/thrift/transport/THeaderTransport.h b/lib/cpp/src/thrift/transport/THeaderTransport.h
index bf82674..af20fe3 100644
--- a/lib/cpp/src/thrift/transport/THeaderTransport.h
+++ b/lib/cpp/src/thrift/transport/THeaderTransport.h
@@ -21,11 +21,18 @@
#define THRIFT_TRANSPORT_THEADERTRANSPORT_H_ 1
#include <bitset>
+#include <limits>
#include <vector>
#include <stdexcept>
#include <string>
#include <map>
+#ifdef HAVE_STDINT_H
+#include <stdint.h>
+#elif HAVE_INTTYPES_H
+#include <inttypes.h>
+#endif
+
#include <boost/scoped_array.hpp>
#include <boost/shared_ptr.hpp>
@@ -135,8 +142,7 @@
void transform(uint8_t* ptr, uint32_t sz);
uint16_t getNumTransforms() const {
- int trans = writeTrans_.size();
- return trans;
+ return safe_numeric_cast<uint16_t>(writeTrans_.size());
}
void setTransform(uint16_t transId) { writeTrans_.push_back(transId); }
@@ -204,7 +210,7 @@
/**
* Returns the maximum number of bytes that write k/v headers can take
*/
- size_t getMaxWriteHeadersSize() const;
+ uint32_t getMaxWriteHeadersSize() const;
struct infoIdType {
enum idType {
diff --git a/lib/cpp/src/thrift/transport/THttpClient.cpp b/lib/cpp/src/thrift/transport/THttpClient.cpp
index c610636..732c7e4 100644
--- a/lib/cpp/src/thrift/transport/THttpClient.cpp
+++ b/lib/cpp/src/thrift/transport/THttpClient.cpp
@@ -22,6 +22,7 @@
#include <sstream>
#include <boost/algorithm/string.hpp>
+#include <thrift/config.h>
#include <thrift/transport/THttpClient.h>
#include <thrift/transport/TSocket.h>
@@ -102,7 +103,7 @@
std::ostringstream h;
h << "POST " << path_ << " HTTP/1.1" << CRLF << "Host: " << host_ << CRLF
<< "Content-Type: application/x-thrift" << CRLF << "Content-Length: " << len << CRLF
- << "Accept: application/x-thrift" << CRLF << "User-Agent: Thrift/" << VERSION
+ << "Accept: application/x-thrift" << CRLF << "User-Agent: Thrift/" << PACKAGE_VERSION
<< " (C++/THttpClient)" << CRLF << CRLF;
string header = h.str();
diff --git a/lib/cpp/src/thrift/transport/THttpServer.cpp b/lib/cpp/src/thrift/transport/THttpServer.cpp
index c89ab94..3bf3053 100644
--- a/lib/cpp/src/thrift/transport/THttpServer.cpp
+++ b/lib/cpp/src/thrift/transport/THttpServer.cpp
@@ -21,6 +21,7 @@
#include <sstream>
#include <iostream>
+#include <thrift/config.h>
#include <thrift/transport/THttpServer.h>
#include <thrift/transport/TSocket.h>
#if defined(_MSC_VER) || defined(__MINGW32__)
@@ -124,7 +125,7 @@
// Construct the HTTP header
std::ostringstream h;
h << "HTTP/1.1 200 OK" << CRLF << "Date: " << getTimeRFC1123() << CRLF << "Server: Thrift/"
- << VERSION << CRLF << "Access-Control-Allow-Origin: *" << CRLF
+ << PACKAGE_VERSION << CRLF << "Access-Control-Allow-Origin: *" << CRLF
<< "Content-Type: application/x-thrift" << CRLF << "Content-Length: " << len << CRLF
<< "Connection: Keep-Alive" << CRLF << CRLF;
string header = h.str();
diff --git a/lib/cpp/src/thrift/transport/TServerSocket.cpp b/lib/cpp/src/thrift/transport/TServerSocket.cpp
index 87b6383..8b65319 100644
--- a/lib/cpp/src/thrift/transport/TServerSocket.cpp
+++ b/lib/cpp/src/thrift/transport/TServerSocket.cpp
@@ -60,10 +60,6 @@
#endif // _WIN32
#endif
-#if defined(_WIN32) && (_WIN32_WINNT < 0x0600)
- #define AI_ADDRCONFIG 0x0400
-#endif
-
template <class T>
inline const SOCKOPT_CAST_T* const_cast_sockopt(const T* v) {
return reinterpret_cast<const SOCKOPT_CAST_T*>(v);
diff --git a/lib/cpp/src/thrift/transport/TSocket.cpp b/lib/cpp/src/thrift/transport/TSocket.cpp
index e1c106a..9fad590 100644
--- a/lib/cpp/src/thrift/transport/TSocket.cpp
+++ b/lib/cpp/src/thrift/transport/TSocket.cpp
@@ -53,10 +53,6 @@
#endif // _WIN32
#endif
-#if defined(_WIN32) && (_WIN32_WINNT < 0x0600)
- #define AI_ADDRCONFIG 0x0400
-#endif
-
template <class T>
inline const SOCKOPT_CAST_T* const_cast_sockopt(const T* v) {
return reinterpret_cast<const SOCKOPT_CAST_T*>(v);
diff --git a/lib/cpp/src/thrift/transport/TTransportException.h b/lib/cpp/src/thrift/transport/TTransportException.h
index 83e876a..dbbb971 100644
--- a/lib/cpp/src/thrift/transport/TTransportException.h
+++ b/lib/cpp/src/thrift/transport/TTransportException.h
@@ -20,6 +20,7 @@
#ifndef _THRIFT_TRANSPORT_TTRANSPORTEXCEPTION_H_
#define _THRIFT_TRANSPORT_TTRANSPORTEXCEPTION_H_ 1
+#include <boost/numeric/conversion/cast.hpp>
#include <string>
#include <thrift/Thrift.h>
@@ -83,6 +84,21 @@
/** Error code */
TTransportExceptionType type_;
};
+
+/**
+ * Legacy code in transport implementations have overflow issues
+ * that need to be enforced.
+ */
+template <typename To, typename From> To safe_numeric_cast(From i) {
+ try {
+ return boost::numeric_cast<To>(i);
+ }
+ catch (const std::bad_cast& bc) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ bc.what());
+ }
+}
+
}
}
} // apache::thrift::transport
diff --git a/lib/cpp/src/thrift/windows/config.h b/lib/cpp/src/thrift/windows/config.h
index 674c260..cde9de6 100644
--- a/lib/cpp/src/thrift/windows/config.h
+++ b/lib/cpp/src/thrift/windows/config.h
@@ -36,6 +36,9 @@
#define USE_BOOST_THREAD 1
#endif
+// Something that defines PRId64 is required to build
+#define HAVE_INTTYPES_H 1
+
// VS2010 or later has stdint.h
#if _MSC_VER >= 1600
#define HAVE_STDINT_H 1
@@ -65,7 +68,6 @@
#pragma warning(disable : 4996) // Deprecated posix name.
-#define VERSION "1.0.0-dev"
#define HAVE_GETTIMEOFDAY 1
#define HAVE_SYS_STAT_H 1
diff --git a/lib/cpp/test/AllProtocolTests.tcc b/lib/cpp/test/AllProtocolTests.tcc
index 8c8eaa8..3e0b833 100644
--- a/lib/cpp/test/AllProtocolTests.tcc
+++ b/lib/cpp/test/AllProtocolTests.tcc
@@ -190,14 +190,14 @@
testNaked<TProto, int64_t>(0);
for (int64_t i = 0; i < 62; i++) {
- testNaked<TProto, int64_t>(1L << i);
- testNaked<TProto, int64_t>(-(1L << i));
+ testNaked<TProto, int64_t>(1LL << i);
+ testNaked<TProto, int64_t>(-(1LL << i));
}
testField<TProto, T_I64, int64_t>(0);
for (int i = 0; i < 62; i++) {
- testField<TProto, T_I64, int64_t>(1L << i);
- testField<TProto, T_I64, int64_t>(-(1L << i));
+ testField<TProto, T_I64, int64_t>(1LL << i);
+ testField<TProto, T_I64, int64_t>(-(1LL << i));
}
testNaked<TProto, double>(123.456);
diff --git a/lib/cpp/test/CMakeLists.txt b/lib/cpp/test/CMakeLists.txt
index cbeff08..74b0a09 100644
--- a/lib/cpp/test/CMakeLists.txt
+++ b/lib/cpp/test/CMakeLists.txt
@@ -19,7 +19,13 @@
include_directories(SYSTEM "${Boost_INCLUDE_DIRS}")
-#Make sure gen-cpp files can be included
+add_definitions("-D__STDC_LIMIT_MACROS")
+
+if (WITH_DYN_LINK_TEST)
+ add_definitions( -DBOOST_TEST_DYN_LINK )
+endif()
+
+# Make sure gen-cpp files can be included
include_directories("${CMAKE_CURRENT_BINARY_DIR}")
# Create the thrift C++ test library
@@ -54,7 +60,6 @@
)
add_library(testgencpp_cob STATIC ${testgencpp_cob_SOURCES})
-
add_executable(Benchmark Benchmark.cpp)
target_link_libraries(Benchmark testgencpp)
LINK_AGAINST_THRIFT_LIBRARY(Benchmark thrift)
diff --git a/lib/cpp/test/JSONProtoTest.cpp b/lib/cpp/test/JSONProtoTest.cpp
index 301a064..2da3044 100644
--- a/lib/cpp/test/JSONProtoTest.cpp
+++ b/lib/cpp/test/JSONProtoTest.cpp
@@ -262,8 +262,9 @@
":[\"i8\",3,1,2,3]},\"13\":{\"lst\":[\"i16\",3,1,2,3]},\"14\":{\"lst\":[\"i64"
"\",3,1,2,3]}}";
+ const std::size_t bufSiz = strlen(json_string) * sizeof(char);
boost::shared_ptr<TMemoryBuffer> buffer(new TMemoryBuffer(
- (uint8_t*)(json_string), strlen(json_string)*sizeof(char)));
+ (uint8_t*)(json_string), static_cast<uint32_t>(bufSiz)));
boost::shared_ptr<TJSONProtocol> proto(new TJSONProtocol(buffer));
OneOfEach ooe2;
diff --git a/lib/cpp/test/ZlibTest.cpp b/lib/cpp/test/ZlibTest.cpp
index e155970..a4387a9 100644
--- a/lib/cpp/test/ZlibTest.cpp
+++ b/lib/cpp/test/ZlibTest.cpp
@@ -17,13 +17,19 @@
* under the License.
*/
-#define __STDC_FORMAT_MACROS
-
#ifndef _GNU_SOURCE
#define _GNU_SOURCE // needed for getopt_long
#endif
+#if (_MSC_VER <= 1700)
+// polynomial and std::fill_t warning happens in MSVC 2010, 2013, maybe others
+// https://svn.boost.org/trac/boost/ticket/11426
+#pragma warning(disable:4996)
+#endif
+
+#ifdef HAVE_STDINT_H
#include <stdint.h>
+#endif
#ifdef HAVE_INTTYPES_H
#include <inttypes.h>
#endif
diff --git a/lib/cpp/test/processor/ServerThread.cpp b/lib/cpp/test/processor/ServerThread.cpp
index e9d468f..009c4c6 100644
--- a/lib/cpp/test/processor/ServerThread.cpp
+++ b/lib/cpp/test/processor/ServerThread.cpp
@@ -35,6 +35,8 @@
assert(!running_);
running_ = true;
+ helper_.reset(new Helper(this));
+
// Start the other thread
concurrency::PlatformThreadFactory threadFactory;
threadFactory.setDetached(false);
diff --git a/lib/cpp/test/processor/ServerThread.h b/lib/cpp/test/processor/ServerThread.h
index eed3469..3803e7e 100644
--- a/lib/cpp/test/processor/ServerThread.h
+++ b/lib/cpp/test/processor/ServerThread.h
@@ -71,8 +71,7 @@
class ServerThread {
public:
ServerThread(const boost::shared_ptr<ServerState>& state, bool autoStart)
- : helper_(new Helper(this)),
- port_(0),
+ : port_(0),
running_(false),
serving_(false),
error_(false),