From 4df11394a10b3b15d2fb9bde8b831cf68785aa45 Mon Sep 17 00:00:00 2001 From: Michael Buch Date: Fri, 25 Nov 2022 14:45:09 +0000 Subject: [PATCH] [lldb][Target] Flush the scratch TypeSystem when owning lldb_private::Module gets unloaded MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit **Summary** This patch addresses #59128, where LLDB would crash when evaluating importing a type that has been imported before into the same target. The proposed solution is to clear the scratch AST (and associated persistent variables, `ClangASTImporter`, etc.) whenever a module that could've owned one of the stale `TypeSystem`s gets unloaded/destroyed. Details: 1. The first time we evaluate the expression we import the decl for Foo into the Targets scratch AST context (lives in m_scratch_type_system_map). During this process we also create a ClangASTImporter that lives in the ClangPersistentVariables::m_ast_importer_sp. This importer has decl tracking structures which reference the source AST that the decl got imported from. This importer also gets re-used for all calls to DeportType (which we use to copy the final decl into the Targets scratch AST). 2. Rebuilding the executable triggers a tear-down of the Module that was backing the ASTContext that we originally got the Foo decl from (which lived in the Module::m_type_system_map). However, the Target’s scratch AST lives on. 3. Re-running the same expression will now create a new ASTImporterDelegate where the destination TranslationUnitDecl is the same as the one from step (1). 4. When importing the new Foo decl we first try to find it in the destination DeclContext, which happens to be the scratch destination TranslationUnitDecl. The `Foo` decl exists in this context since we copied it into the scratch AST in the first run. The ASTImporter then queries LLDB for the origin of that decl. Using the same persistent variable ClangASTImporter we claim the decl has an origin in the AST context that got torn down with the Module. This faulty origin leads to a use-after-free. **Testing** - Added API test Differential Revision: https://reviews.llvm.org/D138724 --- lldb/source/Target/Target.cpp | 24 ++++++++ .../API/functionalities/rerun_and_expr/Makefile | 1 + .../rerun_and_expr/TestRerunAndExpr.py | 57 +++++++++++++++++ .../API/functionalities/rerun_and_expr/main.cpp | 8 +++ .../API/functionalities/rerun_and_expr/rebuild.cpp | 12 ++++ .../functionalities/rerun_and_expr_dylib/Makefile | 3 + .../rerun_and_expr_dylib/TestRerunAndExprDylib.py | 72 ++++++++++++++++++++++ .../functionalities/rerun_and_expr_dylib/lib.cpp | 1 + .../functionalities/rerun_and_expr_dylib/main.cpp | 12 ++++ .../rerun_and_expr_dylib/rebuild.cpp | 7 +++ 10 files changed, 197 insertions(+) create mode 100644 lldb/test/API/functionalities/rerun_and_expr/Makefile create mode 100644 lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py create mode 100644 lldb/test/API/functionalities/rerun_and_expr/main.cpp create mode 100644 lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp create mode 100644 lldb/test/API/functionalities/rerun_and_expr_dylib/Makefile create mode 100644 lldb/test/API/functionalities/rerun_and_expr_dylib/TestRerunAndExprDylib.py create mode 100644 lldb/test/API/functionalities/rerun_and_expr_dylib/lib.cpp create mode 100644 lldb/test/API/functionalities/rerun_and_expr_dylib/main.cpp create mode 100644 lldb/test/API/functionalities/rerun_and_expr_dylib/rebuild.cpp diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index 22071b7..8a197bf 100644 --- a/lldb/source/Target/Target.cpp +++ b/lldb/source/Target/Target.cpp @@ -1682,6 +1682,30 @@ void Target::ModulesDidUnload(ModuleList &module_list, bool delete_locations) { m_breakpoint_list.UpdateBreakpoints(module_list, false, delete_locations); m_internal_breakpoint_list.UpdateBreakpoints(module_list, false, delete_locations); + + // If a module was torn down it will have torn down the 'TypeSystemClang's + // that we used as source 'ASTContext's for the persistent variables in + // the current target. Those would now be unsafe to access because the + // 'DeclOrigin' are now possibly stale. Thus clear all persistent + // variables. We only want to flush 'TypeSystem's if the module being + // unloaded was capable of describing a source type. JITted module unloads + // happen frequently for Objective-C utility functions or the REPL and rely + // on the persistent variables to stick around. + const bool should_flush_type_systems = + module_list.AnyOf([](lldb_private::Module &module) { + auto *object_file = module.GetObjectFile(); + + if (!object_file) + return true; + + auto type = object_file->GetType(); + + return type == ObjectFile::eTypeObjectFile || + type == ObjectFile::eTypeExecutable; + }); + + if (should_flush_type_systems) + m_scratch_type_system_map.Clear(); } } diff --git a/lldb/test/API/functionalities/rerun_and_expr/Makefile b/lldb/test/API/functionalities/rerun_and_expr/Makefile new file mode 100644 index 0000000..22f1051 --- /dev/null +++ b/lldb/test/API/functionalities/rerun_and_expr/Makefile @@ -0,0 +1 @@ +include Makefile.rules diff --git a/lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py b/lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py new file mode 100644 index 0000000..19907ce --- /dev/null +++ b/lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py @@ -0,0 +1,57 @@ +""" +Test that re-running a process from within the same target +after rebuilding the executable flushes the scratch TypeSystems +tied to that process. +""" + +import lldb +from lldbsuite.test.lldbtest import * +from lldbsuite.test import lldbutil + +class TestRerun(TestBase): + def test(self): + """ + Tests whether re-launching a process without destroying + the owning target keeps invalid ASTContexts in the + scratch AST's importer. + + We test this by: + 1. Evaluating an expression to import 'struct Foo' into + the scratch AST + 2. Change the definition of 'struct Foo' and rebuild the executable + 3. Re-launch the process + 4. Evaluate the same expression in (1). We expect to have only + the latest definition of 'struct Foo' in the scratch AST. + """ + self.build(dictionary={'CXX_SOURCES':'main.cpp', 'EXE':'a.out'}) + (target, _, _, bkpt) = \ + lldbutil.run_to_source_breakpoint(self, 'return', lldb.SBFileSpec('main.cpp')) + + target.BreakpointCreateBySourceRegex('return', lldb.SBFileSpec('rebuild.cpp', False)) + + self.expect_expr('foo', result_type='Foo', result_children=[ + ValueCheck(name='m_val', value='42') + ]) + + self.build(dictionary={'CXX_SOURCES':'rebuild.cpp', 'EXE':'a.out'}) + + self.runCmd('process launch') + + self.expect_expr('foo', result_type='Foo', result_children=[ + ValueCheck(name='Base', children=[ + ValueCheck(name='m_base_val', value='42') + ]), + ValueCheck(name='m_derived_val', value='137') + ]) + + self.filecheck("target module dump ast", __file__) + + # The new definition 'struct Foo' is in the scratch AST + # CHECK: |-CXXRecordDecl {{.*}} struct Foo definition + # CHECK: | |-public 'Base' + # CHECK-NEXT: | `-FieldDecl {{.*}} m_derived_val 'int' + # CHECK-NEXT: `-CXXRecordDecl {{.*}} struct Base definition + + # ...but the original definition of 'struct Foo' is not in the scratch AST anymore + # CHECK-NOT: FieldDecl {{.*}} m_val 'int' + diff --git a/lldb/test/API/functionalities/rerun_and_expr/main.cpp b/lldb/test/API/functionalities/rerun_and_expr/main.cpp new file mode 100644 index 0000000..a19dac0 --- /dev/null +++ b/lldb/test/API/functionalities/rerun_and_expr/main.cpp @@ -0,0 +1,8 @@ +struct Foo { + int m_val = 42; +}; + +int main() { + Foo foo; + return 0; +} diff --git a/lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp b/lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp new file mode 100644 index 0000000..745a2ce --- /dev/null +++ b/lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp @@ -0,0 +1,12 @@ +struct Base { + int m_base_val = 42; +}; + +struct Foo : public Base { + int m_derived_val = 137; +}; + +int main() { + Foo foo; + return 0; +} diff --git a/lldb/test/API/functionalities/rerun_and_expr_dylib/Makefile b/lldb/test/API/functionalities/rerun_and_expr_dylib/Makefile new file mode 100644 index 0000000..99998b2 --- /dev/null +++ b/lldb/test/API/functionalities/rerun_and_expr_dylib/Makefile @@ -0,0 +1,3 @@ +CXX_SOURCES := main.cpp + +include Makefile.rules diff --git a/lldb/test/API/functionalities/rerun_and_expr_dylib/TestRerunAndExprDylib.py b/lldb/test/API/functionalities/rerun_and_expr_dylib/TestRerunAndExprDylib.py new file mode 100644 index 0000000..df75cbe --- /dev/null +++ b/lldb/test/API/functionalities/rerun_and_expr_dylib/TestRerunAndExprDylib.py @@ -0,0 +1,72 @@ +""" +Test that re-running a process from within the same target +after rebuilding the a dynamic library flushes the scratch +TypeSystems tied to that process. +""" + +import lldb +from lldbsuite.test.lldbtest import * +from lldbsuite.test import lldbutil + +class TestRerun(TestBase): + def test(self): + """ + Tests whether re-launching a process without destroying + the owning target keeps invalid ASTContexts in the + scratch AST's importer. + + We test this by: + 1. Evaluating an expression to import 'struct Foo' into + the scratch AST + 2. Change the definition of 'struct Foo' and rebuild the dylib + 3. Re-launch the process + 4. Evaluate the same expression in (1). We expect to have only + the latest definition of 'struct Foo' in the scratch AST. + """ + + # Build a.out + self.build(dictionary={'EXE':'a.out', + 'CXX_SOURCES':'main.cpp'}) + + # Build libfoo.dylib + self.build(dictionary={'DYLIB_CXX_SOURCES':'lib.cpp', + 'DYLIB_ONLY':'YES', + 'DYLIB_NAME':'foo', + 'USE_LIBDL':'1', + 'LD_EXTRAS':'-L.'}) + + (target, _, _, bkpt) = \ + lldbutil.run_to_source_breakpoint(self, 'return', lldb.SBFileSpec('main.cpp')) + + self.expect_expr('*foo', result_type='Foo', result_children=[ + ValueCheck(name='m_val', value='42') + ]) + + # Re-build libfoo.dylib + self.build(dictionary={'DYLIB_CXX_SOURCES':'rebuild.cpp', + 'DYLIB_ONLY':'YES', + 'DYLIB_NAME':'foo', + 'USE_LIBDL':'1', + 'LD_EXTRAS':'-L.'}) + + self.runCmd('process launch') + (target, _, _, bkpt) = \ + lldbutil.run_to_source_breakpoint(self, 'return', lldb.SBFileSpec('main.cpp')) + + self.expect_expr('*foo', result_type='Foo', result_children=[ + ValueCheck(name='Base', children=[ + ValueCheck(name='m_base_val', value='42') + ]), + ValueCheck(name='m_derived_val', value='137') + ]) + + self.filecheck("target module dump ast", __file__) + + # The new definition 'struct Foo' is in the scratch AST + # CHECK: |-CXXRecordDecl {{.*}} struct Foo definition + # CHECK: | |-public 'Base' + # CHECK-NEXT: | `-FieldDecl {{.*}} m_derived_val 'int' + # CHECK-NEXT: `-CXXRecordDecl {{.*}} struct Base definition + + # ...but the original definition of 'struct Foo' is not in the scratch AST anymore + # CHECK-NOT: FieldDecl {{.*}} m_val 'int' diff --git a/lldb/test/API/functionalities/rerun_and_expr_dylib/lib.cpp b/lldb/test/API/functionalities/rerun_and_expr_dylib/lib.cpp new file mode 100644 index 0000000..aad9816 --- /dev/null +++ b/lldb/test/API/functionalities/rerun_and_expr_dylib/lib.cpp @@ -0,0 +1 @@ +LLDB_DYLIB_EXPORT struct Foo { int m_val = 42; } global_foo; diff --git a/lldb/test/API/functionalities/rerun_and_expr_dylib/main.cpp b/lldb/test/API/functionalities/rerun_and_expr_dylib/main.cpp new file mode 100644 index 0000000..144e8ba --- /dev/null +++ b/lldb/test/API/functionalities/rerun_and_expr_dylib/main.cpp @@ -0,0 +1,12 @@ +#include +#include + +extern struct Foo imported; + +int main() { + void *handle = dlopen("libfoo.dylib", RTLD_NOW); + struct Foo *foo = (struct Foo *)dlsym(handle, "global_foo"); + assert(foo != nullptr); + + return 0; +} diff --git a/lldb/test/API/functionalities/rerun_and_expr_dylib/rebuild.cpp b/lldb/test/API/functionalities/rerun_and_expr_dylib/rebuild.cpp new file mode 100644 index 0000000..b9d8351 --- /dev/null +++ b/lldb/test/API/functionalities/rerun_and_expr_dylib/rebuild.cpp @@ -0,0 +1,7 @@ +struct Base { + int m_base_val = 42; +}; + +LLDB_DYLIB_EXPORT struct Foo : public Base { + int m_derived_val = 137; +} global_foo; -- 2.7.4