-
Notifications
You must be signed in to change notification settings - Fork 15.5k
[LifetimeSafety] Add support for tracking non-trivially destructed temporary objects #172007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,26 @@ static const PathLoan *createLoan(FactManager &FactMgr, | |
| return nullptr; | ||
| } | ||
|
|
||
| /// Creates a loan for the storage location of a temporary object. | ||
| /// \param MTW The MaterializeTemporaryExpr that represents the temporary | ||
| /// binding. \return The new Loan. | ||
| static const PathLoan *createLoan(FactManager &FactMgr, | ||
usx95 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const MaterializeTemporaryExpr *MTE) { | ||
| AccessPath Path(MTE); | ||
| return FactMgr.getLoanMgr().createLoan<PathLoan>(Path, MTE); | ||
| } | ||
|
|
||
| /// Try to find a CXXBindTemporaryExpr that descends from MTE, stripping away | ||
| /// any implicit casts. | ||
| /// \param MTE MaterializeTemporaryExpr whose descendants we are interested in. | ||
| /// \return Pointer to descendant CXXBindTemporaryExpr or nullptr when not | ||
| /// found. | ||
| static const CXXBindTemporaryExpr * | ||
| getChildBinding(const MaterializeTemporaryExpr *MTE) { | ||
| const Expr *Child = MTE->getSubExpr()->IgnoreImpCasts(); | ||
| return dyn_cast<CXXBindTemporaryExpr>(Child); | ||
| } | ||
|
|
||
| void FactsGenerator::run() { | ||
| llvm::TimeTraceScope TimeProfile("FactGenerator"); | ||
| const CFG &Cfg = *AC.getCFG(); | ||
|
|
@@ -60,6 +80,9 @@ void FactsGenerator::run() { | |
| else if (std::optional<CFGLifetimeEnds> LifetimeEnds = | ||
| Element.getAs<CFGLifetimeEnds>()) | ||
| handleLifetimeEnds(*LifetimeEnds); | ||
| else if (std::optional<CFGTemporaryDtor> TemporaryDtor = | ||
| Element.getAs<CFGTemporaryDtor>()) | ||
| handleTemporaryDtor(*TemporaryDtor); | ||
| } | ||
| CurrentBlockFacts.append(EscapesInCurrentBlock.begin(), | ||
| EscapesInCurrentBlock.end()); | ||
|
|
@@ -218,9 +241,14 @@ void FactsGenerator::VisitMaterializeTemporaryExpr( | |
| const MaterializeTemporaryExpr *MTE) { | ||
| if (!hasOrigin(MTE)) | ||
| return; | ||
| // A temporary object's origin is the same as the origin of the | ||
| // expression that initializes it. | ||
| killAndFlowOrigin(*MTE, *MTE->getSubExpr()); | ||
| if (getChildBinding(MTE)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would use
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mentioned lifetime extended temporaries offline. Is that reflected in the current iteration of the patch or do you need to update this?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is not reflected here, it will be in future iterations. For the future iterations, I believe my next PR would be in handling the case of non lifetime-extended general temporaries. I believe @usx95 suggested that I should choose to not track the MTE if I detect it is lifetime extended?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case this is not too hard to do, I'd prefer that bit to be included in this PR.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense will include it! |
||
| // Issue a loan to MTE for the storage location represented by MTE. | ||
| const Loan *L = createLoan(FactMgr, MTE); | ||
| OriginID OID = FactMgr.getOriginMgr().getOrCreate(*MTE); | ||
| CurrentBlockFacts.push_back(FactMgr.createFact<IssueFact>(L->getID(), OID)); | ||
| } else { | ||
| killAndFlowOrigin(*MTE, *MTE->getSubExpr()); | ||
usx95 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
|
||
| void FactsGenerator::handleLifetimeEnds(const CFGLifetimeEnds &LifetimeEnds) { | ||
|
|
@@ -233,13 +261,38 @@ void FactsGenerator::handleLifetimeEnds(const CFGLifetimeEnds &LifetimeEnds) { | |
| if (const auto *BL = dyn_cast<PathLoan>(Loan)) { | ||
| // Check if the loan is for a stack variable and if that variable | ||
| // is the one being destructed. | ||
| if (BL->getAccessPath().D == LifetimeEndsVD) | ||
| const AccessPath AP = BL->getAccessPath(); | ||
| const ValueDecl *Path = AP.getAsValueDecl(); | ||
| if (Path == LifetimeEndsVD) | ||
| CurrentBlockFacts.push_back(FactMgr.createFact<ExpireFact>( | ||
| BL->getID(), LifetimeEnds.getTriggerStmt()->getEndLoc())); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void FactsGenerator::handleTemporaryDtor( | ||
| const CFGTemporaryDtor &TemporaryDtor) { | ||
| const CXXBindTemporaryExpr *BTE = TemporaryDtor.getBindTemporaryExpr(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: rename to |
||
| if (!BTE) { | ||
| return; | ||
| } | ||
|
Comment on lines
+276
to
+278
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: remove the curly braces in a single-statement |
||
| // Iterate through all loans to see if any expire. | ||
| for (const auto *Loan : FactMgr.getLoanMgr().getLoans()) { | ||
| if (const auto *BL = dyn_cast<PathLoan>(Loan)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: rename to |
||
| // Check if the loan is for a temporary materialization and if that storage | ||
| // location is the one being destructed. | ||
| const AccessPath &AP = BL->getAccessPath(); | ||
| const MaterializeTemporaryExpr *Path = AP.getAsMaterializeTemporaryExpr(); | ||
| if (!Path) | ||
| continue; | ||
| if (BTE == getChildBinding(Path)) { | ||
| CurrentBlockFacts.push_back(FactMgr.createFact<ExpireFact>( | ||
| BL->getID(), TemporaryDtor.getBindTemporaryExpr()->getEndLoc())); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void FactsGenerator::handleGSLPointerConstruction(const CXXConstructExpr *CCE) { | ||
| assert(isGslPointerType(CCE->getType())); | ||
| if (CCE->getNumArgs() != 1) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,16 @@ namespace clang::lifetimes::internal { | |
|
|
||
| void PathLoan::dump(llvm::raw_ostream &OS) const { | ||
| OS << getID() << " (Path: "; | ||
| OS << Path.D->getNameAsString() << ")"; | ||
| if (const clang::ValueDecl *VD = Path.getAsValueDecl()) { | ||
| OS << VD->getNameAsString(); | ||
| } else if (const clang::MaterializeTemporaryExpr *MTE = | ||
| Path.getAsMaterializeTemporaryExpr()) { | ||
| // No nice "name" for the temporary, so deferring to LLVM default | ||
| OS << "MaterializeTemporaryExpr at " << MTE; | ||
| } else { | ||
| llvm_unreachable("access path is not one of any supported types"); | ||
| } | ||
| OS << ")"; | ||
|
Comment on lines
+15
to
+24
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: remove the |
||
| } | ||
|
|
||
| void PlaceholderLoan::dump(llvm::raw_ostream &OS) const { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,9 @@ class TriviallyDestructedClass { | |
| View a, b; | ||
| }; | ||
|
|
||
| MyObj temporary(); | ||
| void use(View); | ||
|
|
||
| //===----------------------------------------------------------------------===// | ||
| // Basic Definite Use-After-Free (-W...permissive) | ||
| // These are cases where the pointer is guaranteed to be dangling at the use site. | ||
|
|
@@ -943,3 +946,10 @@ void parentheses(bool cond) { | |
| } // expected-note 4 {{destroyed here}} | ||
| (void)*p; // expected-note 4 {{later used here}} | ||
| } | ||
|
|
||
| void use_temporary_after_destruction() { | ||
| View a; | ||
|
Comment on lines
+950
to
+951
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you try adding more tests. Like passing a temporary to a function with lifetimebound arg. Please also add tests which do not work at the moment with a FIXME (trivial temporaries, nested BTE). |
||
| a = temporary(); // expected-warning {{object whose reference is captured does not live long enough}} \ | ||
| expected-note {{destroyed here}} | ||
| use(a); // expected-note {{later used here}} | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo
MTE