Merge pull request Exiv2#380 from D4N/misc_issues_fix

Fix various memory issues and enable ASAN for the test suite

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

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,13 @@ namespace Exiv2 {
                  buffer as a data pointer and size pair, resets the internal
                  buffer.
          */
-        std::pair<byte*, long> release();
+        EXV_WARN_UNUSED_RESULT std::pair<byte*, long> release();
+
+         /*!
+           @brief Free the internal buffer and reset the size to 0.
+          */
+        void free();
+
         //! Reset value
         void reset(std::pair<byte*, long> =std::make_pair((byte*)(0),long(0)));
         //@}

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<char> buffer(buffSize);
+            long nBytes = static_cast<long>(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) {

src/image.cpp

@@ -662,7 +662,7 @@ namespace Exiv2 {
 
     void Image::clearIccProfile()
     {
-        iccProfile_.release();
+        iccProfile_.free();
     }
 
     void Image::setByteOrder(ByteOrder byteOrder)

src/iptc.cpp

@@ -348,24 +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 ( bytes[i] == 0x1c && i < size-3 ) {
-			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--;
-	}
+    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--;
+    }
 
     const char *IptcData::detectCharset() const
     {

src/pngimage.cpp

@@ -1,6 +1,6 @@
 // ***************************************************************** -*- C++ -*-
 /*
- * Copyright (C) 2004-2017 Andreas Huggel <ahuggel@gmx.net>
+ * 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<long>(length), buf.size_)) == 0;
+    }
+}  // namespace
+
 // *****************************************************************************
 // class member definitions
 namespace Exiv2 {
@@ -99,13 +109,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 +137,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);
             }
@@ -154,12 +165,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;
@@ -678,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

src/types.cpp

@@ -155,14 +155,21 @@ namespace Exiv2 {
         }
     }
 
-    std::pair<byte*, long> DataBuf::release()
+    EXV_WARN_UNUSED_RESULT std::pair<byte*, long> DataBuf::release()
     {
         std::pair<byte*, long> p = std::make_pair(pData_, size_);
         pData_ = 0;
         size_ = 0;
         return p;
     }
 
+    void DataBuf::free()
+    {
+        delete[] pData_;
+        pData_ = 0;
+        size_ = 0;
+    }
+
     void DataBuf::reset(std::pair<byte*, long> p)
     {
         if (pData_ != p.first) {

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*"