From ae16332a113dd3e16d748d6ee6f0aa296f5ace91 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Wed, 7 May 2025 15:42:58 -0700 Subject: [PATCH] Fix handling of null StringRef file buffers (#5428) The current behavior hits UBSAN and ASAN issues. Note, `RequiresNullTerminator` is already set to `false` in `source_buffer.cpp`; setting it in `compile_helper.cpp` is making things more consistent. The related logic is an [assert fail](https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/MemoryBuffer.cpp#L52). This was fuzzer-discovered. --- toolchain/lex/lex.cpp | 27 +++++++++++++++++++------ toolchain/lex/tokenized_buffer_test.cpp | 8 ++++++++ toolchain/parse/tree_test.cpp | 5 +++++ toolchain/testing/compile_helper.cpp | 6 ++++-- 4 files changed, 38 insertions(+), 8 deletions(-) diff --git a/toolchain/lex/lex.cpp b/toolchain/lex/lex.cpp index c6c9ad17651c..c7cf18d30f31 100644 --- a/toolchain/lex/lex.cpp +++ b/toolchain/lex/lex.cpp @@ -216,6 +216,10 @@ class [[clang::internal_linkage]] Lexer { // marker. auto EndDumpSemIRRangeIfIncomplete(const char* diag_loc) -> void; + auto has_dump_sem_ir_ranges() -> bool { + return buffer_.has_dump_sem_ir_ranges(); + } + private: class ErrorRecoveryBuffer; @@ -683,10 +687,14 @@ static auto DispatchNext(Lexer& lexer, llvm::StringRef source_text, source_text[position])](lexer, source_text, position); } - // Incomplete ranges will use the next token for their end; we want that to be - // `FileEnd` in this case, so check before adding `FileEnd`. The argument is - // just the final character for diagnostic locations. - lexer.EndDumpSemIRRangeIfIncomplete(source_text.end() - 1); + if (lexer.has_dump_sem_ir_ranges()) { + // Incomplete ranges will use the next token for their end; we want that to + // be `FileEnd` in this case, so check before adding `FileEnd`. The argument + // is just the final character for diagnostic locations. + // TODO: This offset may not be needed if `file_test` handled diagnostics + // pointing at `.end()`. + lexer.EndDumpSemIRRangeIfIncomplete(source_text.end() - 1); + } // When we finish the source text, stop recursing. We also hint this so that // the tail-dispatch is optimized as that's essentially the loop back-edge @@ -768,6 +776,13 @@ auto Lexer::Lex() && -> TokenizedBuffer { } auto Lexer::MakeLines(llvm::StringRef source_text) -> void { + if (source_text.empty()) { + // Construct a single line for empty input. + buffer_.AddLine(TokenizedBuffer::LineInfo(0)); + line_index_ = 0; + return; + } + // We currently use `memchr` here which typically is well optimized to use // SIMD or other significantly faster than byte-wise scanning. We also use // carefully selected variables and the `ssize_t` type for performance and @@ -898,8 +913,8 @@ auto Lexer::LexCommentOrSlash(llvm::StringRef source_text, ssize_t& position) auto Lexer::BeginDumpSemIRRange(const char* diag_loc) -> void { EndDumpSemIRRangeIfIncomplete(diag_loc); - // The begin here will be the next token, which may be FileEnd. The end will - // be assigned by either AddDumpSemIREnd or, if invalid, + // The begin here will be the next token, which may be dump-sem-ir-begin. The + // end will be assigned by either AddDumpSemIREnd or, if invalid, // EndDumpSemIRRangeIfIncomplete. buffer_.dump_sem_ir_ranges_.push_back( {.begin = TokenIndex(buffer_.size()), .end = TokenIndex::None}); diff --git a/toolchain/lex/tokenized_buffer_test.cpp b/toolchain/lex/tokenized_buffer_test.cpp index 6d241cdf1595..c31cb416df90 100644 --- a/toolchain/lex/tokenized_buffer_test.cpp +++ b/toolchain/lex/tokenized_buffer_test.cpp @@ -50,6 +50,14 @@ TEST_F(LexerTest, HandlesEmptyBuffer) { {.kind = TokenKind::FileEnd}})); } +TEST_F(LexerTest, NullStringRef) { + auto& buffer = compile_helper_.GetTokenizedBuffer(llvm::StringRef()); + EXPECT_FALSE(buffer.has_errors()); + EXPECT_THAT(buffer, HasTokens(llvm::ArrayRef{ + {.kind = TokenKind::FileStart}, + {.kind = TokenKind::FileEnd}})); +} + TEST_F(LexerTest, TracksLinesAndColumns) { auto& buffer = compile_helper_.GetTokenizedBuffer( "\n ;;\n ;;;\n x\"foo\" '''baz\n a\n ''' y"); diff --git a/toolchain/parse/tree_test.cpp b/toolchain/parse/tree_test.cpp index 48171fc5e48f..6e63e29aedc4 100644 --- a/toolchain/parse/tree_test.cpp +++ b/toolchain/parse/tree_test.cpp @@ -40,6 +40,11 @@ TEST_F(TreeTest, IsValid) { EXPECT_TRUE((*tree.postorder().begin()).has_value()); } +TEST_F(TreeTest, NullStringRef) { + Tree& tree = compile_helper_.GetTree(llvm::StringRef()); + EXPECT_TRUE((*tree.postorder().begin()).has_value()); +} + TEST_F(TreeTest, AsAndTryAs) { auto [tokens, tree_and_subtrees] = compile_helper_.GetTokenizedBufferWithTreeAndSubtrees("fn F();"); diff --git a/toolchain/testing/compile_helper.cpp b/toolchain/testing/compile_helper.cpp index 5d3330ff0b0c..3021b267f1a0 100644 --- a/toolchain/testing/compile_helper.cpp +++ b/toolchain/testing/compile_helper.cpp @@ -50,8 +50,10 @@ auto CompileHelper::GetTokenizedBufferWithTreeAndSubtrees(llvm::StringRef text) auto CompileHelper::GetSourceBuffer(llvm::StringRef text) -> SourceBuffer& { std::string filename = llvm::formatv("test{0}.carbon", ++file_index_); - CARBON_CHECK(fs_.addFile(filename, /*ModificationTime=*/0, - llvm::MemoryBuffer::getMemBuffer(text))); + CARBON_CHECK( + fs_.addFile(filename, /*ModificationTime=*/0, + llvm::MemoryBuffer::getMemBuffer( + text, filename, /*RequiresNullTerminator=*/false))); source_storage_.push_front( std::move(*SourceBuffer::MakeFromFile(fs_, filename, consumer_))); return source_storage_.front();