Merge pull request #1786 from Exiv2/mergify/bp/main/pr-1769

Safer std::vector indexing (backport #1769)
Exiv2 · Jul 25, 2021 · 3575a82 · 3575a82
2 parents 2944af7 + 637a765
commit 3575a82
Show file tree

Hide file tree

Showing 19 changed files with 84 additions and 34 deletions.
diff --git a/samples/addmoddel.cpp b/samples/addmoddel.cpp
@@ -101,7 +101,7 @@ try {
         throw Exiv2::Error(Exiv2::kerErrorMessage, "Downcast failed");
     rv = Exiv2::URationalValue::UniquePtr(prv);
     // Modify the value directly through the interface of URationalValue
-    rv->value_[2] = std::make_pair(88,77);
+    rv->value_.at(2) = std::make_pair(88,77);
     // Copy the modified value back to the metadatum
     pos->setValue(rv.get());
     std::cout << "Modified key \"" << key

diff --git a/samples/exiv2json.cpp b/samples/exiv2json.cpp
@@ -74,7 +74,7 @@ bool getToken(std::string& in, Token& token, std::set<std::string>* pNS = nullpt
 
     while ( !result && in.length() ) {
         std::string c = in.substr(0,1);
-        char        C = c[0];
+        char        C = c.at(0);
         in            = in.substr(1,std::string::npos);
         if ( in.length() == 0 && C != ']' ) token.n += c;
         if ( C == '/' || C == '[' || C == ':' || C == '.' || C == ']' || in.length() == 0 ) {
@@ -115,7 +115,7 @@ Jzon::Node& addToTree(Jzon::Node& r1, const Token& token)
 
 Jzon::Node& recursivelyBuildTree(Jzon::Node& root,Tokens& tokens,size_t k)
 {
-    return addToTree( k==0 ? root : recursivelyBuildTree(root,tokens,k-1), tokens[k] );
+    return addToTree( k==0 ? root : recursivelyBuildTree(root,tokens,k-1), tokens.at(k) );
 }
 
 // build the json tree for this key.  return location and discover the name
@@ -128,7 +128,7 @@ Jzon::Node& objectForKey(const std::string& Key, Jzon::Object& root, std::string
     std::string input  = Key ; // Example: "XMP.xmp.MP.RegionInfo/MPRI:Regions[1]/MPReg:Rectangle"
     while ( getToken(input,token,pNS) ) tokens.push_back(token);
     size_t      l      = tokens.size()-1; // leave leaf name to push()
-    name               = tokens[l].n ;
+    name               = tokens.at(l).n ;
 
     // The second token.  For example: XMP.dc is a namespace
     if ( pNS && tokens.size() > 1 ) pNS->insert(tokens[1].n);

diff --git a/src/actions.cpp b/src/actions.cpp
@@ -994,19 +994,22 @@ namespace Action {
         Exiv2::PreviewPropertiesList pvList = pvMgr.getPreviewProperties();
 
         const Params::PreviewNumbers& numbers = Params::instance().previewNumbers_;
-        for (auto&& number : numbers) {
-            if (number == 0) {
+        for (auto number : numbers) {
+            size_t num = static_cast<size_t>(number);
+            if (num == 0) {
                 // Write all previews
-                for (int num = 0; num < static_cast<int>(pvList.size()); ++num) {
-                    writePreviewFile(pvMgr.getPreviewImage(pvList[num]), num + 1);
+                for (num = 0; num < pvList.size(); ++num) {
+                    writePreviewFile(pvMgr.getPreviewImage(pvList[num]), static_cast<int>(num + 1));
                 }
                 break;
             }
-            if (number > static_cast<int>(pvList.size())) {
-                std::cerr << path_ << ": " << _("Image does not have preview") << " " << number << "\n";
+            num--;
+            if (num >= pvList.size()) {
+                std::cerr << path_ << ": " << _("Image does not have preview")
+                          << " " << num + 1 << "\n";
                 continue;
             }
-            writePreviewFile(pvMgr.getPreviewImage(pvList[number - 1]), number);
+            writePreviewFile(pvMgr.getPreviewImage(pvList[num]), static_cast<int>(num + 1));
         }
         return 0;
     } // Extract::writePreviews
@@ -2070,7 +2073,7 @@ namespace {
                               << "' " << _("exists. [O]verwrite, [r]ename or [s]kip?")
                               << " ";
                     std::cin >> s;
-                    switch (s[0]) {
+                    switch (s.at(0)) {
                     case 'o':
                     case 'O':
                         go = false;
@@ -2135,7 +2138,7 @@ namespace {
                       << ": " << _("Overwrite") << " `" << path << "'? ";
             std::string s;
             std::cin >> s;
-            if (s[0] != 'y' && s[0] != 'Y') return 1;
+            if (s.at(0) != 'y' && s.at(0) != 'Y') return 1;
         }
         return 0;
     }

diff --git a/src/basicio.cpp b/src/basicio.cpp
@@ -184,11 +184,11 @@ namespace Exiv2 {
         case opRead:
             // Flush if current mode allows reading, else reopen (in mode "r+b"
             // as in this case we know that we can write to the file)
-            if (openMode_[0] == 'r' || openMode_[1] == '+') reopen = false;
+            if (openMode_.at(0) == 'r' || openMode_.at(1) == '+') reopen = false;
             break;
         case opWrite:
             // Flush if current mode allows writing, else reopen
-            if (openMode_[0] != 'r' || openMode_[1] == '+') reopen = false;
+            if (openMode_.at(0) != 'r' || openMode_.at(1) == '+') reopen = false;
             break;
         case opSeek:
             reopen = false;
@@ -929,7 +929,7 @@ namespace Exiv2 {
     size_t FileIo::size() const
     {
         // Flush and commit only if the file is open for writing
-        if (p_->fp_ != nullptr && (p_->openMode_[0] != 'r' || p_->openMode_[1] == '+')) {
+        if (p_->fp_ != nullptr && (p_->openMode_.at(0) != 'r' || p_->openMode_.at(1) == '+')) {
             std::fflush(p_->fp_);
 #if defined WIN32 && !defined __CYGWIN__
             // This is required on msvcrt before stat after writing to a file

diff --git a/src/exiv2.cpp b/src/exiv2.cpp
@@ -1478,8 +1478,8 @@ namespace {
             if (valStart != std::string::npos) {
                 value = parseEscapes(line.substr(valStart, valEnd+1-valStart));
                 std::string::size_type last = value.length()-1;
-                if (   (value[0] == '"' && value[last] == '"')
-                       || (value[0] == '\'' && value[last] == '\'')) {
+                if (   (value.at(0) == '"' && value.at(last) == '"')
+                       || (value.at(0) == '\'' && value.at(last) == '\'')) {
                     value = value.substr(1, value.length()-2);
                 }
             }

diff --git a/src/minoltamn_int.cpp b/src/minoltamn_int.cpp
@@ -2030,7 +2030,7 @@ namespace Exiv2 {
     {
         const TagDetails* td = find(minoltaSonyLensID, lensID);
         std::vector<std::string> tokens = split(td[0].label_,"|");
-        return os << exvGettext(trim(tokens[index-1]).c_str());
+        return os << exvGettext(trim(tokens.at(index-1)).c_str());
     }
 
     static std::ostream& resolveLens0x1c(std::ostream& os, const Value& value,

diff --git a/src/properties.cpp b/src/properties.cpp
@@ -2601,7 +2601,7 @@ namespace Exiv2 {
         // If property is a path for a nested property, determines the innermost element
         std::string::size_type i = property.find_last_of('/');
         if (i != std::string::npos) {
-            for (; i != std::string::npos && !isalpha(property[i]); ++i) {}
+            for (; i != std::string::npos && !isalpha(property.at(i)); ++i) {}
             property = property.substr(i);
             i = property.find_first_of(':');
             if (i != std::string::npos) {

diff --git a/src/sigmamn_int.cpp b/src/sigmamn_int.cpp
@@ -126,7 +126,7 @@ namespace Exiv2 {
         std::string v = value.toString();
         std::string::size_type pos = v.find(':');
         if (pos != std::string::npos) {
-            if (v[pos + 1] == ' ') ++pos;
+            if (v.at(pos + 1) == ' ') ++pos;
             v = v.substr(pos + 1);
         }
         return os << v;
@@ -136,7 +136,7 @@ namespace Exiv2 {
                                               const Value& value,
                                               const ExifData*)
     {
-        switch (value.toString()[0]) {
+        switch (value.toString().at(0)) {
         case 'P': os << _("Program"); break;
         case 'A': os << _("Aperture priority"); break;
         case 'S': os << _("Shutter priority"); break;
@@ -150,7 +150,7 @@ namespace Exiv2 {
                                               const Value& value,
                                               const ExifData*)
     {
-        switch (value.toString()[0]) {
+        switch (value.toString().at(0)) {
         case 'A': os << _("Average"); break;
         case 'C': os << _("Center"); break;
         case '8': os << _("8-Segment"); break;

diff --git a/src/tags_int.cpp b/src/tags_int.cpp
@@ -3248,7 +3248,7 @@ namespace Exiv2 {
         }
 
         std::string stringValue = value.toString();
-        if (stringValue[19] == 'Z') {
+        if (stringValue.at(19) == 'Z') {
             stringValue = stringValue.substr(0, 19);
         }
         std::replace(stringValue.begin(), stringValue.end(), 'T', ' ');

diff --git a/src/tiffvisitor_int.cpp b/src/tiffvisitor_int.cpp
@@ -461,7 +461,7 @@ namespace Exiv2 {
             uint.push_back(static_cast<uint16_t>(object->pValue()->toLong(i)));
         }
         // Check this is AFInfo2 (ints[0] = bytes in object)
-        if ( ints[0] != object->pValue()->count()*2 ) return ;
+        if ( ints.at(0) != object->pValue()->count()*2 ) return ;
 
         std::string familyGroup(std::string("Exif.") + groupName(object->group()) + ".");
 

diff --git a/src/types.cpp b/src/types.cpp
@@ -586,7 +586,7 @@ namespace Exiv2 {
     bool stringTo<bool>(const std::string& s, bool& ok)
     {
         std::string lcs(s); /* lowercase string */
-        for(unsigned i = 0; i < lcs.length(); i++) {
+        for(size_t i = 0; i < lcs.length(); i++) {
             lcs[i] = std::tolower(s[i]);
         }
         /* handle the same values as xmp sdk */

diff --git a/src/utils.cpp b/src/utils.cpp
@@ -61,7 +61,7 @@ namespace Util {
         if (p.length() == 2 && p[1] == ':') return p; // For Windows paths
         std::string::size_type idx = p.find_last_of("\\/");
         if (idx == std::string::npos) return ".";
-        if (idx == 1 && p[0] == '\\' && p[1] == '\\') return p; // For Windows paths
+        if (idx == 1 && p.at(0) == '\\' && p.at(1) == '\\') return p; // For Windows paths
         p = p.substr(0, idx == 0 ? 1 : idx);
         while (   p.length() > 1
                && (p[p.length()-1] == '\\' || p[p.length()-1] == '/')) {
@@ -82,7 +82,7 @@ namespace Util {
         }
         if (p.length() == 2 && p[1] == ':') return ""; // For Windows paths
         std::string::size_type idx = p.find_last_of("\\/");
-        if (idx == 1 && p[0] == '\\' && p[1] == '\\') return ""; // For Windows paths
+        if (idx == 1 && p.at(0) == '\\' && p.at(1) == '\\') return ""; // For Windows paths
         if (idx != std::string::npos) p = p.substr(idx+1);
         if (delsuffix) p = p.substr(0, p.length() - suffix(p).length());
         return p;

diff --git a/src/value.cpp b/src/value.cpp
@@ -425,8 +425,10 @@ namespace Exiv2 {
             std::string::size_type pos = comment.find_first_of(' ');
             std::string name = comment.substr(8, pos-8);
             // Strip quotes (so you can also specify the charset without quotes)
-            if (name[0] == '"') name = name.substr(1);
-            if (name[name.length()-1] == '"') name = name.substr(0, name.length()-1);
+            if (!name.empty()) {
+                if (name[0] == '"') name = name.substr(1);
+                if (name[name.length()-1] == '"') name = name.substr(0, name.length()-1);
+            }
             charsetId = CharsetInfo::charsetIdByName(name);
             if (charsetId == invalidCharsetId) {
 #ifndef SUPPRESS_WARNINGS
@@ -622,6 +624,9 @@ namespace Exiv2 {
         if (buf.length() > 5 && buf.substr(0, 5) == "type=") {
             std::string::size_type pos = buf.find_first_of(' ');
             type = buf.substr(5, pos-5);
+            if (type.empty()) {
+                throw Error(kerInvalidXmpText, type);
+            }
             // Strip quotes (so you can also specify the type without quotes)
             if (type[0] == '"') type = type.substr(1);
             if (type[type.length()-1] == '"') type = type.substr(0, type.length()-1);
@@ -785,6 +790,7 @@ namespace Exiv2 {
 
             std::string::size_type pos = buf.find_first_of(' ');
             lang = buf.substr(5, pos-5);
+            if (lang.empty()) throw Error(kerInvalidLangAltValue, buf);
             // Strip quotes (so you can also specify the language without quotes)
             if (lang[0] == '"') {
                 lang = lang.substr(1);
@@ -801,7 +807,7 @@ namespace Exiv2 {
             // Check language is in the correct format (see https://www.ietf.org/rfc/rfc3066.txt)
             std::string::size_type charPos = lang.find_first_not_of(ALPHA);
             if (charPos != std::string::npos) {
-                if (lang[charPos] != '-' || lang.find_first_not_of(ALPHA_NUM, charPos+1) != std::string::npos)
+                if (lang.at(charPos) != '-' || lang.find_first_not_of(ALPHA_NUM, charPos+1) != std::string::npos)
                     throw Error(kerInvalidLangAltValue, buf);
             }
 

diff --git a/src/xmp.cpp b/src/xmp.cpp
@@ -499,8 +499,8 @@ namespace Exiv2 {
         bool bNS  = out.find(':') != std::string::npos && !bURI;
 
         // pop trailing ':' on a namespace
-        if ( bNS ) {
-        std::size_t length = out.length();
+        if ( bNS && !out.empty() ) {
+            std::size_t length = out.length();
             if ( out[length-1] == ':' ) out = out.substr(0,length-1);
         }
 

diff --git a/src/xmpsidecar.cpp b/src/xmpsidecar.cpp
@@ -229,7 +229,7 @@ namespace Exiv2 {
         std::string head(reinterpret_cast<const char*>(buf + start), len - start);
         if (head.substr(0, 5)  == "<?xml") {
             // Forward to the next tag
-            for (unsigned i = 5; i < head.size(); ++i) {
+            for (size_t i = 5; i < head.size(); ++i) {
                 if (head[i] == '<') {
                     head = head.substr(i);
                     break;

diff --git a/test/data/coverage_xmpsidecar_isXmpType.xmp b/test/data/coverage_xmpsidecar_isXmpType.xmp
@@ -0,0 +1,3 @@
+<?xml                       <?xpacket
+aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+
diff --git a/test/data/issue_ghsa_v5g7_46xf_h728_poc.exv b/test/data/issue_ghsa_v5g7_46xf_h728_poc.exv
diff --git a/tests/bugfixes/github/test_coverage_xmpsidecar_isXmpType.py b/tests/bugfixes/github/test_coverage_xmpsidecar_isXmpType.py
@@ -0,0 +1,20 @@
+# -*- coding: utf-8 -*-
+
+from system_tests import CaseMeta, path, check_no_ASAN_UBSAN_errors
+
+class coverage_xmpsidecar_isXmpType(metaclass=CaseMeta):
+    """
+    Test added to improve code coverage in xmpsidecar.cpp after
+    Codecov complained about a lack of code coverage in this PR:
+    https://github.com/Exiv2/exiv2/pull/1786
+    """
+
+    filename = path("$data_path/coverage_xmpsidecar_isXmpType.xmp")
+    commands = ["$exiv2 $filename"]
+    stderr = ["""Error: XMP Toolkit error 201: XML parsing failure
+Warning: Failed to decode XMP metadata.
+$filename: No Exif data found in the file
+"""]
+    retval = [253]
+
+    compare_stdout = check_no_ASAN_UBSAN_errors
diff --git a/tests/bugfixes/github/test_issue_ghsa_v5g7_46xf_h728.py b/tests/bugfixes/github/test_issue_ghsa_v5g7_46xf_h728.py
@@ -0,0 +1,18 @@
+# -*- coding: utf-8 -*-
+
+from system_tests import CaseMeta, CopyTmpFiles, path, check_no_ASAN_UBSAN_errors
+
+class Jp2ImageEncodeJp2HeaderOutOfBoundsRead2(metaclass=CaseMeta):
+    """
+    Regression test for the bug described in:
+    https://github.com/Exiv2/exiv2/security/advisories/GHSA-v5g7-46xf-h728
+    """
+    url = "https://github.com/Exiv2/exiv2/security/advisories/GHSA-v5g7-46xf-h728"
+
+    filename = path("$data_path/issue_ghsa_v5g7_46xf_h728_poc.exv")
+    commands = ["$exiv2 $filename"]
+    stdout = [""]
+    stderr = ["""Exiv2 exception in print action for file $filename:
+Invalid XmpText type `'
+"""]
+    retval = [1]