From c100d9389671379cab3f23080c02ee619e122347 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=ADn=20Lucas=20Golini?= Date: Fri, 18 Sep 2026 15:51:39 -0300 Subject: [PATCH] Fix String::trim on all-separator input. Fix RegEx freeing Oniguruma patterns. String::trim: when the input consisted only of separators, both find_first_not_of and find_last_not_of returned npos and the pos2 fallback computed length() - 1, so trim(" ") returned " " and every all-separator value came back one character shorter instead of empty, making an all-whitespace value look non-empty to callers. All eight overloads were affected: std::string, std::string_view, String and String::View, in their char and separator-set forms. Return an empty result when there is no non-separator, and compute the trimmed range as substr(pos1, pos2 - pos1 + 1). RegEx: mCompiledPattern stores whichever engine compiled the pattern, a pcre2_code* or an OnigRegex, but the destructor released it with pcre2_code_free() whenever RegExCache did not own it. With useCache = false that freed an Oniguruma pattern as a PCRE2 block, which corrupts the heap: constructing and destroying such a RegEx crashes with SIGSEGV, a path snippetparser.cpp reaches by passing Utf | AllowFallback with useCache = false. Release the pattern with the deallocator of the engine recorded in Options::UseOniguruma, the predicate matches(), getCaptureCount() and RegExCache::clear() already use, and remove the mOnigEngine flag, which was never set or read anywhere. Both fixes come with regression tests. String.trim covers the all-separator boundary and the trimmed range of every overload. RegExEngines.uncachedPatternIsFreedByItsOwnEngine destroys uncached patterns from both engines; it crashed the test binary before the fix. --- include/eepp/system/regex.hpp | 3 +- src/eepp/core/string.cpp | 48 ++++++++++++------ src/eepp/system/regex.cpp | 11 ++++- src/tests/unit_tests/regex_tests.cpp | 20 ++++++++ .../unit_tests/stringsoperations_tests.cpp | 49 +++++++++++++++++++ 5 files changed, 112 insertions(+), 19 deletions(-) diff --git a/include/eepp/system/regex.hpp b/include/eepp/system/regex.hpp index 98f16aaaa..ce15759bf 100644 --- a/include/eepp/system/regex.hpp +++ b/include/eepp/system/regex.hpp @@ -92,10 +92,11 @@ class EE_API RegEx : public PatternMatcher { protected: std::string_view mPattern; mutable size_t mMatchNum; + /** Compiled pattern, owned by the engine named in the Options::UseOniguruma bit of mOptions: + * pcre2_code* or OnigRegex respectively. The cache owns it when mCached is set. */ void* mCompiledPattern; int mCaptureCount{ 0 }; Uint32 mOptions{ Options::Utf | Options::AllowFallback }; - bool mOnigEngine : 1 { false }; bool mValid : 1 { false }; bool mCached : 1 { false }; bool mFilterOutCaptures : 1 { false }; diff --git a/src/eepp/core/string.cpp b/src/eepp/core/string.cpp index 8fc2855d6..61898471e 100644 --- a/src/eepp/core/string.cpp +++ b/src/eepp/core/string.cpp @@ -1215,9 +1215,11 @@ std::string String::rTrim( const std::string& str, char character ) { std::string String::trim( const std::string& str, char character ) { std::string::size_type pos1 = str.find_first_not_of( character ); + // A string made only of separators has nothing left once trimmed. + if ( pos1 == std::string::npos ) + return {}; std::string::size_type pos2 = str.find_last_not_of( character ); - return str.substr( pos1 == std::string::npos ? 0 : pos1, - pos2 == std::string::npos ? str.length() - 1 : pos2 - pos1 + 1 ); + return str.substr( pos1, pos2 - pos1 + 1 ); } std::string_view String::lTrim( const std::string_view& str, char character ) { @@ -1232,9 +1234,11 @@ std::string_view String::rTrim( const std::string_view& str, char character ) { std::string_view String::trim( const std::string_view& str, char character ) { std::string::size_type pos1 = str.find_first_not_of( character ); + // A string made only of separators has nothing left once trimmed. + if ( pos1 == std::string::npos ) + return {}; std::string::size_type pos2 = str.find_last_not_of( character ); - return str.substr( pos1 == std::string::npos ? 0 : pos1, - pos2 == std::string::npos ? str.length() - 1 : pos2 - pos1 + 1 ); + return str.substr( pos1, pos2 - pos1 + 1 ); } String::View String::lTrim( const String::View& str, char character ) { @@ -1249,9 +1253,11 @@ String::View String::rTrim( const String::View& str, char character ) { String::View String::trim( const String::View& str, char character ) { String::View::size_type pos1 = str.find_first_not_of( character ); + // A string made only of separators has nothing left once trimmed. + if ( pos1 == String::View::npos ) + return {}; String::View::size_type pos2 = str.find_last_not_of( character ); - return str.substr( pos1 == String::View::npos ? 0 : pos1, - pos2 == String::View::npos ? str.length() - 1 : pos2 - pos1 + 1 ); + return str.substr( pos1, pos2 - pos1 + 1 ); } void String::trimInPlace( std::string& str, char character ) { @@ -1272,9 +1278,11 @@ String String::rTrim( const String& str, char character ) { String String::trim( const String& str, char character ) { StringType::size_type pos1 = str.find_first_not_of( character ); + // A string made only of separators has nothing left once trimmed. + if ( pos1 == String::InvalidPos ) + return {}; StringType::size_type pos2 = str.find_last_not_of( character ); - return str.substr( pos1 == String::InvalidPos ? 0 : pos1, - pos2 == String::InvalidPos ? str.length() - 1 : pos2 - pos1 + 1 ); + return str.substr( pos1, pos2 - pos1 + 1 ); } void String::trimInPlace( String& str, char character ) { @@ -1293,9 +1301,11 @@ std::string String::rTrim( const std::string& str, std::string_view characters ) std::string String::trim( const std::string& str, std::string_view characters ) { std::string::size_type pos1 = str.find_first_not_of( characters ); + // A string made only of separators has nothing left once trimmed. + if ( pos1 == std::string::npos ) + return {}; std::string::size_type pos2 = str.find_last_not_of( characters ); - return str.substr( pos1 == std::string::npos ? 0 : pos1, - pos2 == std::string::npos ? str.length() - 1 : pos2 - pos1 + 1 ); + return str.substr( pos1, pos2 - pos1 + 1 ); } std::string_view String::lTrim( const std::string_view& str, std::string_view characters ) { @@ -1310,9 +1320,11 @@ std::string_view String::rTrim( const std::string_view& str, std::string_view ch std::string_view String::trim( const std::string_view& str, std::string_view characters ) { std::string::size_type pos1 = str.find_first_not_of( characters ); + // A string made only of separators has nothing left once trimmed. + if ( pos1 == std::string::npos ) + return {}; std::string::size_type pos2 = str.find_last_not_of( characters ); - return str.substr( pos1 == std::string::npos ? 0 : pos1, - pos2 == std::string::npos ? str.length() - 1 : pos2 - pos1 + 1 ); + return str.substr( pos1, pos2 - pos1 + 1 ); } String::View String::lTrim( const String::View& str, String::View characters ) { @@ -1327,9 +1339,11 @@ String::View String::rTrim( const String::View& str, String::View characters ) { String::View String::trim( const String::View& str, String::View characters ) { String::View::size_type pos1 = str.find_first_not_of( characters ); + // A string made only of separators has nothing left once trimmed. + if ( pos1 == String::View::npos ) + return {}; String::View::size_type pos2 = str.find_last_not_of( characters ); - return str.substr( pos1 == String::View::npos ? 0 : pos1, - pos2 == String::View::npos ? str.length() - 1 : pos2 - pos1 + 1 ); + return str.substr( pos1, pos2 - pos1 + 1 ); } void String::trimInPlace( std::string& str, std::string_view characters ) { @@ -1348,9 +1362,11 @@ String String::rTrim( const String& str, std::string_view characters ) { String String::trim( const String& str, std::string_view characters ) { StringType::size_type pos1 = str.find_first_not_of( characters ); + // A string made only of separators has nothing left once trimmed. + if ( pos1 == String::InvalidPos ) + return {}; StringType::size_type pos2 = str.find_last_not_of( characters ); - return str.substr( pos1 == String::InvalidPos ? 0 : pos1, - pos2 == String::InvalidPos ? str.length() - 1 : pos2 - pos1 + 1 ); + return str.substr( pos1, pos2 - pos1 + 1 ); } void String::trimInPlace( String& str, std::string_view characters ) { diff --git a/src/eepp/system/regex.cpp b/src/eepp/system/regex.cpp index 872457d13..091273400 100644 --- a/src/eepp/system/regex.cpp +++ b/src/eepp/system/regex.cpp @@ -135,9 +135,16 @@ RegEx::RegEx( std::string_view pattern, Uint32 options, bool useCache ) : } RegEx::~RegEx() { - if ( !mCached && mCompiledPattern != nullptr ) { + if ( mCached || mCompiledPattern == nullptr ) + return; + + // The pattern is owned by whichever engine compiled it, so it must be released with that + // engine's deallocator: freeing an Oniguruma pattern with pcre2_code_free() (or the reverse) + // corrupts the heap. The cache, which owns the patterns it hands out, does the same split. + if ( mOptions & Options::UseOniguruma ) + onig_free( static_cast( mCompiledPattern ) ); + else pcre2_code_free( reinterpret_cast( mCompiledPattern ) ); - } } bool RegEx::matches( const char* stringSearch, int stringStartOffset, diff --git a/src/tests/unit_tests/regex_tests.cpp b/src/tests/unit_tests/regex_tests.cpp index 849697b4d..7d2587026 100644 --- a/src/tests/unit_tests/regex_tests.cpp +++ b/src/tests/unit_tests/regex_tests.cpp @@ -147,3 +147,23 @@ UTEST( RegExEngines, basicTest ) { EXPECT_EQ( 38, matchesOniguruma[0].end ); RegExCache::destroySingleton(); } + +UTEST( RegExEngines, uncachedPatternIsFreedByItsOwnEngine ) { + // A pattern compiled by Oniguruma but not owned by the cache has to be released with onig_free(). + // Releasing it with pcre2_code_free() corrupted the heap and crashed this test binary, so the + // engine that compiled a pattern decides its deallocator (the same split RegExCache::clear() + // makes for the patterns it owns). + { + RegEx oniguruma( "a+", RegEx::Options::Utf | RegEx::Options::UseOniguruma, false ); + EXPECT_EQ( oniguruma.isValid(), true ); + EXPECT_EQ( oniguruma.matches( std::string( "aaa" ) ), true ); + } + + { + RegEx pcre2( "a+", RegEx::Options::Utf | RegEx::Options::AllowFallback, false ); + EXPECT_EQ( pcre2.isValid(), true ); + EXPECT_EQ( pcre2.matches( std::string( "aaa" ) ), true ); + } + + RegExCache::destroySingleton(); +} diff --git a/src/tests/unit_tests/stringsoperations_tests.cpp b/src/tests/unit_tests/stringsoperations_tests.cpp index 49a4fa3f6..7b84b92a9 100644 --- a/src/tests/unit_tests/stringsoperations_tests.cpp +++ b/src/tests/unit_tests/stringsoperations_tests.cpp @@ -85,6 +85,55 @@ UTEST( String, fromStringView ) { EXPECT_EQ( 1234, intValue ); } +UTEST( String, trim ) { + // Separators are removed from both ends and interior ones are kept. + EXPECT_TRUE( String::trim( std::string( "abc" ) ) == std::string( "abc" ) ); + EXPECT_TRUE( String::trim( std::string( " a " ) ) == std::string( "a" ) ); + // The char overload trims only that character, so tab and newline survive it; the + // string_view overload takes a whole separator set. + EXPECT_TRUE( String::trim( std::string( "\t a b \n" ) ) == std::string( "\t a b \n" ) ); + EXPECT_TRUE( String::trim( std::string( " a " ), ' ' ) == std::string( "a" ) ); + EXPECT_TRUE( String::trim( std::string( "xxbxx" ), 'x' ) == std::string( "b" ) ); + EXPECT_TRUE( String::trim( std::string( "\t\r\n a \t\r\n" ), std::string_view( " \t\r\n" ) ) == + std::string( "a" ) ); + + // A string made only of separators has nothing left once trimmed. It used to come back as a + // shorter string of separators instead of as an empty one. + EXPECT_TRUE( String::trim( std::string() ).empty() ); + EXPECT_TRUE( String::trim( std::string( " " ) ).empty() ); + EXPECT_TRUE( String::trim( std::string( " " ) ).empty() ); + EXPECT_TRUE( String::trim( std::string( " " ) ).empty() ); + EXPECT_TRUE( String::trim( std::string( "xxxx" ), 'x' ).empty() ); + EXPECT_TRUE( String::trim( std::string( "\t\r\n " ), std::string_view( " \t\r\n" ) ).empty() ); + + std::string inPlace = " "; + String::trimInPlace( inPlace, ' ' ); + EXPECT_TRUE( inPlace.empty() ); + inPlace = " a "; + String::trimInPlace( inPlace, ' ' ); + EXPECT_TRUE( inPlace == std::string( "a" ) ); + + // The view overloads must report the trimmed range, not a truncated one. + EXPECT_TRUE( String::trim( std::string_view() ).empty() ); + EXPECT_TRUE( String::trim( std::string_view( " " ) ).empty() ); + EXPECT_TRUE( String::trim( std::string_view( " a " ) ) == std::string_view( "a" ) ); + EXPECT_TRUE( String::trim( std::string_view( "xx" ), std::string_view( "x" ) ).empty() ); + EXPECT_TRUE( String::trim( std::string_view( "xxa xx" ), std::string_view( "x " ) ) == + std::string_view( "a" ) ); + + // The UTF-32 overloads are separate implementations and had the same defect. + EXPECT_TRUE( String::trim( String() ).empty() ); + EXPECT_TRUE( String::trim( String( " " ) ).empty() ); + EXPECT_TRUE( String::trim( String( " a " ) ) == String( "a" ) ); + EXPECT_TRUE( String::trim( String( "xxx" ), std::string_view( "x" ) ).empty() ); + EXPECT_TRUE( String::trim( String( " a " ), std::string_view( " " ) ) == String( "a" ) ); + EXPECT_TRUE( String::trim( String::View( U" " ) ).empty() ); + EXPECT_TRUE( String::trim( String::View( U" a " ) ) == String::View( U"a" ) ); + EXPECT_TRUE( String::trim( String::View( U" " ), String::View( U" " ) ).empty() ); + EXPECT_TRUE( String::trim( String::View( U" a " ), String::View( U" " ) ) == + String::View( U"a" ) ); +} + UTEST( String, reusableFormattingAndUtf8Assignment ) { std::string formatted; formatted.reserve( 128 );