From 31a8d9c711dfdbdb4b11067125d763b1d570033b Mon Sep 17 00:00:00 2001 From: Stephan Bergmann Date: Fri, 26 Oct 2018 13:16:57 +0200 Subject: Don't read past end of string in Guess ctor : > ==26422==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x604000accc72 at pc 0x2ae43e63f4b6 bp 0x2ae43e600510 sp 0x2ae43e600508 > READ of size 1 at 0x604000accc72 thread T70 (cppu_threadpool) > #0 0x2ae43e63f4b5 in Guess::Guess(char const*) /lingucomponent/source/languageguessing/guess.cxx:95:28 > #1 0x2ae43e667f2f in SimpleGuesser::GetManagedLanguages(char) /lingucomponent/source/languageguessing/simpleguesser.cxx:169:19 > #2 0x2ae43e668420 in SimpleGuesser::GetAvailableLanguages() /lingucomponent/source/languageguessing/simpleguesser.cxx:179:12 > #3 0x2ae43e64a18e in LangGuess_Impl::getEnabledLanguages() /lingucomponent/source/languageguessing/guesslang.cxx:229:24 [...] > 0x604000accc72 is located 0 bytes to the right of 34-byte region [0x604000accc50,0x604000accc72) > allocated by thread T70 (cppu_threadpool) here: [...] > #7 0x2ae43e65350a in std::string::operator+=(char const*) /home/tdf/lode/opt_private/lib/gcc/x86_64-unknown-linux-gnu/5.2.0/../../../../include/c++/5.2.0/bits/basic_string.h:3355:16 > #8 0x2ae43e667e6e in SimpleGuesser::GetManagedLanguages(char) /lingucomponent/source/languageguessing/simpleguesser.cxx:168:21 > #9 0x2ae43e668420 in SimpleGuesser::GetAvailableLanguages() /lingucomponent/source/languageguessing/simpleguesser.cxx:179:12 > #10 0x2ae43e64a18e in LangGuess_Impl::getEnabledLanguages() /lingucomponent/source/languageguessing/guesslang.cxx:229:24 [...] shows, during UITest_librelogo, the Guess ctor making wrong assumptions about the structure of guess_str and skipping over the terminating NUL. Locally I could see that while most inputs do have the expected syntax of starting with "[" and containing two "-", one input is indeed just "[haw-utf8" without a second "-". I don't know where the strings passed into the Guess ctor in the two places in lingucomponent/source/languageguessing/simpleguesser.cxx ultimately come from, and what their guaranteed syntax and their semantics is. So from the existing code and the non--well-formed "[haw-utf8" sample (where the second segment shall apparently designate an encoding, not a country), construct rules how to robustly parse any input into potential language/country/encoding parts. (What is obvious from the call sites is that for one each input will start with "[", and for another the item to parse need neither be "]"- nor NUL-terminated.) (Guess::encoding_str and the local enc variable have effectively been unused ever since the code's introduction in 0762811922c4e1a3474bdd91daf308baafbc18e4 "INTEGRATION: CWS languageguessing". Guess::encoding_str, but not the local enc variable, got later removed with b275246c30ce3796cd22f72cd82c58b5cf4c86f0 "loplugin:unusedfields in formula..registry".) Change-Id: Icbedc05ed5b119ee4efbc3118cc17076a4d80c74 Reviewed-on: https://gerrit.libreoffice.org/62390 Tested-by: Jenkins Reviewed-by: Stephan Bergmann --- lingucomponent/source/languageguessing/guess.cxx | 75 ++++++++++++------------ 1 file changed, 36 insertions(+), 39 deletions(-) (limited to 'lingucomponent') diff --git a/lingucomponent/source/languageguessing/guess.cxx b/lingucomponent/source/languageguessing/guess.cxx index eefa7ab6c0f0..a2f3be35382b 100644 --- a/lingucomponent/source/languageguessing/guess.cxx +++ b/lingucomponent/source/languageguessing/guess.cxx @@ -17,6 +17,9 @@ * the License at http://www.apache.org/licenses/LICENSE-2.0 . */ +#include + +#include #include #include @@ -39,11 +42,6 @@ using namespace std; -static bool isSeparator(const char c){ - return c == GUESS_SEPARATOR_OPEN || c == GUESS_SEPARATOR_SEP || c == GUESS_SEPARATOR_CLOSE || c == '\0'; -} - - Guess::Guess() : language_str(DEFAULT_LANGUAGE) , country_str(DEFAULT_COUNTRY) @@ -59,48 +57,47 @@ Guess::Guess(const char * guess_str) : language_str(DEFAULT_LANGUAGE) , country_str(DEFAULT_COUNTRY) { - string lang; - string country; - string enc; - //if the guess is not like "UNKNOWN" or "SHORT", go into the brackets if(strcmp(guess_str + 1, TEXTCAT_RESULT_UNKNOWN_STR) != 0 && strcmp(guess_str + 1, TEXTCAT_RESULT_SHORT_STR) != 0) { - - int current_pointer = 0; - - //this is to go to the first char of the guess string ( the '[' of "[en-US-utf8]" ) - while(!isSeparator(guess_str[current_pointer])){ - current_pointer++; + // From how this ctor is called from SimpleGuesser::GuessLanguage and + // SimpleGuesser::GetManagedLanguages in + // lingucomponent/source/languageguessing/simpleguesser.cxx, guess_str must start with "[": + assert(guess_str[0] == GUESS_SEPARATOR_OPEN); + auto const start = guess_str + 1; + // Only look at the prefix of guess_str, delimited by the next "]" or "[" or end-of-string; + // split it into at most three segments separated by "-" (where excess occurrences of "-" + // would become part of the third segment), like "en-US-utf8"; the first segment denotes the + // language; if there are three segments, the second denotes the country and the third the + // encoding; otherwise, the second segment, if any (e.g., in "haw-utf8"), denotes the + // encoding: + char const * dash1 = nullptr; + char const * dash2 = nullptr; + auto p = start; + for (;; ++p) { + auto const c = *p; + if (c == '\0' || c == GUESS_SEPARATOR_OPEN || c == GUESS_SEPARATOR_CLOSE) { + break; + } + if (c == GUESS_SEPARATOR_SEP) { + if (dash1 == nullptr) { + dash1 = p; + } else { + dash2 = p; + // The encoding is ignored, so we can stop as soon as we found the second "-": + break; + } + } } - current_pointer++; - - //this is to pick up the language ( the "en" from "[en-US-utf8]" ) - while(!isSeparator(guess_str[current_pointer])){ - lang+=guess_str[current_pointer]; - current_pointer++; - } - current_pointer++; - - //this is to pick up the country ( the "US" from "[en-US-utf8]" ) - while(!isSeparator(guess_str[current_pointer])){ - country+=guess_str[current_pointer]; - current_pointer++; + auto const langLen = (dash1 == nullptr ? p : dash1) - start; + if (langLen != 0) { // if not we use the default value + language_str.assign(start, langLen); } - current_pointer++; - - //this is to pick up the encoding ( the "utf8" from "[en-US-utf8]" ) - while(!isSeparator(guess_str[current_pointer])){ - enc+=guess_str[current_pointer]; - current_pointer++; - } - - if(!lang.empty()){//if not we use the default value - language_str=lang; + if (dash2 != nullptr) { + country_str.assign(dash1 + 1, dash2 - (dash1 + 1)); } - country_str=country; } } -- cgit