Merge pull request #328 from Exiv2/memory_fixes

Memory issue fixes

src/CMakeLists.txt

@@ -17,6 +17,7 @@ set( LIBEXIV2_SRC
     exif.cpp
     futils.cpp
     gifimage.cpp
+    helper_functions.cpp
     http.cpp
     image.cpp
     ini.cpp

src/helper_functions.cpp

@@ -0,0 +1,41 @@
+// ********************************************************* -*- C++ -*-
+/*
+ * Copyright (C) 2004-2018 Exiv2 authors
+ *
+ * This program is part of the Exiv2 distribution.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, 5th Floor, Boston, MA 02110-1301 USA.
+ */
+/*!
+  @file    helper_functions.cpp
+  @brief   A collection of helper functions
+  @author  Dan Čermák (D4N)
+           <a href="mailto:dan.cermak@cgc-instruments.com">dan.cermak@cgc-instruments.com</a>
+  @date    25-May-18, D4N: created
+ */
+
+#include "helper_functions.hpp"
+
+#include <algorithm>
+
+
+std::string string_from_unterminated(const char* data, size_t data_length)
+{
+    const char* termination = std::find(data, data + data_length, 0);
+    // if find returned the end iterator => no \0 found
+    const size_t string_length = termination == data + data_length ? data_length : termination - data;
+
+    return std::string(data, string_length);
+}

src/helper_functions.hpp

@@ -0,0 +1,43 @@
+// ********************************************************* -*- C++ -*-
+/*
+ * Copyright (C) 2004-2018 Exiv2 authors
+ *
+ * This program is part of the Exiv2 distribution.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, 5th Floor, Boston, MA 02110-1301 USA.
+ */
+/*!
+  @file    helper_functions.hpp
+  @brief   A collection of helper functions
+  @author  Dan Čermák (D4N)
+           <a href="mailto:dan.cermak@cgc-instruments.com">dan.cermak@cgc-instruments.com</a>
+  @date    25-May-18, D4N: created
+ */
+#ifndef HELPER_FUNCTIONS_HPP
+#define HELPER_FUNCTIONS_HPP
+
+#include <string>
+
+/*!
+  @brief Convert a (potentially not null terminated) array into a
+  std::string.
+
+  Convert a C style string that may or may not be null terminated safely
+  into a std::string. The string's termination is either set at the first \0
+  or after data_length characters.
+ */
+std::string string_from_unterminated(const char* data, size_t data_length);
+
+#endif  // HELPER_FUNCTIONS_HPP

src/jpgimage.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.
  *
@@ -30,6 +30,7 @@
 #include "image_int.hpp"
 #include "error.hpp"
 #include "futils.hpp"
+#include "helper_functions.hpp"
 #include "enforce.hpp"
 
 #ifdef WIN32
@@ -46,6 +47,7 @@
 #include <cstdio>                               // for EOF
 #include <cstring>
 #include <cassert>
+#include <stdexcept>
 
 // *****************************************************************************
 // class member definitions
@@ -643,7 +645,8 @@ namespace Exiv2 {
                 // print signature for APPn
                 if (marker >= app0_ && marker <= (app0_ | 0x0F)) {
                     // http://www.adobe.com/content/dam/Adobe/en/devnet/xmp/pdfs/XMPSpecificationPart3.pdf p75
-                    const char* signature = (const char*)buf.pData_ + 2;
+                    const std::string signature =
+                        string_from_unterminated(reinterpret_cast<const char*>(buf.pData_ + 2), buf.size_ - 2);
 
                     // 728 rmills@rmillsmbp:~/gnu/exiv2/ttt $ exiv2 -pS test/data/exiv2-bug922.jpg
                     // STRUCTURE OF JPEG FILE: test/data/exiv2-bug922.jpg
@@ -652,12 +655,12 @@ namespace Exiv2 {
                     //       2 | 0xe1 APP1  |     911 | Exif..MM.*.......%.........#....
                     //     915 | 0xe1 APP1  |     870 | http://ns.adobe.com/xap/1.0/.<x:
                     //    1787 | 0xe1 APP1  |   65460 | http://ns.adobe.com/xmp/extensio
-                    if (option == kpsXMP && std::string(signature).find("http://ns.adobe.com/x") == 0) {
+                    if (option == kpsXMP && signature.find("http://ns.adobe.com/x") == 0) {
                         // extract XMP
                         if (size > 0) {
                             io_->seek(-bufRead, BasicIo::cur);
-                            byte* xmp = new byte[size + 1];
+                            std::vector<byte> xmp(size + 1);
-                            io_->read(xmp, size);
+                            io_->read(xmp.data(), size);
                             int start = 0;
 
                             // http://wwwimages.adobe.com/content/dam/Adobe/en/devnet/xmp/pdfs/XMPSpecificationPart3.pdf
@@ -668,10 +671,13 @@ namespace Exiv2 {
                             // and dumping the XMP in a post read operation similar to kpsIptcErase
                             // for the moment, dumping 'on the fly' is working fine
                             if (!bExtXMP) {
-                                while (xmp[start])
+                                while (xmp.at(start)) {
                                     start++;
+                                }
                                 start++;
-                                if (::strstr((char*)xmp + start, "HasExtendedXMP")) {
+                                const std::string xmp_from_start = string_from_unterminated(
+                                    reinterpret_cast<const char*>(&xmp.at(start)), size - start);
+                                if (xmp_from_start.find("HasExtendedXMP", start) != xmp_from_start.npos) {
                                     start = size;  // ignore this packet, we'll get on the next time around
                                     bExtXMP = true;
                                 }
@@ -679,25 +685,24 @@ namespace Exiv2 {
                                 start = 2 + 35 + 32 + 4 + 4;  // Adobe Spec, p19
                             }
 
-                            out.write((const char*)(xmp + start), size - start);
+                            out.write(reinterpret_cast<const char*>(&xmp.at(start)), size - start);
-                            delete[] xmp;
                             bufRead = size;
                             done = !bExtXMP;
                         }
-                    } else if (option == kpsIccProfile && std::strcmp(signature, iccId_) == 0) {
+                    } else if (option == kpsIccProfile && signature.compare(iccId_) == 0) {
                         // extract ICCProfile
                         if (size > 0) {
                             io_->seek(-bufRead, BasicIo::cur);  // back to buffer (after marker)
                             io_->seek(14 + 2, BasicIo::cur);    // step over header
                             DataBuf icc(size - (14 + 2));
                             io_->read(icc.pData_, icc.size_);
-                            out.write((const char*)icc.pData_, icc.size_);
+                            out.write(reinterpret_cast<const char*>(icc.pData_), icc.size_);
 #ifdef DEBUG
                             std::cout << "iccProfile size = " << icc.size_ << std::endl;
 #endif
                             bufRead = size;
                         }
-                    } else if (option == kpsIptcErase && std::strcmp(signature, "Photoshop 3.0") == 0) {
+                    } else if (option == kpsIptcErase && signature.compare("Photoshop 3.0") == 0) {
                         // delete IPTC data segment from JPEG
                         if (size > 0) {
                             io_->seek(-bufRead, BasicIo::cur);
@@ -706,19 +711,29 @@ namespace Exiv2 {
                         }
                     } else if (bPrint) {
                         out << "| " << Internal::binaryToString(buf, size > 32 ? 32 : size, size > 0 ? 2 : 0);
-                        if (std::strcmp(signature, iccId_) == 0) {
+                        if (signature.compare(iccId_) == 0) {
-                            int chunk = (int)signature[12];
+                            // extract the chunk information from the buffer
-                            int chunks = (int)signature[13];
+                            //
+                            // the buffer looks like this in this branch
+                            // ICC_PROFILE\0AB
+                            // where A & B are bytes (the variables chunk & chunks)
+                            //
+                            // We cannot extract the variables A and B from the signature string, as they are beyond the
+                            // null termination (and signature ends there).
+                            // => Read the chunk info from the DataBuf directly
+                            enforce<std::out_of_range>(buf.size_ - 2 > 14, "Buffer too small to extract chunk information.");
+                            const int chunk = buf.pData_[2 + 12];
+                            const int chunks = buf.pData_[2 + 13];
                             out << Internal::stringFormat(" chunk %d/%d", chunk, chunks);
                         }
                     }
 
                     // for MPF: http://www.sno.phy.queensu.ca/~phil/exiftool/TagNames/MPF.html
                     // for FLIR: http://owl.phy.queensu.ca/~phil/exiftool/TagNames/FLIR.html
-                    bool bFlir = option == kpsRecursive && marker == (app0_ + 1) && std::strcmp(signature, "FLIR") == 0;
+                    bool bFlir = option == kpsRecursive && marker == (app0_ + 1) && signature.compare("FLIR") == 0;
-                    bool bExif = option == kpsRecursive && marker == (app0_ + 1) && std::strcmp(signature, "Exif") == 0;
+                    bool bExif = option == kpsRecursive && marker == (app0_ + 1) && signature.compare("Exif") == 0;
-                    bool bMPF = option == kpsRecursive && marker == (app0_ + 2) && std::strcmp(signature, "MPF") == 0;
+                    bool bMPF = option == kpsRecursive && marker == (app0_ + 2) && signature.compare("MPF") == 0;
-                    bool bPS = option == kpsRecursive && std::strcmp(signature, "Photoshop 3.0") == 0;
+                    bool bPS = option == kpsRecursive && signature.compare("Photoshop 3.0") == 0;
                     if (bFlir || bExif || bMPF || bPS) {
                         // extract Exif data block which is tiff formatted
                         if (size > 0) {
@@ -731,7 +746,7 @@ namespace Exiv2 {
                             // copy the data to memory
                             io_->seek(-bufRead, BasicIo::cur);
                             io_->read(exif, size);
-                            uint32_t start = std::strcmp(signature, "Exif") == 0 ? 8 : 6;
+                            uint32_t start = signature.compare("Exif") == 0 ? 8 : 6;
                             uint32_t max = (uint32_t)size - 1;
 
                             // is this an fff block?

unitTests/CMakeLists.txt

@@ -8,6 +8,7 @@ add_executable(unit_tests mainTestRunner.cpp
     test_XmpKey.cpp
     test_DateValue.cpp
     test_cr2header_int.cpp
+    test_helper_functions.cpp
 )
 
 #TODO Use GTest::GTest once we upgrade the minimum CMake version required

unitTests/test_XmpKey.cpp

@@ -24,6 +24,11 @@ public:
         XmpProperties::registerNs(expectedFamily, expectedPrefix);
     }
 
+    static void TearDownTestCase()
+    {
+        XmpProperties::unregisterNs();
+    }
+
     void checkValidity(const XmpKey& key)
     {
         ASSERT_EQ(expectedKey, key.key());

unitTests/test_helper_functions.cpp

@@ -0,0 +1,21 @@
+#include "helper_functions.hpp"
+
+#include "gtestwrapper.h"
+
+TEST(string_from_unterminated, terminatedArray)
+{
+    const char data[5] = {'a', 'b', 'c', 0, 'd'};
+    const std::string res = string_from_unterminated(data, 5);
+
+    ASSERT_EQ(res.size(), 3);
+    ASSERT_STREQ(res.c_str(), "abc");
+}
+
+TEST(string_from_unterminated, unterminatedArray)
+{
+    const char data[4] = {'a', 'b', 'c', 'd'};
+    const std::string res = string_from_unterminated(data, 4);
+
+    ASSERT_EQ(res.size(), 4);
+    ASSERT_STREQ(res.c_str(), "abcd");
+}