From c261fbaa2567687eec6a595d3016212fd6ae648d Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 16 May 2021 15:05:08 +0100 Subject: [PATCH] Fix quadratic complexity performance bug. --- xmpsdk/src/XMPMeta-Parse.cpp | 57 +++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/xmpsdk/src/XMPMeta-Parse.cpp b/xmpsdk/src/XMPMeta-Parse.cpp index 9f66fe8..f9c37d7 100644 --- a/xmpsdk/src/XMPMeta-Parse.cpp +++ b/xmpsdk/src/XMPMeta-Parse.cpp @@ -976,12 +976,26 @@ ProcessUTF8Portion ( XMLParserAdapter * xmlParser, { const XMP_Uns8 * bufEnd = buffer + length; - const XMP_Uns8 * spanStart = buffer; const XMP_Uns8 * spanEnd; - - for ( spanEnd = spanStart; spanEnd < bufEnd; ++spanEnd ) { - if ( (0x20 <= *spanEnd) && (*spanEnd <= 0x7E) && (*spanEnd != '&') ) continue; // A regular ASCII character. + // `buffer` is copied into this std::string. If `buffer` only + // contains valid UTF-8 and no escape characters, then the copy + // will be identical to the original, but invalid characters are + // replaced - usually with a space character. This std::string was + // added as a performance fix for: + // https://github.com/Exiv2/exiv2/security/advisories/GHSA-w8mv-g8qq-36mj + // Previously, the code was repeatedly calling + // `xmlParser->ParseBuffer()`, which turned out to have quadratic + // complexity, because expat kept reparsing the entire string from + // the beginning. + std::string copy; + + for ( spanEnd = buffer; spanEnd < bufEnd; ++spanEnd ) { + + if ( (0x20 <= *spanEnd) && (*spanEnd <= 0x7E) && (*spanEnd != '&') ) { + copy.push_back(*spanEnd); + continue; // A regular ASCII character. + } if ( *spanEnd >= 0x80 ) { @@ -992,21 +1006,20 @@ ProcessUTF8Portion ( XMLParserAdapter * xmlParser, if ( uniLen > 0 ) { // A valid UTF-8 character, keep it as-is. + copy.append((const char*)spanEnd, uniLen); spanEnd += uniLen - 1; // ! The loop increment will put back the +1. } else if ( (uniLen < 0) && (! last) ) { // Have a partial UTF-8 character at the end of the buffer and more input coming. - xmlParser->ParseBuffer ( spanStart, (spanEnd - spanStart), false ); + xmlParser->ParseBuffer ( copy.c_str(), copy.size(), false ); return (spanEnd - buffer); } else { // Not a valid UTF-8 sequence. Replace the first byte with the Latin-1 equivalent. - xmlParser->ParseBuffer ( spanStart, (spanEnd - spanStart), false ); const char * replacement = kReplaceLatin1 [ *spanEnd - 0x80 ]; - xmlParser->ParseBuffer ( replacement, strlen ( replacement ), false ); - spanStart = spanEnd + 1; // ! The loop increment will do "spanEnd = spanStart". + copy.append ( replacement ); } @@ -1014,11 +1027,12 @@ ProcessUTF8Portion ( XMLParserAdapter * xmlParser, // Replace ASCII controls other than tab, LF, and CR with a space. - if ( (*spanEnd == kTab) || (*spanEnd == kLF) || (*spanEnd == kCR) ) continue; + if ( (*spanEnd == kTab) || (*spanEnd == kLF) || (*spanEnd == kCR) ) { + copy.push_back(*spanEnd); + continue; + } - xmlParser->ParseBuffer ( spanStart, (spanEnd - spanStart), false ); - xmlParser->ParseBuffer ( " ", 1, false ); - spanStart = spanEnd + 1; // ! The loop increment will do "spanEnd = spanStart". + copy.push_back(' '); } else { @@ -1030,18 +1044,21 @@ ProcessUTF8Portion ( XMLParserAdapter * xmlParser, if ( escLen < 0 ) { // Have a partial numeric escape in this buffer, wait for more input. - if ( last ) continue; // No more buffers, not an escape, absorb as normal input. - xmlParser->ParseBuffer ( spanStart, (spanEnd - spanStart), false ); + if ( last ) { + copy.push_back('&'); + continue; // No more buffers, not an escape, absorb as normal input. + } + xmlParser->ParseBuffer ( copy.c_str(), copy.size(), false ); return (spanEnd - buffer); } else if ( escLen > 0 ) { // Have a complete numeric escape to replace. - xmlParser->ParseBuffer ( spanStart, (spanEnd - spanStart), false ); - xmlParser->ParseBuffer ( " ", 1, false ); - spanStart = spanEnd + escLen; - spanEnd = spanStart - 1; // ! The loop continuation will increment spanEnd! + copy.push_back(' '); + spanEnd = spanEnd + escLen - 1; // ! The loop continuation will increment spanEnd! + } else { + copy.push_back('&'); } } @@ -1050,8 +1067,8 @@ ProcessUTF8Portion ( XMLParserAdapter * xmlParser, XMP_Assert ( spanEnd == bufEnd ); - if ( spanStart < bufEnd ) xmlParser->ParseBuffer ( spanStart, (spanEnd - spanStart), false ); - if ( last ) xmlParser->ParseBuffer ( " ", 1, true ); + copy.push_back(' '); + xmlParser->ParseBuffer ( copy.c_str(), copy.size(), true ); return length;