Skip to content

Adding use-after-return in Lifetime Analysis #165370

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+329 −206
Open

Adding use-after-return in Lifetime Analysis #165370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ class Fact {
Use,
/// A marker for a specific point in the code, for testing.
TestPoint,
OriginEscapes,
// An origin that stores a loan escapes the function.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the comment should probably go before the enum constant and might need some word smithing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wording suggestion: /// Indicates that an origin escapes the function scope (e.g., via return).

};

private:
Expand Down Expand Up @@ -142,6 +144,23 @@ class ReturnOfOriginFact : public Fact {
const OriginManager &OM) const override;
};

class OriginEscapesFact : public Fact {
OriginID OID;
const Stmt *EscapeSource;

public:
static bool classof(const Fact *F) {
return F->getKind() == Kind::OriginEscapes;
}

OriginEscapesFact(OriginID OID, const Stmt *Source)
: Fact(Kind::OriginEscapes), OID(OID), EscapeSource(Source) {}
OriginID getEscapedOriginID() const { return OID; }
const Stmt *getEscapeSource() const { return EscapeSource; }
void dump(llvm::raw_ostream &OS, const LoanManager &,
const OriginManager &OM) const override;
};

class UseFact : public Fact {
const Expr *UseExpr;
// True if this use is a write operation (e.g., left-hand side of assignment).
Expand Down
1 change: 1 addition & 0 deletions clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ class FactsGenerator : public ConstStmtVisitor<FactsGenerator> {
FactManager &FactMgr;
AnalysisDeclContext &AC;
llvm::SmallVector<Fact *> CurrentBlockFacts;
llvm::SmallVector<Fact *> EscapesInCurrentBlock;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a line or two to explain the purpose.

// To distinguish between reads and writes for use-after-free checks, this map
// stores the `UseFact` for each `DeclRefExpr`. We initially identify all
// `DeclRefExpr`s as "read" uses. When an assignment is processed, the use
Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ class LifetimeSafetyReporter {
virtual void reportUseAfterFree(const Expr *IssueExpr, const Expr *UseExpr,
SourceLocation FreeLoc,
Confidence Confidence) {}

virtual void reportUseAfterReturn(const Expr *IssueExpr,
const Stmt *EscapeStmt,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it have to be a stmt ?
I would be more specific here const Expr *EscapedExpr,

SourceLocation ExpiryLoc,
Confidence Confidence) {}
};

/// The main entry point for the analysis.
Expand Down
1 change: 1 addition & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10730,6 +10730,7 @@ def warn_lifetime_safety_loan_expires_strict : Warning<
InGroup<LifetimeSafetyStrict>, DefaultIgnore;
def note_lifetime_safety_used_here : Note<"later used here">;
def note_lifetime_safety_destroyed_here : Note<"destroyed here">;
def note_lifetime_safety_returned_here : Note<"returned here">;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can have a separate warning for this.
def warn_lifetime_safety_return_stack_addr_permissive: "reference to stack address maybe returned later".
def warn_lifetime_safety_return_stack_addr_strict: "reference to stack address is returned later".


// For non-floating point, expressions of the form x == x or x != x
// should result in a warning, since these always evaluate to a constant.
Expand Down
77 changes: 74 additions & 3 deletions clang/lib/Analysis/LifetimeSafety/Checker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "clang/Analysis/AnalysisDeclContext.h"
#include "clang/Basic/SourceLocation.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/SmallSet.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/TimeProfiler.h"

Expand Down Expand Up @@ -61,10 +62,23 @@ class LifetimeChecker {
AnalysisDeclContext &ADC, LifetimeSafetyReporter *Reporter)
: LoanPropagation(LoanPropagation), LiveOrigins(LiveOrigins), FactMgr(FM),
Reporter(Reporter) {
for (const CFGBlock *B : *ADC.getAnalysis<PostOrderCFGView>())
for (const Fact *F : FactMgr.getFacts(B))
if (const auto *EF = F->getAs<ExpireFact>())
for (const CFGBlock *B : *ADC.getAnalysis<PostOrderCFGView>()) {
llvm::SmallVector<const ExpireFact *> BlockExpires;
llvm::SmallVector<const OriginEscapesFact *> BlockEscapes;
for (const Fact *F : FactMgr.getFacts(B)) {
if (const auto *EF = F->getAs<ExpireFact>()) {
checkExpiry(EF);
BlockExpires.push_back(EF);
} else if (const auto *OEF = F->getAs<OriginEscapesFact>()) {
BlockEscapes.push_back(OEF);
}
}
if (Reporter) {
for (const OriginEscapesFact *OEF : BlockEscapes) {
checkEscape(OEF, BlockExpires);
}
}
}
issuePendingWarnings();
}

Expand Down Expand Up @@ -106,6 +120,63 @@ class LifetimeChecker {
/*ConfidenceLevel=*/CurConfidence};
}

void checkEscape(const OriginEscapesFact *OEF,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we correctly use the liveness information, checkExpiry can deal with use-after-return as well.
Liveness can tell the if origin is live because of an OriginEscape, we can mark the loan with a use-after-return violation.

llvm::ArrayRef<const ExpireFact *> BlockExpires) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we catch the error if the returned loan expired in a different block?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems important to choose the diagnostic loc and even kind of diagnostic

std::string_view foo() {
  std::string_view res;
  {
    std::string small = "small scope";
    res = small;
  }
  return res;
}

I think for this we should have an existing use-after-scope (because res in return res is a "use" even before the return).

Also there could be cases where the loan is bad due to both use-after-scope and return-stack-addr

std::string_view foo() {
  std::string_view res;
  {
    std::string small = "small scope";
    res = small;
    if (cond) {
      return res;
    }
  }
  std::cout << res;
}

We would see two loan expiries here. One before return and other at end of scope. Both would be strict confidence (based on liveness). In this case we could just prefer the earliest gen responsible for liveness. In this case it would be the EscapeOrigin from return res;. So we would diagnose this as return-stack-addr.

On the other hand, following would be existing use-after-scope:

std::string_view foo() {
  std::string_view res;
  {
    std::string small = "small scope";
    res = small; // does not live long enough.
  }
  std::cout << res; // later used here.
  return res;
}


if (!Reporter) {
return;
}

OriginID returnedOID = OEF->getEscapedOriginID();
ProgramPoint PP = OEF;

LoanSet HeldLoans = LoanPropagation.getLoans(returnedOID, PP);
if (HeldLoans.isEmpty()) {
return;
}

llvm::SmallSet<LoanID, 4> ExpiredLoansInBlock;
llvm::DenseMap<LoanID, SourceLocation> ExpiryLocs;

for (const ExpireFact *EF : BlockExpires) {
ExpiredLoansInBlock.insert(EF->getLoanID());
ExpiryLocs[EF->getLoanID()] = EF->getExpiryLoc();
}

bool hasExpiredDependency = false;
bool allHeldLoansExpired = true;
LoanID exampleExpiredLoan = LoanID();

for (LoanID heldLoan : HeldLoans) {
if (ExpiredLoansInBlock.count(heldLoan)) {
hasExpiredDependency = true;
if (exampleExpiredLoan.Value == LoanID().Value) {
exampleExpiredLoan = heldLoan;
}
} else {
allHeldLoansExpired = false;
}
}

if (!hasExpiredDependency) {
return;
}

Confidence FinalConfidence;
if (allHeldLoansExpired) {
FinalConfidence = Confidence::Definite;
} else {
FinalConfidence = Confidence::Maybe;
}

const Loan &L = FactMgr.getLoanMgr().getLoan(exampleExpiredLoan);
SourceLocation ExpiryLoc = ExpiryLocs[exampleExpiredLoan];
const Stmt *EscapeSource = OEF->getEscapeSource();

Reporter->reportUseAfterReturn(L.IssueExpr, EscapeSource, ExpiryLoc,
FinalConfidence);
}

void issuePendingWarnings() {
if (!Reporter)
return;
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/Analysis/LifetimeSafety/Dataflow.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,8 @@ class DataflowAnalysis {
return D->transfer(In, *F->getAs<OriginFlowFact>());
case Fact::Kind::ReturnOfOrigin:
return D->transfer(In, *F->getAs<ReturnOfOriginFact>());
case Fact::Kind::OriginEscapes:
return D->transfer(In, *F->getAs<OriginEscapesFact>());
case Fact::Kind::Use:
return D->transfer(In, *F->getAs<UseFact>());
case Fact::Kind::TestPoint:
Expand All @@ -181,6 +183,7 @@ class DataflowAnalysis {
Lattice transfer(Lattice In, const ExpireFact &) { return In; }
Lattice transfer(Lattice In, const OriginFlowFact &) { return In; }
Lattice transfer(Lattice In, const ReturnOfOriginFact &) { return In; }
Lattice transfer(Lattice In, const OriginEscapesFact &) { return In; }
Lattice transfer(Lattice In, const UseFact &) { return In; }
Lattice transfer(Lattice In, const TestPointFact &) { return In; }
};
Expand Down
8 changes: 8 additions & 0 deletions clang/lib/Analysis/LifetimeSafety/Facts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,14 @@ void ReturnOfOriginFact::dump(llvm::raw_ostream &OS, const LoanManager &,
OS << ")\n";
}

void OriginEscapesFact::dump(llvm::raw_ostream &OS, const LoanManager &,
const OriginManager &OM) const {
OS << "OriginEscapes (";
OM.dump(getEscapedOriginID(), OS);
OS << ")\n";
}


void UseFact::dump(llvm::raw_ostream &OS, const LoanManager &,
const OriginManager &OM) const {
OS << "Use (";
Expand Down
6 changes: 5 additions & 1 deletion clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ void FactsGenerator::run() {
// initializations and destructions are processed in the correct sequence.
for (const CFGBlock *Block : *AC.getAnalysis<PostOrderCFGView>()) {
CurrentBlockFacts.clear();
EscapesInCurrentBlock.clear();
for (unsigned I = 0; I < Block->size(); ++I) {
const CFGElement &Element = Block->Elements[I];
if (std::optional<CFGStmt> CS = Element.getAs<CFGStmt>())
Expand All @@ -66,6 +67,8 @@ void FactsGenerator::run() {
Element.getAs<CFGAutomaticObjDtor>())
handleDestructor(*DtorOpt);
}
CurrentBlockFacts.append(EscapesInCurrentBlock.begin(),
EscapesInCurrentBlock.end());
FactMgr.addBlockFacts(Block, CurrentBlockFacts);
}
}
Expand Down Expand Up @@ -166,7 +169,8 @@ void FactsGenerator::VisitReturnStmt(const ReturnStmt *RS) {
if (const Expr *RetExpr = RS->getRetValue()) {
if (hasOrigin(RetExpr)) {
OriginID OID = FactMgr.getOriginMgr().getOrCreate(*RetExpr);
CurrentBlockFacts.push_back(FactMgr.createFact<ReturnOfOriginFact>(OID));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure this is intentional:
I do not think we have a need of ReturnOfOriginFact after this change so deleting this fact makes sense to me. If we go this way, we need to delete ReturnOfOriginFact definition altogether as well.

WDYT @Xazax-hun, I remember you had some thoughts about still keeping it around.

EscapesInCurrentBlock.push_back(
FactMgr.createFact<OriginEscapesFact>(OID, RS));
}
}
}
Expand Down
Loading
Loading