From 8cd723d91b78c8ac78490714cebe3917b4a4cc31 Mon Sep 17 00:00:00 2001 From: Florent Rougon Date: Mon, 13 Feb 2017 08:55:07 +0100 Subject: [PATCH] Fix build errors in zlibstream*.cxx, re-enable their compilation Compilation of these files was disabled in commit e21ad4b5c189022d64483bdbdb321d09a90f47bd. Fix build errors and warnings: - Ambiguous template parameters for std::min(); - No appropriate default constructor available for std::basic_istream> (the std::istream subclasses didn't explicitly call the std::istream constructor, which requires an argument). This is presumably exactly the reason why sg_gzifstream is declared like this: class sg_gzifstream : private gzifstream_base, public std::istream where gzifstream_base is an empty shell for a stream buffer object: struct gzifstream_base { gzifstream_base() {} gzfilebuf gzbuf; }; This ensures that the stream buffer object (gzbuf) is initialized before std::istream's constructor is called. I solved this problem in a different way, hopefully easier to understand, and requiring neither an additional class nor multiple inheritance: first, we initialize the std::istream base with a nullptr as the std::streambuf * argument (this is valid C++11), then in the constructor bodies for ZlibCompressorIStream and ZlibDecompressorIStream, we call std::istream::rdbuf() to attach the std::istream instance to the now-initialized stream buffer object. - Possible truncation of constant value on 32 bits systems (this was in zlibMaxChunkSize() which is now removed, see below). Type-related improvements: - Remove zlibMaxChunkSize() and zlibMaxChunk: in C++, one can simply use std::numeric_limits::max()---most of the code in zlibMaxChunkSize() was there only to find this value via a zlib function call. - Add helper function templates zlibChunk() and clipCast(). - Split preparation of the putback area out of ZlibAbstractIStreambuf::xsgetn() to a new utility method: xsgetn_preparePutbackArea(). - More rigorous type handling in zlibstream_test.cxx. Some precautions are necessary because the IOStreams API uses std::streamsize in many places (e.g., the return value of std::istream::gcount()), but functions such as the following std::string constructor: std::string(const char* s, std::size_t n); work with std::size_t instead. Since these types are different and opaque, this requires some care! --- simgear/io/iostreams/CMakeLists.txt | 6 + simgear/io/iostreams/zlibstream.cxx | 189 +++++++++++++---------- simgear/io/iostreams/zlibstream.hxx | 3 + simgear/io/iostreams/zlibstream_test.cxx | 41 +++-- 4 files changed, 150 insertions(+), 89 deletions(-) diff --git a/simgear/io/iostreams/CMakeLists.txt b/simgear/io/iostreams/CMakeLists.txt index 6c87ae22..37be39c7 100644 --- a/simgear/io/iostreams/CMakeLists.txt +++ b/simgear/io/iostreams/CMakeLists.txt @@ -4,12 +4,14 @@ set(HEADERS sgstream.hxx gzfstream.hxx gzcontainerfile.hxx + zlibstream.hxx ) set(SOURCES sgstream.cxx gzfstream.cxx gzcontainerfile.cxx + zlibstream.cxx ) simgear_component(IOStreams io/iostreams "${SOURCES}" "${HEADERS}") @@ -20,4 +22,8 @@ if(ENABLE_TESTS) target_link_libraries(test_streams ${TEST_LIBS}) add_test(streams ${EXECUTABLE_OUTPUT_PATH}/test_streams) + add_executable(test_zlibstream zlibstream_test.cxx) + target_link_libraries(test_zlibstream ${TEST_LIBS}) + add_test(zlibstream ${EXECUTABLE_OUTPUT_PATH}/test_zlibstream) + endif(ENABLE_TESTS) diff --git a/simgear/io/iostreams/zlibstream.cxx b/simgear/io/iostreams/zlibstream.cxx index b7cafcc8..577048b4 100644 --- a/simgear/io/iostreams/zlibstream.cxx +++ b/simgear/io/iostreams/zlibstream.cxx @@ -27,6 +27,7 @@ #include #include #include // std::numeric_limits +#include // std::make_unsigned() #include // std::size_t, std::ptrdiff_t #include @@ -75,38 +76,53 @@ static string zlibErrorMessage(const z_stream& zstream, int errorCode) return res; } -// Return the largest value that can be represented with zlib's uInt type, -// which is the type of z_stream.avail_in and z_stream.avail_out, hence the -// function name. -static std::size_t zlibMaxChunkSize() +// Requirement: 'val' must be non-negative. +// +// Return: +// - 'val' cast as BoundType if it's lower than the largest value BoundType +// can represent; +// - this largest value otherwise. +template +static BoundType clipCast(T val) { - uLong flags = ::zlibCompileFlags(); - std::size_t res; + typedef typename std::make_unsigned::type uT; + typedef typename std::make_unsigned::type uBoundType; + assert(val >= 0); // otherwise, the comparison and cast to uT would be unsafe - switch (flags & 0x3) { - case 0x3: - SG_LOG(SG_IO, SG_WARN, - "Unknown size for zlib's uInt type (code 3). Will assume 64 bits, " - "but the actual value is probably higher."); - // No 'break' here, this is intentional. - case 0x2: - res = 0xFFFFFFFFFFFFFFFF; // 2^64 - 1 - break; - case 0x1: - res = 0xFFFFFFFF; // 2^32 - 1 - break; - case 0x0: - res = 0xFFFF; // 2^16 - 1 - break; - default: - throw std::logic_error("It should be impossible to get here."); + // Casts to avoid the signed-compare warning; they don't affect the values, + // since both are non-negative. + if (static_cast(val) < + static_cast( std::numeric_limits::max() )) { + return static_cast(val); + } else { + return std::numeric_limits::max(); } - - return res; } -static const std::size_t zlibMaxChunk = ::zlibMaxChunkSize(); +// Requirement: 'size' must be non-negative. +// +// Return: +// - 'size' if it is lower than or equal to std::numeric_limits::max(); +// - std::numeric_limits::max() cast as a T otherwise (this is always +// possible in a lossless way, since in this case, one has +// 0 <= std::numeric_limits::max() < size, and 'size' is of type T). +// +// Note: uInt is the type of z_stream.avail_in and z_stream.avail_out, hence +// the function name. +template +static T zlibChunk(T size) +{ + typedef typename std::make_unsigned::type uT; + assert(size >= 0); // otherwise, the comparison and cast to uT would be unsafe + if (static_cast(size) <= std::numeric_limits::max()) { + return size; + } else { + // In this case, we are sure that T can represent + // std::numeric_limits::max(), thus the cast is safe. + return static_cast(std::numeric_limits::max()); + } +} namespace simgear { @@ -214,8 +230,8 @@ int ZlibAbstractIStreambuf::underflow() std::size_t ZlibAbstractIStreambuf::fOB_remainingSpace(unsigned char* nextOutPtr) const { - std::ptrdiff_t remainingSpaceInOutBuf = _outBuf + _outBufSize - - reinterpret_cast(nextOutPtr); + std::ptrdiff_t remainingSpaceInOutBuf = ( + _outBuf + _outBufSize - reinterpret_cast(nextOutPtr)); assert(remainingSpaceInOutBuf >= 0); return static_cast(remainingSpaceInOutBuf); @@ -265,9 +281,7 @@ char* ZlibAbstractIStreambuf::fillOutputBuffer() remainingSpaceInOutBuf = fOB_remainingSpace(_zstream.next_out); while (remainingSpaceInOutBuf > 0) { - // This does fit in a zlib uInt: that's the whole point of zlibMaxChunk. - _zstream.avail_out = static_cast( - std::min(remainingSpaceInOutBuf, zlibMaxChunk)); + _zstream.avail_out = clipCast(remainingSpaceInOutBuf); if (_zstream.avail_in == 0 && !allInputRead) { // Get data from _iStream, store it in _inBuf @@ -327,9 +341,7 @@ bool ZlibAbstractIStreambuf::getInputData() // Data already available? if (alreadyAvailable > 0) { - _zstream.avail_in = static_cast( - std::min(static_cast(alreadyAvailable), - zlibMaxChunk)); + _zstream.avail_in = clipCast(alreadyAvailable); return allInputRead; } @@ -340,9 +352,8 @@ bool ZlibAbstractIStreambuf::getInputData() // Fill the input buffer (as much as possible) while (_inBufEndPtr < _inBuf + _inBufSize && !_iStream.eof()) { - std::streamsize nbCharsToRead = std::min( - _inBuf + _inBufSize - _inBufEndPtr, - std::numeric_limits::max()); // max we can pass to read() + std::streamsize nbCharsToRead = clipCast( + _inBuf + _inBufSize - _inBufEndPtr); _iStream.read(_inBufEndPtr, nbCharsToRead); if (_iStream.bad()) { @@ -361,10 +372,8 @@ bool ZlibAbstractIStreambuf::getInputData() std::ptrdiff_t availableChars = _inBufEndPtr - reinterpret_cast(_zstream.next_in); - assert(availableChars >= 0); - _zstream.avail_in = static_cast( - std::min(static_cast(availableChars), - zlibMaxChunk)); + // assert(availableChars >= 0); <-- already done in clipCast() + _zstream.avail_in = clipCast(availableChars); if (_iStream.eof()) { allInputRead = true; @@ -384,24 +393,34 @@ bool ZlibAbstractIStreambuf::getInputData() // (_outBuf) before being copied to its (hopefully) final destination. std::streamsize ZlibAbstractIStreambuf::xsgetn(char* dest, std::streamsize n) { - std::size_t remaining = static_cast(n); - std::size_t chunkSize; - std::ptrdiff_t avail; - const char* origGptr = gptr(); + // Despite the somewhat misleading footnote 296 of ยง27.5.3 of the C++11 + // standard, one can't assume std::size_t to be at least as large as + // std::streamsize (64 bits std::streamsize in a 32 bits Windows program). + std::streamsize remaining = n; + char* origGptr = gptr(); char* writePtr = dest; // we'll need dest later -> work with a copy // First, let's take data present in our internal buffer (_outBuf) while (remaining > 0) { - avail = egptr() - gptr(); // number of available chars in _outBuf + // Number of available chars in _outBuf + std::ptrdiff_t avail = egptr() - gptr(); if (avail == 0) { // our internal buffer is empty break; } - chunkSize = std::min(remaining, static_cast(avail)); - std::copy(gptr(), gptr() + chunkSize, writePtr); - gbump(chunkSize); - writePtr += chunkSize; - remaining -= chunkSize; + // We need an int for gbump(), at least in C++11. + int chunkSize_i = clipCast(avail); + if (chunkSize_i > remaining) { + chunkSize_i = static_cast(remaining); + } + assert(chunkSize_i >= 0); + + std::copy(gptr(), gptr() + chunkSize_i, writePtr); + gbump(chunkSize_i); + writePtr += chunkSize_i; + // This cast is okay because 0 <= chunkSize_i <= remaining, which is an + // std::streamsize + remaining -= static_cast(chunkSize_i); } if (remaining == 0) { @@ -418,9 +437,10 @@ std::streamsize ZlibAbstractIStreambuf::xsgetn(char* dest, std::streamsize n) int retCode; while (remaining > 0) { - chunkSize = std::min(remaining, zlibMaxChunk); - // It does fit in a zlib uInt: that's the whole point of zlibMaxChunk. - _zstream.avail_out = static_cast(chunkSize); + std::streamsize chunkSize_s = zlibChunk(remaining); + // chunkSize_s > 0 and does fit in a zlib uInt: that's the whole point of + // zlibChunk. + _zstream.avail_out = static_cast(chunkSize_s); if (_zstream.avail_in == 0 && !allInputRead) { allInputRead = getInputData(); @@ -429,8 +449,9 @@ std::streamsize ZlibAbstractIStreambuf::xsgetn(char* dest, std::streamsize n) // Make zlib process some data (compress or decompress). This updates // _zstream.{avail,next}_{in,out} (4 fields of the z_stream struct). retCode = zlibProcessData(); - // chunkSize - _zstream.avail_out is the nb of chars written by zlib - remaining -= chunkSize - _zstream.avail_out; + // chunkSize_s - _zstream.avail_out is the number of chars written by zlib. + // 0 <= _zstream.avail_out <= chunkSize_s, which is an std::streamsize. + remaining -= chunkSize_s - static_cast(_zstream.avail_out); if (retCode == Z_BUF_ERROR) { handleZ_BUF_ERROR(); // doesn't return @@ -444,12 +465,23 @@ std::streamsize ZlibAbstractIStreambuf::xsgetn(char* dest, std::streamsize n) } } - // Finally, prepare the putback area. - // - // We could use reinterpret_cast(_zstream.next_out) everywhere below, - // except it is hardly readable. This points after the latest data we wrote. - writePtr = reinterpret_cast(_zstream.next_out); + // Finally, copy chars to the putback area. + std::size_t nbPutbackChars = xsgetn_preparePutbackArea( + origGptr, dest, reinterpret_cast(_zstream.next_out)); + setg(_outBuf + _putbackSize - nbPutbackChars, // start of putback area + _outBuf + _putbackSize, // the buffer for pending, + _outBuf + _putbackSize); // available data is empty + assert(remaining >= 0); + assert(n - remaining >= 0); + // Total number of chars copied. + return n - remaining; +} + +// Utility method for xsgetn(): copy some chars to the putback area +std::size_t ZlibAbstractIStreambuf::xsgetn_preparePutbackArea( + char* origGptr, char* dest, char* writePtr) +{ // There are two buffers containing characters we potentially have to copy // to the putback area: the one starting at _outBuf and the one starting at // dest. In the following diagram, ***** represents those from _outBuf, @@ -472,38 +504,33 @@ std::streamsize ZlibAbstractIStreambuf::xsgetn(char* dest, std::streamsize n) // // [1] This means that the last char represented by a star is at address // origGptr-1. + + // It seems std::ptrdiff_t is the signed counterpart of std::size_t, + // therefore this should always hold (even with equality). + static_assert(sizeof(std::size_t) >= sizeof(std::ptrdiff_t), + "Unexpected: sizeof(std::size_t) < sizeof(std::ptrdiff_t)"); assert(writePtr - dest >= 0); std::size_t inDestBuffer = static_cast(writePtr - dest); + assert(origGptr - eback() >= 0); std::size_t nbPutbackChars = std::min( static_cast(origGptr - eback()) + inDestBuffer, _putbackSize); std::size_t nbPutbackCharsToGo = nbPutbackChars; - // chunkSize has an unsigned type; precomputing it before the following test - // wouldn't work. // Are there chars in _outBuf that need to be copied to the putback area? if (nbPutbackChars > inDestBuffer) { - chunkSize = nbPutbackChars - inDestBuffer; // yes, this number + std::size_t chunkSize = nbPutbackChars - inDestBuffer; // yes, this number std::copy(origGptr - chunkSize, origGptr, _outBuf + _putbackSize - nbPutbackChars); nbPutbackCharsToGo -= chunkSize; } + // Finally, copy those that are not in _outBuf std::copy(writePtr - nbPutbackCharsToGo, writePtr, _outBuf + _putbackSize - nbPutbackCharsToGo); - setg(_outBuf + _putbackSize - nbPutbackChars, // start of putback area - _outBuf + _putbackSize, // the buffer for pending, - _outBuf + _putbackSize); // available data is empty - std::streamsize rem = static_cast(remaining); - // This is guaranteed because n is of type std::streamsize and the whole - // algorithm ensures that 0 <= remaining <= n. - assert(rem >= 0); - assert(static_cast(rem) == remaining); - assert(n - rem >= 0); - // Total number of chars copied. - return n - rem; + return nbPutbackChars; } // *************************************************************************** @@ -684,10 +711,12 @@ ZlibCompressorIStream::ZlibCompressorIStream(std::istream& iStream, char *outBuf, std::size_t outBufSize, std::size_t putbackSize) - : _streamBuf(iStream, path, compressionLevel, format, memStrategy, inBuf, + : std::istream(nullptr), + _streamBuf(iStream, path, compressionLevel, format, memStrategy, inBuf, inBufSize, outBuf, outBufSize, putbackSize) { - rdbuf(&_streamBuf); // Associate _streamBuf to 'this' + // Associate _streamBuf to 'this' and clear the error state flags + rdbuf(&_streamBuf); } ZlibCompressorIStream::~ZlibCompressorIStream() @@ -705,10 +734,12 @@ ZlibDecompressorIStream::ZlibDecompressorIStream(std::istream& iStream, char *outBuf, std::size_t outBufSize, std::size_t putbackSize) - : _streamBuf(iStream, path, format, inBuf, inBufSize, outBuf, outBufSize, + : std::istream(nullptr), + _streamBuf(iStream, path, format, inBuf, inBufSize, outBuf, outBufSize, putbackSize) { - rdbuf(&_streamBuf); // Associate _streamBuf to 'this' + // Associate _streamBuf to 'this' and clear the error state flags + rdbuf(&_streamBuf); } ZlibDecompressorIStream::~ZlibDecompressorIStream() diff --git a/simgear/io/iostreams/zlibstream.hxx b/simgear/io/iostreams/zlibstream.hxx index d268acc3..4babe165 100644 --- a/simgear/io/iostreams/zlibstream.hxx +++ b/simgear/io/iostreams/zlibstream.hxx @@ -180,6 +180,9 @@ private: // output buffer, data is written by zlib directly to the destination // buffer). std::streamsize xsgetn(char* dest, std::streamsize n) override; + // Utility method for xsgetn() + std::size_t xsgetn_preparePutbackArea(char* origGptr, char* dest, + char* writePtr); // Make sure there is data to read in the input buffer, or signal EOF. bool getInputData(); // Utility method for fillOutputBuffer() diff --git a/simgear/io/iostreams/zlibstream_test.cxx b/simgear/io/iostreams/zlibstream_test.cxx index dd3094f4..6d8f6999 100644 --- a/simgear/io/iostreams/zlibstream_test.cxx +++ b/simgear/io/iostreams/zlibstream_test.cxx @@ -24,23 +24,37 @@ #include #include #include // std::numeric_limits +#include // std::make_unsigned() #include // std::bind() #include #include // EXIT_SUCCESS #include // std::size_t #include // strcmp() -using std::string; -using std::cout; -using std::cerr; -using traits = std::char_traits; - #include #include #include #include #include +using std::string; +using std::cout; +using std::cerr; +using traits = std::char_traits; + +typedef typename std::make_unsigned::type uStreamSize; + +// Safely convert a non-negative std::streamsize into an std::size_t. If +// impossible, bail out. +static std::size_t streamsizeToSize_t(std::streamsize n) +{ + SG_CHECK_GE(n, 0); + SG_CHECK_LE(static_cast(n), + std::numeric_limits::max()); + + return static_cast(n); +} + // In many tests below, I use very small buffer sizes. Of course, this is bad // for performance. The reason I do it this way is simply because it better // exercises the code we want to *test* here (we are more likely to find bugs @@ -214,7 +228,9 @@ void test_StreambufBasicOperations() // Most efficient way (with the underlying xsgetn()) to read several chars // at once. std::streamsize n = decompSBuf.sgetn(buf, bufSize); - SG_VERIFY(n == bufSize && string(buf, bufSize) == "3456789abc"); + SG_CHECK_EQUAL(n, bufSize); + SG_CHECK_EQUAL(string(buf, static_cast(bufSize)), + "3456789abc"); ch = decompSBuf.sungetc(); // same as sputbackc(), except no value to check SG_VERIFY(ch != EOF && traits::to_char_type(ch) == 'c'); @@ -226,15 +242,17 @@ void test_StreambufBasicOperations() SG_VERIFY(ch != EOF && traits::to_char_type(ch) == 'b'); n = decompSBuf.sgetn(buf, bufSize); - SG_VERIFY(n == bufSize && string(buf, bufSize) == "bcdefghijk"); + SG_CHECK_EQUAL(n, bufSize); + SG_CHECK_EQUAL(string(buf, static_cast(bufSize)), + "bcdefghijk"); ch = decompSBuf.sungetc(); SG_VERIFY(ch != EOF && traits::to_char_type(ch) == 'k'); static char buf2[64]; n = decompSBuf.sgetn(buf2, sizeof(buf2)); - SG_VERIFY(n == 36 && string(buf2, n) == "klmnopqrstuvwxyz\nABCDEF\nGHIJK " - "LMNOPQ"); + SG_CHECK_EQUAL(n, 36); + SG_CHECK_EQUAL(string(buf2, 36), "klmnopqrstuvwxyz\nABCDEF\nGHIJK LMNOPQ"); ch = decompSBuf.sbumpc(); SG_CHECK_EQUAL(ch, EOF); @@ -404,7 +422,10 @@ void test_ZlibDecompressorIStream_readPutbackEtc() string rest(buf2, nbCharsRead); do { decompressor.read(buf2, sizeof(buf2)); - rest += string(buf2, decompressor.gcount()); + // The conversion to std::size_t is safe because decompressor.read() + // returns a non-negative value which, in this case, can't exceed + // sizeof(buf2). + rest += string(buf2, streamsizeToSize_t(decompressor.gcount())); } while (decompressor); SG_CHECK_EQUAL(rest, " LMNOPQ");