[PATCH] D81315: [analyzer] Warning for default constructed unique pointer dereferences
Nithin VR via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Jun 12 07:31:44 PDT 2020
vrnithinkumar marked 38 inline comments as done.
vrnithinkumar added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:31
namespace {
-class SmartPtrModeling : public Checker<eval::Call> {
+struct RegionState {
+private:
----------------
vsavchenko wrote:
> xazax.hun wrote:
> > I think `RegionState` is not very descriptive. I'd call it something like `RegionNullness`.
> linter: LLVM coding standards require to use `class` keyword in situations like this (https://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords). I would even say that `struct` is good for POD types.
With the new changes, the struct is removed.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:31
namespace {
-class SmartPtrModeling : public Checker<eval::Call> {
+struct RegionState {
+private:
----------------
vrnithinkumar wrote:
> vsavchenko wrote:
> > xazax.hun wrote:
> > > I think `RegionState` is not very descriptive. I'd call it something like `RegionNullness`.
> > linter: LLVM coding standards require to use `class` keyword in situations like this (https://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords). I would even say that `struct` is good for POD types.
> With the new changes, the struct is removed.
With the new changes, the struct is removed.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:33
+private:
+ enum Kind { Null, NonNull, Unknown } K;
+ RegionState(Kind InK) : K(InK) {}
----------------
vsavchenko wrote:
> I think that it would be better to put declarations for `enum` and for a field separately.
> Additionally, I don't think that `K` is a very good name for a data member. It should be evident from the name of the member what is it. Shot names like that can be fine only for iterators or for certain `clang`-specific structures because of existing traditions (like `SM` for `SourceManager` and `LO` for `LanguageOptions`).
With the new changes, the `enum` is removed.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:46
+};
+} // end of anonymous namespace
+
----------------
Szelethus wrote:
> xazax.hun wrote:
> > You can merge the two anonymous namespaces, this and the one below.
> [[https://llvm.org/docs/CodingStandards.html#anonymous-namespaces | I prefer them like this. ]]
with new change only one anonymous namespace is there
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:60
+private:
+ mutable std::unique_ptr<BugType> BT;
+ void reportBug(CheckerContext &C, const CallEvent &Call) const;
----------------
xazax.hun wrote:
> This is how we used to do it, but we did not update all the checks yet. Nowadays we can just initialize bugtypes immediately, see https://github.com/llvm/llvm-project/blob/master/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp#L169
updated to initialize bugtype immediately
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:65
+
+ // STL smart pointers which we are trying to model
+ const llvm::StringSet<> StdSmartPtrs = {
----------------
xazax.hun wrote:
> In LLVM we aim for full sentences as comments with a period at the end.
Updated the comments as LLVM style
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:72-76
+ // STL smart pointer methods which resets to null
+ const llvm::StringSet<> ResetMethods = {"reset", "release", "swap"};
+
+ // STL smart pointer methods which resets to null via null argument
+ const llvm::StringSet<> NullResetMethods = {"reset", "swap"};
----------------
NoQ wrote:
> Please consider `CallDescription` and `CallDescriptionMap`!
Made changes to use `CallDescription` and `CallDescriptionMap`
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:81
+// to track the mem region and curresponding states
+REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, RegionState)
+// to track the Symbols which will get inner raw pointer via unique_ptr.get()
----------------
NoQ wrote:
> I ultimately believe this map should go away. The only thing we really need is the map from smart pointer region to the symbol for its current raw pointer. As long as we have such data we can discover the nullness of that symbol (which *is* the nullness of the smart pointer as well) from Range Constraints.
Removed this map.
Now maping region to SVal
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:119
+ ProgramStateRef State = C.getState();
+ const auto OC = dyn_cast<CXXMemberOperatorCall>(&Call);
+ if (!OC)
----------------
vsavchenko wrote:
> xazax.hun wrote:
> > Here the const applies for the pointer, not the pointee. We usually do `const auto *OC` instead.
> As I said above, I think we should be really careful about abbreviated and extremely short variable names. Longer names make it much easier to read the code without paying a lot of attention to declarations.
fixed
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:119
+ ProgramStateRef State = C.getState();
+ const auto OC = dyn_cast<CXXMemberOperatorCall>(&Call);
+ if (!OC)
----------------
vrnithinkumar wrote:
> vsavchenko wrote:
> > xazax.hun wrote:
> > > Here the const applies for the pointer, not the pointee. We usually do `const auto *OC` instead.
> > As I said above, I think we should be really careful about abbreviated and extremely short variable names. Longer names make it much easier to read the code without paying a lot of attention to declarations.
> fixed
Okay.
Will try to use longer names in future.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:150
+ const CXXRecordDecl *RD = CtorDec->getParent();
+ if (isSmartPointer(RD)) {
+ State =
----------------
xazax.hun wrote:
> I wonder if you wanted to add `isSmartPointer` checks below as well. In that case you can hoist this check and early return for non-smart-pointers.
Added the check for early return
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:160-187
+ if (const auto IC = dyn_cast<CXXInstanceCall>(&Call)) {
+ const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(IC->getDecl());
+ if (!MethodDecl)
+ return;
+ auto ArgsNum = IC->getNumArgs();
+
+ if (ArgsNum == 0 && isResetMethod(MethodDecl)) {
----------------
vsavchenko wrote:
> Considering the fact that more cases and more functions will get supported in the future, I vote for merging common parts of these two blocks.
Merged
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:195
+ // Clean up dead regions from the region map.
+ TrackedRegionMapTy TrackedRegions = State->get<TrackedRegionMap>();
+ for (auto E : TrackedRegions) {
----------------
xazax.hun wrote:
> You probably also want to clean up the `SymbolRegionMap`. It is ok to not do it right now, but we usually tend to add `FIXMEs` or `TODOs` early and aggressively to make sure we do not forget stuff.
This map is removed in the new changes
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:213-214
+
+ if (!BT)
+ BT.reset(new BugType(this, "unique_ptr", "Dont call unique_ptr"));
+ auto R = std::make_unique<PathSensitiveBugReport>(
----------------
NoQ wrote:
> These days we don't bother with lazy initialization, the usual syntax is:
> ```lang=c++
> class YourChecker {
> // ...
> BugType BT { this, "unique_ptr", "Dont call unique_ptr" };
> // ...
> };
> ```
updated to initialize bugtype immediately
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:225
+ if (RecordDec->getDeclName().isIdentifier()) {
+ return StdSmartPtrs.count(RecordDec->getName().lower());
+ }
----------------
xazax.hun wrote:
> This looks good for now. But we sometimes cache the pointer to identifier info objects so after the initial lookup we can just do pointer comparison instead of more expensive operations. Also add a fixme to check for the `std` namespace.
>
> Also, this method could even be promoted to a free functions making the list of SmarPtrs global.
Added std namespace check
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:233
+ return false;
+ if (MethodDec->getDeclName().isIdentifier()) {
+ return ResetMethods.count(MethodDec->getName().lower());
----------------
NoQ wrote:
> vsavchenko wrote:
> > I'm not sure about it myself, but can `DeclName` be `isEmpty()`? If yes, it is a potential null-pointer dereference. Again, I don't know it for a fact, but I think it should be checked.
> NOTE: 👏 Call 👏 Description 👏 Map 👏
Changed to use `CallDescription`
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:233
+ return false;
+ if (MethodDec->getDeclName().isIdentifier()) {
+ return ResetMethods.count(MethodDec->getName().lower());
----------------
vrnithinkumar wrote:
> NoQ wrote:
> > vsavchenko wrote:
> > > I'm not sure about it myself, but can `DeclName` be `isEmpty()`? If yes, it is a potential null-pointer dereference. Again, I don't know it for a fact, but I think it should be checked.
> > NOTE: 👏 Call 👏 Description 👏 Map 👏
> Changed to use `CallDescription`
Changed to use `CallDescription`
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:233
+ return false;
+ if (MethodDec->getDeclName().isIdentifier()) {
+ return ResetMethods.count(MethodDec->getName().lower());
----------------
vrnithinkumar wrote:
> vrnithinkumar wrote:
> > NoQ wrote:
> > > vsavchenko wrote:
> > > > I'm not sure about it myself, but can `DeclName` be `isEmpty()`? If yes, it is a potential null-pointer dereference. Again, I don't know it for a fact, but I think it should be checked.
> > > NOTE: 👏 Call 👏 Description 👏 Map 👏
> > Changed to use `CallDescription`
> Changed to use `CallDescription`
Changed to use CallDescription
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81315/new/
https://reviews.llvm.org/D81315
More information about the cfe-commits
mailing list