Fix a stack use after scope and a heap use after free found by fuzzing. (#3126)

There were two issues contributing to this crash:

- Primarily, the issue is that we queue up diagnostics and don't format
them into a string until we reach the end of compilation. In some code
paths in the driver, we destroyed the Semantics IR object before this
happened. But diagnostics can contain references to Semantics IR
objects, such as strings stored in the string table, which can lead to a
use after destruction bug.

This is fixed by ensuring the diagnotics consumer is flushed before
destroying any of the objects that it can refer to. The current approach
to this is not especially clean, unfortunately, but this requires
fighting C++ as this isn't the order in which it wants to destroy
things.

- This issue was obscured by the Semantics IR's string table holding a
reference to whatever underlying storage it was given rather than its
own string storage, so sometimes it would hold a reference to a string
from the source file, and sometimes a string from the tokenized buffer's
string table. The diagnostics were always flushed before the source file
was destroyed, but not before the tokenized buffer was destroyed. So to
see the issue, you'd need to have a string literal with certain contents
followed by an identifier with a name that matched those contents.

The crash is made more reliable by holding references to the Semantics
IR's string map in its string table, rather than references to someone
else's strings. This also fixes a latent bug where passing a string
temporary to SemanticsIR::AddString would store a dangling reference in
the string table. Incidentally, AddString is also changed to perform
only one hash table lookup rather than two for each added string.
This commit is contained in:
Richard Smith
2023-08-22 00:51:13 +00:00
committed by GitHub
parent d74b8f0497
commit e05523db21
3 changed files with 53 additions and 18 deletions
+12 -1
View File
@@ -400,7 +400,7 @@ auto Driver::Compile(const CompileOptions& options) -> bool {
CARBON_VLOG() << "*** SourceBuffer::CreateFromFile done ***\n";
// Require flushing the consumer before the source buffer is destroyed,
// because diagnostics may reference the buffer.
auto flush = llvm::make_scope_exit([&]() { consumer->Flush(); });
auto flush_for_source = llvm::make_scope_exit([&]() { consumer->Flush(); });
if (!source.ok()) {
error_stream_ << "ERROR: Unable to open input source file: "
<< source.error();
@@ -410,6 +410,8 @@ auto Driver::Compile(const CompileOptions& options) -> bool {
CARBON_VLOG() << "*** TokenizedBuffer::Lex ***\n";
auto tokenized_source = TokenizedBuffer::Lex(*source, *consumer);
// Diagnostics may reference the tokenized buffer.
auto flush_for_tokens = llvm::make_scope_exit([&]() { consumer->Flush(); });
bool has_errors = tokenized_source.has_errors();
CARBON_VLOG() << "*** TokenizedBuffer::Lex done ***\n";
if (options.dump_tokens) {
@@ -424,6 +426,8 @@ auto Driver::Compile(const CompileOptions& options) -> bool {
CARBON_VLOG() << "*** ParseTree::Parse ***\n";
auto parse_tree = ParseTree::Parse(tokenized_source, *consumer, vlog_stream_);
// Diagnostics may reference the parse tree.
auto flush_for_parse = llvm::make_scope_exit([&]() { consumer->Flush(); });
has_errors |= parse_tree.has_errors();
CARBON_VLOG() << "*** ParseTree::Parse done ***\n";
if (options.dump_parse_tree) {
@@ -439,6 +443,8 @@ auto Driver::Compile(const CompileOptions& options) -> bool {
CARBON_VLOG() << "*** SemanticsIR::MakeFromParseTree ***\n";
const SemanticsIR semantics_ir = SemanticsIR::MakeFromParseTree(
builtin_ir, tokenized_source, parse_tree, *consumer, vlog_stream_);
// Diagnostics may reference the semantics IR.
auto flush_for_ir = llvm::make_scope_exit([&]() { consumer->Flush(); });
has_errors |= semantics_ir.has_errors();
CARBON_VLOG() << "*** SemanticsIR::MakeFromParseTree done ***\n";
if (options.dump_raw_semantics_ir) {
@@ -463,6 +469,11 @@ auto Driver::Compile(const CompileOptions& options) -> bool {
CARBON_VLOG() << "*** Stopping before lowering due to syntax errors ***";
return false;
}
// Emit diagnostics now, so that the developer sees them sooner and doesn't
// need to wait for code generation.
// TODO: If we allow lowering to produce warnings, should we interleave them
// with diagnostics produced by earlier steps?
consumer->Flush();
CARBON_VLOG() << "*** LowerToLLVM ***\n";