NoQ added a comment.
Yay, i see the "Schrödinger handle" pattern: until the return value of release
is checked, the handle is believed to be both dead and alive at the same time.
We usually use this pattern because a lot of code that's already written
doesn't check their return values. But do you really need this for a brand-new
API? Maybe aggressively assume that the release may always fail and fix all the
places in your code where the return value is not checked before use?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:99-100
+
+static const StringRef HandleTypeName = "zx_handle_t";
+static const StringRef ErrorTypeName = "zx_status_t";
+
----------------
So you plan to unhardcode these as well eventually?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:123
+
+ // Only use this in checkDeadSymbols!
+ bool hasError(ProgramStateRef State) const {
----------------
For now it doesn't seem to be used at all.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:141-146
+#define CASE(ID)
\
+ case ID:
\
+ OS << #ID;
\
+ break;
+ CASE(Kind::Allocated)
+ CASE(Kind::Released)
----------------
This macro has literally saved exactly as many lines as it has caused :)
================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:156-168
+ std::unique_ptr<BugType> LeakBugType;
+ std::unique_ptr<BugType> DoubleReleaseBugType;
+ std::unique_ptr<BugType> UseAfterFreeBugType;
+
+public:
+ FuchsiaHandleChecker()
+ : LeakBugType(new BugType(this, "Fuchsia handle leak",
----------------
We've recently dumbed down this idiom to
```lang=c++
class FuchsiaHandleChecker : ... {
LeakBugType{this, "Fuchsia handle leak",
"Fuchsia Handle Error", /*SuppressOnSink=*/true};
...
};
```
:)
================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:207
+ int PtrToHandleLevel = 0;
+ const Type *T = QT.getTypePtr();
+ while (T->isAnyPointerType() || T->isReferenceType() || T->isArrayType()) {
----------------
This is never necessary, just call all the methods directly on `QT` - it has an
overloaded operator `->()` for this.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:208
+ const Type *T = QT.getTypePtr();
+ while (T->isAnyPointerType() || T->isReferenceType() || T->isArrayType()) {
+ ++PtrToHandleLevel;
----------------
Ok, so what's the deal with arrays here? If the function receives an array of
handles, do you ultimately plan to return multiple symbols - one for each
element of the array?
Also, in the generic non-Fuchsia case, what if the handle itself is a pointer?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:212
+ }
+ if (const auto *HandleType = dyn_cast<TypedefType>(T)) {
+ if (HandleType->getDecl()->getName() != HandleTypeName)
----------------
Here `QT->getAs<TypedefType>()` respectively.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:302-303
+ Pred);
+ // Avoid further reports on this symbol.
+ State = State->remove<HStateMap>(HandleSym);
+ } else
----------------
Why not generate a fatal error instead?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:336-338
+ProgramStateRef FuchsiaHandleChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+ bool Assumption) const {
----------------
Ok, so you're basically saying that whenever the code under analysis does
anything to check the return value, this callback will fire immediately? Thus
guaranteeing that whenever the Schrödinger state is accessed, it's either
already collapsed or indeed not checked yet?
This sounds right but i wish i had your confidence :D
My usual approach to Schrödinger states will be to do the same thing but in
`checkDeadSymbols()`. I.e., when the return value symbol dies, see if it's
constrained and collapse the state accordingly. If the access occurs earlier
than symbol death, update the state according to the current constraints and
forget about the symbol. If current constraints are insufficient,
conservatively mark it as a successful release (as that's how non-paranoid code
under analysis would behave).
Regardless of the approach, you still need to update the state upon access as
if the release has succeeded (when the current state is a Schrödinger state).
But with your approach you can avoid checking the constraints upon access, and
instead blindly assume that the symbol is underconstrained.
I like this! Can we prove that it works by adding an assertion that the return
value symbol is underconstrained upon every access to the potentially
Schrödinger state?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:343
+ SymbolRef ErrorSym = CurItem.second.getErrorSym();
+ if (Cmr.isNull(State, ErrorSym).isConstrainedFalse()) {
+ if (CurItem.second.isReleased())
----------------
If you want this check to be universally applicable, you will probably also
need an annotation that explains which statuses indicate an error.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:348
+ else
+ State = State->remove<HStateMap>(CurItem.first);
+ }
----------------
I request comments about this path. When does this happen?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:366
+ for (unsigned Arg = 0; Arg < Call->getNumArgs(); ++Arg) {
+ Optional<unsigned> ParamIdx = Call->getAdjustedParameterIndex(Arg);
+ if (!ParamIdx)
----------------
I strongly recommend testing this (i.e., by annotating an overloaded member
operator with the use-annotation). This code looks correct to me but it's just
too easy to make a mistake here :/
================
Comment at: clang/test/Analysis/fuchsia_handle.c:1
+// RUN: %clang_analyze_cc1 -analyzer-checker=fuchsia.FuchsiaHandleChecker
-verify %s
+
----------------
`core,`!
================
Comment at: clang/test/Analysis/fuchsia_handle.c:76
+
+void checkNoLeak07(int tag) {
+ zx_handle_t sa, sb;
----------------
This test doesn't really test the pointer escape mechanism because both handles
are closed correctly, so there's no leak in this test regardless of escapes.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70470/new/
https://reviews.llvm.org/D70470
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
