From 8a2ef22c2a329c2c26e1a2ad8f356c9579babf7d Mon Sep 17 00:00:00 2001 From: Jon Meow <46229924+jonmeow@users.noreply.github.com> Date: Wed, 2 Mar 2022 13:43:03 -0800 Subject: [PATCH] Validate source text size and fix empty buffer bugs. (#1113) There's currently a bug with empty files, in that it initializes SourceBuffer with an invalid StringRef that results in a crash. That got me looking at the std::optional TODO, but the issue is that there are really three states: - Buffered - mmapped (not buffered) - Moved out of (no longer initialized) Technically an optional could work if we initialize the buffer on move out, indicating the mmap is gone. But the mode setup felt better to me. And then this also adds the size check. Which is really how I started looking at this. --- toolchain/lexer/tokenized_buffer_fuzzer.cpp | 2 +- toolchain/lexer/tokenized_buffer_test.cpp | 3 +- toolchain/parser/parse_tree_fuzzer.cpp | 2 +- toolchain/parser/parse_tree_test.cpp | 3 +- toolchain/semantics/semantics_test.cpp | 2 +- toolchain/source/source_buffer.cpp | 70 ++++++++++++++++----- toolchain/source/source_buffer.h | 57 +++++------------ toolchain/source/source_buffer_test.cpp | 29 ++++++--- 8 files changed, 99 insertions(+), 69 deletions(-) diff --git a/toolchain/lexer/tokenized_buffer_fuzzer.cpp b/toolchain/lexer/tokenized_buffer_fuzzer.cpp index a4eeb38992b9..e1122ea841a2 100644 --- a/toolchain/lexer/tokenized_buffer_fuzzer.cpp +++ b/toolchain/lexer/tokenized_buffer_fuzzer.cpp @@ -28,7 +28,7 @@ extern "C" int LLVMFuzzerTestOneInput(const unsigned char* data, auto source = SourceBuffer::CreateFromText( llvm::StringRef(reinterpret_cast(data), size)); - auto buffer = TokenizedBuffer::Lex(source, NullDiagnosticConsumer()); + auto buffer = TokenizedBuffer::Lex(*source, NullDiagnosticConsumer()); if (buffer.HasErrors()) { return 0; } diff --git a/toolchain/lexer/tokenized_buffer_test.cpp b/toolchain/lexer/tokenized_buffer_test.cpp index dc623ea04d84..319edd4faf58 100644 --- a/toolchain/lexer/tokenized_buffer_test.cpp +++ b/toolchain/lexer/tokenized_buffer_test.cpp @@ -32,7 +32,8 @@ using ::testing::StrEq; class LexerTest : public ::testing::Test { protected: auto GetSourceBuffer(llvm::Twine text) -> SourceBuffer& { - source_storage.push_back(SourceBuffer::CreateFromText(text.str())); + source_storage.push_back( + std::move(*SourceBuffer::CreateFromText(text.str()))); return source_storage.back(); } diff --git a/toolchain/parser/parse_tree_fuzzer.cpp b/toolchain/parser/parse_tree_fuzzer.cpp index 17a7170abe38..01093dcb9139 100644 --- a/toolchain/parser/parse_tree_fuzzer.cpp +++ b/toolchain/parser/parse_tree_fuzzer.cpp @@ -28,7 +28,7 @@ extern "C" int LLVMFuzzerTestOneInput(const unsigned char* data, llvm::StringRef(reinterpret_cast(data), size)); // Lex the input. - auto tokens = TokenizedBuffer::Lex(source, NullDiagnosticConsumer()); + auto tokens = TokenizedBuffer::Lex(*source, NullDiagnosticConsumer()); if (tokens.HasErrors()) { return 0; } diff --git a/toolchain/parser/parse_tree_test.cpp b/toolchain/parser/parse_tree_test.cpp index 4b2d6d40ac16..856671e14e71 100644 --- a/toolchain/parser/parse_tree_test.cpp +++ b/toolchain/parser/parse_tree_test.cpp @@ -32,7 +32,8 @@ using ::testing::StrEq; class ParseTreeTest : public ::testing::Test { protected: auto GetSourceBuffer(llvm::Twine t) -> SourceBuffer& { - source_storage.push_front(SourceBuffer::CreateFromText(t.str())); + source_storage.push_front( + std::move(*SourceBuffer::CreateFromText(t.str()))); return source_storage.front(); } diff --git a/toolchain/semantics/semantics_test.cpp b/toolchain/semantics/semantics_test.cpp index ba50066d8f6b..4f700e2f9096 100644 --- a/toolchain/semantics/semantics_test.cpp +++ b/toolchain/semantics/semantics_test.cpp @@ -20,7 +20,7 @@ namespace { class ParseTreeTest : public ::testing::Test { protected: auto Analyze(llvm::Twine t) -> Semantics { - source_buffer.emplace(SourceBuffer::CreateFromText(t.str())); + source_buffer.emplace(std::move(*SourceBuffer::CreateFromText(t.str()))); tokenized_buffer = TokenizedBuffer::Lex(*source_buffer, consumer); parse_tree = ParseTree::Parse(*tokenized_buffer, consumer); return Semantics::Analyze(*parse_tree, consumer); diff --git a/toolchain/source/source_buffer.cpp b/toolchain/source/source_buffer.cpp index b6aadee6d301..238a9bdf0ad8 100644 --- a/toolchain/source/source_buffer.cpp +++ b/toolchain/source/source_buffer.cpp @@ -11,16 +11,34 @@ #include #include +#include +#include #include +#include #include "common/check.h" #include "llvm/ADT/ScopeExit.h" +#include "llvm/Support/Error.h" namespace Carbon { +// Verifies that the content size is within limits. +static auto CheckContentSize(int64_t size) -> llvm::Error { + if (size < std::numeric_limits::max()) { + return llvm::Error::success(); + } + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "Input too large!"); +} + auto SourceBuffer::CreateFromText(llvm::Twine text, llvm::StringRef filename) - -> SourceBuffer { - return SourceBuffer(filename, text.str()); + -> llvm::Expected { + std::string buffer = text.str(); + auto size_check = CheckContentSize(buffer.size()); + if (size_check) { + return size_check; + } + return SourceBuffer(filename.str(), std::move(buffer)); } static auto ErrnoToError(int errno_value) -> llvm::Error { @@ -30,10 +48,11 @@ static auto ErrnoToError(int errno_value) -> llvm::Error { auto SourceBuffer::CreateFromFile(llvm::StringRef filename) -> llvm::Expected { - SourceBuffer buffer(filename); + // Add storage to ensure there's a nul-terminator for open(). + std::string filename_str = filename.str(); errno = 0; - int file_descriptor = open(buffer.filename_.c_str(), O_RDONLY); + int file_descriptor = open(filename_str.c_str(), O_RDONLY); if (file_descriptor == -1) { return ErrnoToError(errno); } @@ -50,8 +69,12 @@ auto SourceBuffer::CreateFromFile(llvm::StringRef filename) int64_t size = stat_buffer.st_size; if (size == 0) { - // Nothing to do for an empty file. - return {std::move(buffer)}; + // Rather than opening an empty file, create an empty buffer. + return SourceBuffer(std::move(filename_str), std::string()); + } + auto size_check = CheckContentSize(size); + if (size_check) { + return size_check; } errno = 0; @@ -78,24 +101,39 @@ auto SourceBuffer::CreateFromFile(llvm::StringRef filename) return ErrnoToError(errno); } - buffer.text_ = llvm::StringRef(static_cast(mapped_text), size); - CHECK(!buffer.text_.empty()) + return SourceBuffer( + std::move(filename_str), + llvm::StringRef(static_cast(mapped_text), size)); +} + +SourceBuffer::SourceBuffer(SourceBuffer&& arg) noexcept + // Sets Uninitialized to ensure the input doesn't release mmapped data. + : content_mode_( + std::exchange(arg.content_mode_, ContentMode::Uninitialized)), + filename_(std::move(arg.filename_)), + text_storage_(std::move(arg.text_storage_)), + text_(content_mode_ == ContentMode::Owned ? text_storage_ : arg.text_) {} + +SourceBuffer::SourceBuffer(std::string filename, std::string text) + : content_mode_(ContentMode::Owned), + filename_(std::move(filename)), + text_storage_(std::move(text)), + text_(text_storage_) {} + +SourceBuffer::SourceBuffer(std::string filename, llvm::StringRef text) + : content_mode_(ContentMode::MMapped), + filename_(std::move(filename)), + text_(text) { + CHECK(!text.empty()) << "Must not have an empty text when we have mapped data from a file!"; - return {std::move(buffer)}; } SourceBuffer::~SourceBuffer() { - if (is_string_rep_) { - string_storage_.~decltype(string_storage_)(); - return; - } - - if (!text_.empty()) { + if (content_mode_ == ContentMode::MMapped) { errno = 0; int result = munmap(const_cast(static_cast(text_.data())), text_.size()); - (void)result; CHECK(result != -1) << "Unmapping text failed!"; } } diff --git a/toolchain/source/source_buffer.h b/toolchain/source/source_buffer.h index e2c0a01b509a..99c3f4cf945c 100644 --- a/toolchain/source/source_buffer.h +++ b/toolchain/source/source_buffer.h @@ -35,34 +35,18 @@ class SourceBuffer { public: static auto CreateFromText(llvm::Twine text, llvm::StringRef filename = "/text") - -> SourceBuffer; + -> llvm::Expected; static auto CreateFromFile(llvm::StringRef filename) -> llvm::Expected; // Use one of the factory functions above to create a source buffer. SourceBuffer() = delete; - // Cannot copy as there may be non-trivial owned file data, see the class + // Cannot copy as there may be non-trivial owned file data; see the class // comment for details. SourceBuffer(const SourceBuffer& arg) = delete; - SourceBuffer(SourceBuffer&& arg) noexcept - : filename_(std::move(arg.filename_)), - text_(arg.text_), - is_string_rep_(arg.is_string_rep_) { - // The easy case in when we don't need to transfer an allocated string - // representation. - if (!arg.is_string_rep_) { - // Take ownership of a non-string representation by clearing its text. - arg.text_ = llvm::StringRef(); - return; - } - - // If the argument is using a string rep we need to move that storage over - // and recreate our text `StringRef` to point at our storage. - new (&string_storage_) std::string(std::move(arg.string_storage_)); - text_ = string_storage_; - } + SourceBuffer(SourceBuffer&& arg) noexcept; ~SourceBuffer(); @@ -71,28 +55,21 @@ class SourceBuffer { [[nodiscard]] auto Text() const -> llvm::StringRef { return text_; } private: - SourceBuffer(llvm::StringRef fake_filename, std::string buffer_text) - : filename_(fake_filename.str()), - is_string_rep_(true), - string_storage_(std::move(buffer_text)) { - text_ = string_storage_; - } - - explicit SourceBuffer(llvm::StringRef filename) - : filename_(filename.str()), text_(), is_string_rep_(false) {} - - std::string filename_; - - llvm::StringRef text_; - - bool is_string_rep_; - - // We use a transparent union to avoid constructing the storage. - // FIXME: We should replace this and the boolean with an optional which would - // be much simpler. - union { - std::string string_storage_; + enum class ContentMode { + Uninitialized, + MMapped, + Owned, }; + + // Constructor for mmapped content. + SourceBuffer(std::string filename, llvm::StringRef text); + // Constructor for owned content. + SourceBuffer(std::string filename, std::string text); + + ContentMode content_mode_; + std::string filename_; + std::string text_storage_; + llvm::StringRef text_; }; } // namespace Carbon diff --git a/toolchain/source/source_buffer_test.cpp b/toolchain/source/source_buffer_test.cpp index 2f05695493b0..7f6e206557f9 100644 --- a/toolchain/source/source_buffer_test.cpp +++ b/toolchain/source/source_buffer_test.cpp @@ -15,17 +15,17 @@ namespace Carbon::Testing { namespace { TEST(SourceBufferTest, StringRep) { - SourceBuffer buffer = - SourceBuffer::CreateFromText(llvm::Twine("Hello") + " World"); - - EXPECT_EQ("/text", buffer.Filename()); - EXPECT_EQ("Hello World", buffer.Text()); + auto buffer = SourceBuffer::CreateFromText(llvm::Twine("Hello") + " World"); + EXPECT_EQ("/text", buffer->Filename()); + EXPECT_EQ("Hello World", buffer->Text()); +} +TEST(SourceBufferText, StringRepWithFilename) { // Give a custom filename. - auto buffer2 = + auto buffer = SourceBuffer::CreateFromText("Hello World Again!", "/custom/text"); - EXPECT_EQ("/custom/text", buffer2.Filename()); - EXPECT_EQ("Hello World Again!", buffer2.Text()); + EXPECT_EQ("/custom/text", buffer->Filename()); + EXPECT_EQ("Hello World Again!", buffer->Text()); } auto CreateTestFile(llvm::StringRef text) -> std::string { @@ -58,5 +58,18 @@ TEST(SourceBufferTest, FileRep) { EXPECT_EQ("Hello World", buffer.Text()); } +TEST(SourceBufferTest, FileRepEmpty) { + auto test_file_path = CreateTestFile(""); + + auto expected_buffer = SourceBuffer::CreateFromFile(test_file_path); + ASSERT_TRUE(static_cast(expected_buffer)) + << "Error message: " << toString(expected_buffer.takeError()); + + SourceBuffer& buffer = *expected_buffer; + + EXPECT_EQ(test_file_path, buffer.Filename()); + EXPECT_EQ("", buffer.Text()); +} + } // namespace } // namespace Carbon::Testing