From c4da2ff42da55ffc38c77a9170dc151216d75962 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 3 Feb 2021 17:40:05 +0100 Subject: Avoid overstepping bounds in config file parsing Our configuration files are not security relevant, but having a parser which avoids crashing on them even if they are seriously messed up is not a bad idea anyway. It is also a good opportunity to brush up the code a bit avoiding a few small string copies with our string_view. --- apt-pkg/contrib/configuration.cc | 114 +++++++++++++++++++-------------------- apt-pkg/contrib/string_view.h | 26 +++++++++ test/libapt/stringview_test.cc | 8 +++ 3 files changed, 88 insertions(+), 60 deletions(-) diff --git a/apt-pkg/contrib/configuration.cc b/apt-pkg/contrib/configuration.cc index 554307324..a8fced724 100644 --- a/apt-pkg/contrib/configuration.cc +++ b/apt-pkg/contrib/configuration.cc @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -867,83 +868,76 @@ bool ReadConfigFile(Configuration &Conf,const string &FName,bool const &AsSectio // Now strip comments; if the whole line is contained in a // comment, skip this line. + APT::StringView Line{Input.data(), Input.size()}; - // The first meaningful character in the current fragment; will - // be adjusted below as we remove bytes from the front. - std::string::const_iterator Start = Input.begin(); - // The last meaningful character in the current fragment. - std::string::const_iterator End = Input.end(); - - // Multi line comment - if (InComment == true) + // continued Multi line comment + if (InComment) { - for (std::string::const_iterator I = Start; - I != End; ++I) + size_t end = Line.find("*/"); + if (end != APT::StringView::npos) { - if (*I == '*' && I + 1 != End && I[1] == '/') - { - Start = I + 2; - InComment = false; - break; - } + Line.remove_prefix(end + 2); + InComment = false; } - if (InComment == true) + else continue; } // Discard single line comments - bool InQuote = false; - for (std::string::const_iterator I = Start; - I != End; ++I) { - if (*I == '"') - InQuote = !InQuote; - if (InQuote == true) - continue; - - if ((*I == '/' && I + 1 != End && I[1] == '/') || - (*I == '#' && strcmp(string(I,I+6).c_str(),"#clear") != 0 && - strcmp(string(I,I+8).c_str(),"#include") != 0 && - strcmp(string(I,I+strlen("#x-apt-configure-index")).c_str(), "#x-apt-configure-index") != 0)) + size_t start = 0; + while ((start = Line.find("//", start)) != APT::StringView::npos) + { + if (std::count(Line.begin(), Line.begin() + start, '"') % 2 != 0) + { + ++start; + continue; + } + Line.remove_suffix(Line.length() - start); + break; + } + using APT::operator""_sv; + constexpr std::array magicComments { "clear"_sv, "include"_sv, "x-apt-configure-index"_sv }; + start = 0; + while ((start = Line.find('#', start)) != APT::StringView::npos) { - End = I; + if (std::count(Line.begin(), Line.begin() + start, '"') % 2 != 0 || + std::any_of(magicComments.begin(), magicComments.end(), [&](auto const m) { return Line.compare(start+1, m.length(), m) == 0; })) + { + ++start; + continue; + } + Line.remove_suffix(Line.length() - start); break; } } // Look for multi line comments and build up the // fragment. - Fragment.reserve(End - Start); - InQuote = false; - for (std::string::const_iterator I = Start; - I != End; ++I) + Fragment.reserve(Line.length()); { - if (*I == '"') - InQuote = !InQuote; - if (InQuote == true) - Fragment.push_back(*I); - else if (*I == '/' && I + 1 != End && I[1] == '*') - { - InComment = true; - for (std::string::const_iterator J = I; - J != End; ++J) + size_t start = 0; + while ((start = Line.find("/*", start)) != APT::StringView::npos) + { + if (std::count(Line.begin(), Line.begin() + start, '"') % 2 != 0) { - if (*J == '*' && J + 1 != End && J[1] == '/') - { - // Pretend we just finished walking over the - // comment, and don't add anything to the output - // fragment. - I = J + 1; - InComment = false; - break; - } + start += 2; + continue; } - - if (InComment == true) - break; + Fragment.append(Line.data(), start); + auto const end = Line.find("*/", start + 2); + if (end == APT::StringView::npos) + { + Line.clear(); + InComment = true; + break; + } + else + Line.remove_prefix(end + 2); + start = 0; } - else - Fragment.push_back(*I); + if (not Line.empty()) + Fragment.append(Line.data(), Line.length()); } // Skip blank lines. @@ -951,9 +945,9 @@ bool ReadConfigFile(Configuration &Conf,const string &FName,bool const &AsSectio continue; // The line has actual content; interpret what it means. - InQuote = false; - Start = Fragment.begin(); - End = Fragment.end(); + bool InQuote = false; + auto Start = Fragment.cbegin(); + auto End = Fragment.cend(); for (std::string::const_iterator I = Start; I != End; ++I) { diff --git a/apt-pkg/contrib/string_view.h b/apt-pkg/contrib/string_view.h index 05aad3327..04f6ff115 100644 --- a/apt-pkg/contrib/string_view.h +++ b/apt-pkg/contrib/string_view.h @@ -39,6 +39,10 @@ public: StringView(const char *data) : data_(data), size_(strlen(data)) {} StringView(std::string const & str): data_(str.data()), size_(str.size()) {} + /* Modifiers */ + void remove_prefix(size_t n) { data_ += n; size_ -= n; } + void remove_suffix(size_t n) { size_ -= n; } + void clear() { size_ = 0; } /* Viewers */ constexpr StringView substr(size_t pos, size_t n = npos) const { @@ -76,6 +80,28 @@ public: return found - data_; } + size_t find(APT::StringView const needle) const { + if (needle.empty()) + return npos; + if (needle.length() == 1) + return find(*needle.data()); + size_t found = 0; + while ((found = find(*needle.data(), found)) != npos) { + if (compare(found, needle.length(), needle) == 0) + return found; + ++found; + } + return found; + } + size_t find(APT::StringView const needle, size_t pos) const { + if (pos == 0) + return find(needle); + size_t const found = substr(pos).find(needle); + if (found == npos) + return npos; + return pos + found; + } + /* Conversions */ std::string to_string() const { return std::string(data_, size_); diff --git a/test/libapt/stringview_test.cc b/test/libapt/stringview_test.cc index 03d82517b..9cfaa3b48 100644 --- a/test/libapt/stringview_test.cc +++ b/test/libapt/stringview_test.cc @@ -74,6 +74,14 @@ TEST(StringViewTest,Find) EXPECT_EQ(defString.to_string().find('e',3), defString.find('e',3)); EXPECT_EQ(defString.to_string().find('l',6), defString.find('l',6)); EXPECT_EQ(defString.to_string().find('l',11), defString.find('l',11)); + + EXPECT_EQ(defString.to_string().find("l"), defString.find("l")); + EXPECT_EQ(defString.to_string().find("ll"), defString.find("ll")); + EXPECT_EQ(defString.to_string().find("lo"), defString.find("lo")); + EXPECT_EQ(defString.to_string().find("ll", 1), defString.find("ll", 1)); + EXPECT_EQ(defString.to_string().find("ll", 6), defString.find("ll", 6)); + EXPECT_EQ(defString.to_string().find("or"), defString.find("or")); + EXPECT_EQ(defString.to_string().find("od"), defString.find("od")); } TEST(StringViewTest,RFind) -- cgit v1.2.3-18-g5258