From 1b74db8f518896aa7ef25b7b652ae228e78c2778 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Fri, 6 Jul 2018 11:33:56 +0200 Subject: [PATCH 01/10] [types] Add DataBuf::free() DataBuf::release() easily cause memory leaks, when the return value is ignored. free() provides the desired behavior, when the internal data should just be deleted and not used further. --- include/exiv2/types.hpp | 6 ++++++ src/types.cpp | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/include/exiv2/types.hpp b/include/exiv2/types.hpp index 1e8d390c..d9886e6a 100644 --- a/include/exiv2/types.hpp +++ b/include/exiv2/types.hpp @@ -236,6 +236,12 @@ namespace Exiv2 { buffer. */ std::pair release(); + + /*! + @brief Free the internal buffer and reset the size to 0. + */ + void free(); + //! Reset value void reset(std::pair =std::make_pair((byte*)(0),long(0))); //@} diff --git a/src/types.cpp b/src/types.cpp index 33ae8879..80ef1efe 100644 --- a/src/types.cpp +++ b/src/types.cpp @@ -163,6 +163,13 @@ namespace Exiv2 { return p; } + void DataBuf::free() + { + delete[] pData_; + pData_ = 0; + size_ = 0; + } + void DataBuf::reset(std::pair p) { if (pData_ != p.first) { From 607b19111c04464d1c4f9bed32190cfaba3fb5f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Fri, 6 Jul 2018 11:35:08 +0200 Subject: [PATCH 02/10] [DataBuf] Replace wrong usage of release() with free() --- src/image.cpp | 2 +- src/pngimage.cpp | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/image.cpp b/src/image.cpp index bd6ef531..5277c58e 100644 --- a/src/image.cpp +++ b/src/image.cpp @@ -662,7 +662,7 @@ namespace Exiv2 { void Image::clearIccProfile() { - iccProfile_.release(); + iccProfile_.free(); } void Image::setByteOrder(ByteOrder byteOrder) diff --git a/src/pngimage.cpp b/src/pngimage.cpp index 17de5bf8..2e1c4d0c 100644 --- a/src/pngimage.cpp +++ b/src/pngimage.cpp @@ -99,13 +99,14 @@ namespace Exiv2 { zlibResult = uncompress((Bytef*)result.pData_,&uncompressedLen,bytes,length); // if result buffer is large than necessary, redo to fit perfectly. if (zlibResult == Z_OK && (long) uncompressedLen < result.size_ ) { - result.release(); + result.free(); + result.alloc(uncompressedLen); zlibResult = uncompress((Bytef*)result.pData_,&uncompressedLen,bytes,length); } if (zlibResult == Z_BUF_ERROR) { // the uncompressed buffer needs to be larger - result.release(); + result.free(); // Sanity - never bigger than 16mb if (uncompressedLen > 16*1024*1024) zlibResult = Z_DATA_ERROR; @@ -126,10 +127,10 @@ namespace Exiv2 { zlibResult = compress((Bytef*)result.pData_,&compressedLen,bytes,length); if (zlibResult == Z_BUF_ERROR) { // the compressedArray needs to be larger - result.release(); + result.free(); compressedLen *= 2; } else { - result.release(); + result.free(); result.alloc(compressedLen); zlibResult = compress((Bytef*)result.pData_,&compressedLen,bytes,length); } From 39399ac5e80c3f2ce2f12af3f9582d7d5f9c2f8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Fri, 6 Jul 2018 11:37:49 +0200 Subject: [PATCH 03/10] Remove memory leak in SshIo::SshImpl::getDataByRange The buffer array is not deleted, when an exception is thrown (happens for nBytes< 0). => use std::vector instead --- src/basicio.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/basicio.cpp b/src/basicio.cpp index 87751915..33818ca7 100644 --- a/src/basicio.cpp +++ b/src/basicio.cpp @@ -2460,11 +2460,12 @@ namespace Exiv2 { if (protocol_ == pSftp) { if (sftp_seek(fileHandler_, (uint32_t) (lowBlock * blockSize_)) < 0) throw Error(kerErrorMessage, "SFTP: unable to sftp_seek"); size_t buffSize = (highBlock - lowBlock + 1) * blockSize_; - char* buffer = new char[buffSize]; - long nBytes = (long) sftp_read(fileHandler_, buffer, buffSize); - if (nBytes < 0) throw Error(kerErrorMessage, "SFTP: unable to sftp_read"); - response.assign(buffer, buffSize); - delete[] buffer; + std::vector buffer(buffSize); + long nBytes = static_cast(sftp_read(fileHandler_, &buffer.at(0), buffSize)); + if (nBytes < 0) { + throw Error(kerErrorMessage, "SFTP: unable to sftp_read"); + } + response.assign(&buffer.at(0), buffSize); } else { std::stringstream ss; if (lowBlock > -1 && highBlock > -1) { From b2c3b61abcdb8e1a904e7c3f8b9f683c1b0b5668 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Fri, 6 Jul 2018 11:39:45 +0200 Subject: [PATCH 04/10] [IptcData::printStructure] Remove buffer overrun The loop condition will perform a range check correctly, but it will always dereference bytes[i], even if i is too large and fails the second check. => move the bytes[i] == 0x1c check into a if, after the range check was successfull --- src/iptc.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/iptc.cpp b/src/iptc.cpp index ec7a58a9..11cc17bd 100644 --- a/src/iptc.cpp +++ b/src/iptc.cpp @@ -354,7 +354,10 @@ namespace Exiv2 { while ( i < size-3 && bytes[i] != 0x1c ) i++; depth++; out << Internal::indent(depth) << "Record | DataSet | Name | Length | Data" << std::endl; - while ( bytes[i] == 0x1c && i < size-3 ) { + while ( i < size-3 ) { + if (bytes[i] != 0x1c) { + break; + } char buff[100]; uint16_t record = bytes[i+1]; uint16_t dataset = bytes[i+2]; From 67dc3e691fe53d4523cb2e16b617b6cdf64cf907 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Sun, 29 Jul 2018 00:00:52 +0200 Subject: [PATCH 05/10] [IptcData::printStructure] clang-format function --- src/iptc.cpp | 43 +++++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/src/iptc.cpp b/src/iptc.cpp index 11cc17bd..8436b6db 100644 --- a/src/iptc.cpp +++ b/src/iptc.cpp @@ -348,27 +348,30 @@ namespace Exiv2 { return iptcMetadata_.erase(pos); } - void IptcData::printStructure(std::ostream& out, const byte* bytes,const size_t size,uint32_t depth) - { - uint32_t i = 0 ; - while ( i < size-3 && bytes[i] != 0x1c ) i++; - depth++; - out << Internal::indent(depth) << "Record | DataSet | Name | Length | Data" << std::endl; - while ( i < size-3 ) { - if (bytes[i] != 0x1c) { - break; - } - char buff[100]; - uint16_t record = bytes[i+1]; - uint16_t dataset = bytes[i+2]; - uint16_t len = getUShort(bytes+i+3,bigEndian); - sprintf(buff," %6d | %7d | %-24s | %6d | ",record,dataset, Exiv2::IptcDataSets::dataSetName(dataset,record).c_str(), len); + void IptcData::printStructure(std::ostream& out, const byte* bytes, const size_t size, uint32_t depth) + { + uint32_t i = 0; + while (i < size - 3 && bytes[i] != 0x1c) + i++; + depth++; + out << Internal::indent(depth) << "Record | DataSet | Name | Length | Data" << std::endl; + while (i < size - 3) { + if (bytes[i] != 0x1c) { + break; + } + char buff[100]; + uint16_t record = bytes[i + 1]; + uint16_t dataset = bytes[i + 2]; + uint16_t len = getUShort(bytes + i + 3, bigEndian); + sprintf(buff, " %6d | %7d | %-24s | %6d | ", record, dataset, + Exiv2::IptcDataSets::dataSetName(dataset, record).c_str(), len); - out << buff << Internal::binaryToString(bytes,(len>40?40:len),i+5) << (len>40?"...":"") << std::endl; - i += 5 + len; - } - depth--; - } + out << buff << Internal::binaryToString(bytes, (len > 40 ? 40 : len), i + 5) << (len > 40 ? "..." : "") + << std::endl; + i += 5 + len; + } + depth--; + } const char *IptcData::detectCharset() const { From 8d5a3c7dd9e4117b5f5bc91fb4842beedfb19e90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Fri, 6 Jul 2018 11:51:55 +0200 Subject: [PATCH 06/10] Remove buffer overread in tExtToDataBuf The pointer p is advanced in the while loop to step over three '\n'. However, its length is never reduced accordingly. => the length check in the following for loop is invalid, as it permits overreading by the number of characters that p was advanced by. --- src/pngimage.cpp | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/pngimage.cpp b/src/pngimage.cpp index 2e1c4d0c..984f7532 100644 --- a/src/pngimage.cpp +++ b/src/pngimage.cpp @@ -155,12 +155,21 @@ namespace Exiv2 { } // calculate length and allocate result; + // count: number of \n in the header long count=0; + // p points to the current position in the array bytes const byte* p = bytes ; - // header is \nsomething\n number\n hex - while ( count < 3 ) - if ( *p++ == '\n' ) + + // header is '\nsomething\n number\n hex' + // => increment p until it points to the byte after the last \n + // p must stay within bounds of the bytes array! + while ((count < 3) && (p - bytes < length)) { + // length is later used for range checks of p => decrement it for each increment of p + --length; + if ( *p++ == '\n' ) { count++; + } + } for ( long i = 0 ; i < length ; i++ ) if ( value[p[i]] ) ++count; From dbf90b976fff05d66ebfb2e0e05157355a86fd1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Sat, 7 Jul 2018 09:27:59 +0200 Subject: [PATCH 07/10] Fix overread in memcmp in PngImage::doWriteMetadata() memcmp() compares the read data from key with the provided string, but when key.pData_ is shorter than the provided length, then memcmp can read beyond the bounds of key.pData_ => add custom compare function, which ensures that we never read more than key.size_ --- src/pngimage.cpp | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/pngimage.cpp b/src/pngimage.cpp index 984f7532..ffd04ce5 100644 --- a/src/pngimage.cpp +++ b/src/pngimage.cpp @@ -1,6 +1,6 @@ // ***************************************************************** -*- C++ -*- /* - * Copyright (C) 2004-2017 Andreas Huggel + * Copyright (C) 2004-2018 Exiv2 authors * * This program is part of the Exiv2 distribution. * @@ -57,6 +57,16 @@ const unsigned char pngBlank[] = { 0x89,0x50,0x4e,0x47,0x0d,0x0a,0x1a,0x0a,0x00, 0x45,0x4e,0x44,0xae,0x42,0x60,0x82 }; +namespace +{ + inline bool compare(const char* str, const Exiv2::DataBuf& buf, size_t length) + { + // str & length should compile time constants => only running this in DEBUG mode is ok + assert(strlen(str) <= length); + return memcmp(str, buf.pData_, std::min(static_cast(length), buf.size_)) == 0; + } +} // namespace + // ***************************************************************************** // class member definitions namespace Exiv2 { @@ -688,14 +698,14 @@ namespace Exiv2 { !memcmp(cheaderBuf.pData_ + 4, "iCCP", 4)) { DataBuf key = PngChunk::keyTXTChunk(chunkBuf, true); - if (memcmp("Raw profile type exif", key.pData_, 21) == 0 || - memcmp("Raw profile type APP1", key.pData_, 21) == 0 || - memcmp("Raw profile type iptc", key.pData_, 21) == 0 || - memcmp("Raw profile type xmp", key.pData_, 20) == 0 || - memcmp("XML:com.adobe.xmp", key.pData_, 17) == 0 || - memcmp("icc", key.pData_, 3) == 0 || // see test/data/imagemagick.png - memcmp("ICC", key.pData_, 3) == 0 || - memcmp("Description", key.pData_, 11) == 0) + if (compare("Raw profile type exif", key, 21) || + compare("Raw profile type APP1", key, 21) || + compare("Raw profile type iptc", key, 21) || + compare("Raw profile type xmp", key, 20) || + compare("XML:com.adobe.xmp", key, 17) || + compare("icc", key, 3) || // see test/data/imagemagick.png + compare("ICC", key, 3) || + compare("Description", key, 11)) { #ifdef DEBUG std::cout << "Exiv2::PngImage::doWriteMetadata: strip " << szChunk From 1ab921cb83bc7f909cc98f47d113ea9fe1e65fe9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Sat, 7 Jul 2018 10:47:07 +0200 Subject: [PATCH 08/10] Add two padding bytes to exifLongHeader to prevent overreads in the following call: getHeaderOffset (payload.pData_, payload.size_, (byte*)&exifLongHeader, 6); getHeaderOffset would read 6 bytes from exifLongHeader, reading beyond the bounds of the array => add 2 padding bytes to prevent overreads --- src/webpimage.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/webpimage.cpp b/src/webpimage.cpp index ebb7599d..ae165c46 100644 --- a/src/webpimage.cpp +++ b/src/webpimage.cpp @@ -594,7 +594,8 @@ namespace Exiv2 { io_->read(payload.pData_, payload.size_); byte size_buff[2]; - byte exifLongHeader[] = { 0xFF, 0x01, 0xFF, 0xE1 }; + // 4 meaningful bytes + 2 padding bytes + byte exifLongHeader[] = { 0xFF, 0x01, 0xFF, 0xE1, 0x00, 0x00 }; byte exifShortHeader[] = { 0x45, 0x78, 0x69, 0x66, 0x00, 0x00 }; byte exifTiffLEHeader[] = { 0x49, 0x49, 0x2A }; // "MM*" byte exifTiffBEHeader[] = { 0x4D, 0x4D, 0x00, 0x2A }; // "II\0*" From b12a868454d4ce503c2c0f17df291811aac709f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Sat, 7 Jul 2018 10:50:11 +0200 Subject: [PATCH 09/10] Add EXV_WARN_UNUSED_RESULT macro & add it to DataBuf::release() EXV_WARN_UNUSED_RESULT is a conditional macro that expands to either __attribute__((warn_unused_result)) on gcc & clang or to _Check_return for MSVC => Compiler warns if the return value is ignored --- include/exiv2/types.hpp | 10 +++++++++- src/types.cpp | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/include/exiv2/types.hpp b/include/exiv2/types.hpp index d9886e6a..21f58a0e 100644 --- a/include/exiv2/types.hpp +++ b/include/exiv2/types.hpp @@ -85,6 +85,14 @@ //! Simple common max macro #define EXV_MAX(a,b) ((a) > (b) ? (a) : (b)) +#if defined(__GNUC__) && (__GNUC__ >= 4) || defined(__clang__) +#define EXV_WARN_UNUSED_RESULT __attribute__ ((warn_unused_result)) +#elif defined(_MSC_VER) && (_MSC_VER >= 1700) +#define EXV_WARN_UNUSED_RESULT _Check_return_ +#else +#define EXV_WARN_UNUSED_RESULT +#endif + // ***************************************************************************** // forward declarations struct tm; @@ -235,7 +243,7 @@ namespace Exiv2 { buffer as a data pointer and size pair, resets the internal buffer. */ - std::pair release(); + EXV_WARN_UNUSED_RESULT std::pair release(); /*! @brief Free the internal buffer and reset the size to 0. diff --git a/src/types.cpp b/src/types.cpp index 80ef1efe..867cfc03 100644 --- a/src/types.cpp +++ b/src/types.cpp @@ -155,7 +155,7 @@ namespace Exiv2 { } } - std::pair DataBuf::release() + EXV_WARN_UNUSED_RESULT std::pair DataBuf::release() { std::pair p = std::make_pair(pData_, size_); pData_ = 0; From 664e93c0572e6533e922ed92c85a32bd5289f884 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Mon, 9 Jul 2018 05:47:18 +0200 Subject: [PATCH 10/10] [travis] Enable ASAN for the test suite --- ci/run.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ci/run.sh b/ci/run.sh index 941e05b4..12e3094b 100755 --- a/ci/run.sh +++ b/ci/run.sh @@ -15,7 +15,11 @@ fi mkdir build && cd build conan install .. --build missing --profile release -cmake ${CMAKE_OPTIONS} -DCMAKE_INSTALL_PREFIX=install .. +cmake ${CMAKE_OPTIONS} -DCMAKE_INSTALL_PREFIX=install \ + -DCMAKE_CXX_FLAGS="-fsanitize=address" \ + -DCMAKE_C_FLAGS="-fsanitize=address" \ + -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address" \ + -DCMAKE_MODULE_LINKER_FLAGS="-fsanitize=address" .. make -j2 VERBOSE=1 make tests make install