Skip to content
This repository has been archived by the owner on Jul 3, 2024. It is now read-only.

[Issue #70] Warn about the use of release on std::unique_ptr #79

Open
wants to merge 2 commits into
base: lifetime
Choose a base branch
from
Open

[Issue #70] Warn about the use of release on std::unique_ptr #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ad7d12
[Issue #70] Initial version detecting release on std::unique_ptr
lklein53 Nov 11, 2019
6b297de
Initial version of a test case
lklein53 Nov 11, 2019
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
10 changes: 10 additions & 0 deletions clang/lib/Analysis/LifetimePsetBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,16 @@ class PSetsBuilder : public ConstStmtVisitor<PSetsBuilder, void> {
if (!Callee)
return;

if (auto MCE = dyn_cast<CXXMemberCallExpr>(CallE)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is good for getting rid of the false positive but in the future I think it would be great to:

  • Update the state, so we know what is the pointee of the returned ptr (*uniqueptr in our case).
  • Set the unique_ptr to null. I am not sure if we support nullable owners properly at the point. The current paper also does not really elaborate on this.
  • Maybe we need a way to annotate ownership transferring functions in the future? Not all of them migh have the name release.

Some of these notes are more for our future selves designing a new version of the analysis, rather than the current implementation :)

const CXXRecordDecl *RD = MCE->getRecordDecl();
StringRef ClassName = RD->getName();
if (RD->isInStdNamespace() && ClassName.endswith("unique_ptr") &&
Callee->getName() == "release") {
// TODO: Print warning/note to suggest to not use release on std::unique_ptr
return;
}
}

/// Special case for assignment of Pointer into Pointer: copy pset
if (auto *OC = dyn_cast<CXXOperatorCallExpr>(CallE)) {
if (OC->getOperator() == OO_Equal && OC->getNumArgs() == 2 &&
Expand Down
27 changes: 27 additions & 0 deletions clang/test/Sema/warn-lifetime-analysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,3 +467,30 @@ void b(basic_string &c) {
c; // expected-warning {{expression result unused}}
}
} // namespace bug_report_66
namespace bug_report_70 {
namespace std {
template <class T>
class unique_ptr {
public:
explicit unique_ptr(T *t);
T *release();
};
} // namespace std
template <class T>
class [[gsl::Owner]] unique_ptr {
public:
explicit unique_ptr(T *t);
T *release();
};
// A random class that models a private key.
Copy link
Owner

Choose a reason for hiding this comment

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

Can we remove unrelated information? Same for all mentions of PrivateKey

class PrivateKey {
};

PrivateKey *getPrivateKeyStd() {
return std::unique_ptr<PrivateKey>(new PrivateKey()).release();
}
PrivateKey *getPrivateKeyNonStd() {
return unique_ptr<PrivateKey>(new PrivateKey()).release(); // expected-warning {{returning a dangling pointer}}
// expected-note@-1 {{temporary was destroyed at the end of the full expression}}
}
} // namespace bug_report_70