[analyzer] Warning for default constructed unique pointer dereferences

[vrnithinkumar]

Summary

This is just a prototype review for my changes since I thought it will be easier to ask code related doubts here.

I just wanted to share how I prototyped the checker for default constructed unique pointer dereferences.
It is incomplete. Not added tests. Not all cases covered. Reporting part is not proper. This may be a throw away code.

I am sharing this so that if I am fundamentally wrong in any of my directions it will be much better to catch early and rectify.

• Added a two maps to track mem region and corresponding states and Symbols to mem region
• Created a RegionState to track information about the state of memory region whether it is null or not or unknown
• Using PostCall to update the states of mem region
• Using PreCall to check the null pointer dereferences

Few doubts:
I am not sure about whether I should use eval::Call or both check::PreCall and check::PostCall.
In the eval::Call documentation I found this "Note, that only one checker can evaluate a call.". So I am little bit confused about using it.

Using one map for tracking the mem region and states then one more for Symbols to region to track which all Symbol has the inner pointer.
I am just looking is there any better approach for this.

[xazax.hun]

Hi!

Please add the [analyzer] tag in front of your patches as some folks have automated scripts based on that tag to add themselves as subscriber/reviewer.

A small debugging/productivity tip, if you add a printState method to your checker like in https://github.com/llvm/llvm-project/blob/master/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp#L573 , you will be able to see the checker state in the exploded graph dumps.

See some other comments inline.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

32–33

I think RegionState is not very descriptive. I'd call it something like RegionNullness.

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

45

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

47

You can merge the two anonymous namespaces, this and the one below.

50

In LLVM we aim for full sentences as comments with a period at the end.

226

Here the const applies for the pointer, not the pointee. We usually do const auto *OC instead.

241

In case we did not report a bug, we could assume that the pointer is not-null and we could update the state accordingly. This is a common approach the analyzer takes. E.g. when we se a division by X, we either report a bug or add an assumption that X is non-zero.

257

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.

302

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.

332

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.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

57–61

Please consider CallDescription and CallDescriptionMap!

69

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.

320–321

These days we don't bother with lazy initialization, the usual syntax is:

class YourChecker {
  // ...
  BugType BT { this, "unique_ptr", "Dont call unique_ptr" };
  // ...
};

[Szelethus]

Best of luck on your GSoC! I don't have much else to add to your patch, but you seem to have made good progress already!

In D81315#2078043, @xazax.hun wrote:

some folks have automated scripts based on that tag to add themselves as subscriber/reviewer.

Hope you don't mind my intrusion :)

I am not sure about whether I should use eval::Call or both check::PreCall and check::PostCall. In the eval::Call documentation I found this "Note, that only one checker can evaluate a call.". So I am little bit confused about using it.

Inlining (when we model a function call, https://youtu.be/yh2qdnJjizE?t=238) is rather expensive. Creating a new stack frame, parameters, new ExplodedNodes, running checkers, etc., eats memory for breakfast, is slow and limits how deep the analysis can go. Worse still, the analysis could lose precision if the called function's definition isn't available. eval::Call serves to circumvent this by allowing the analyzer to give a precise summary of the function. StreamChecker, for instance, uses this for functions such as clearerr() -- the C standard defines how this function should behave, so upon encountering a call to it, we don't need all the extra work regular inlining demands, just ask StreamChecker to model it for us.

Use eval::Call if you can provide a precise model for a function. Only a single checker is allowed to do that -- you can see that it returns with a boolean value to sign whether the checker could provide an evaluation, and as far as I know, the first checker that returns true will be doing it.

I think it would be appropriate in this instance, because we're modeling a well established API. In general, I think we should use it when appropriate more often, like in MallocChecker.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

47

I prefer them like this.

[vsavchenko]

Great start!
I think you are on the right track, so maybe this code won't be thrown away at all :-)
Try to work on tests and get familiar with lit.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

32–33

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.

34

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).

226

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.

267–294

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.

337

I believe that methods (and data members related to them) can be static.

340

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.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

340

NOTE: 👏 Call 👏 Description 👏 Map 👏

[vrnithinkumar]

Addressing the review comments

[vrnithinkumar]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

32–33

With the new changes, the struct is removed.

32–33

With the new changes, the struct is removed.

34

With the new changes, the enum is removed.

45

updated to initialize bugtype immediately

47

with new change only one anonymous namespace is there

50

Updated the comments as LLVM style

57–61

Made changes to use CallDescription and CallDescriptionMap

69

Removed this map.
Now maping region to SVal

226

fixed

226

Okay.
Will try to use longer names in future.

257

Added the check for early return

267–294

Merged

320–321

updated to initialize bugtype immediately

340

Changed to use CallDescription

340

Changed to use CallDescription

340

Changed to use CallDescription

[vrnithinkumar]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

302

This map is removed in the new changes

332

Added std namespace check

[NoQ]

Hey, that's a lot of progress already! =)

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

124

Not all constructors behave this way. In particular, if it's a copy/move constructor this function would track the value of the original smart pointer to this smart pointer, but what we need is to track the value of the raw pointer instead.

175

When you're doing evalCall, you're responsible for all aspects of the call - not just your private GDM map but also the Store and the Environment.

For .reset() the other important thing to do would be to invalidate the region in the Store so that the Store did not think that it contains the old value. We can't set the new value correctly but invalidating it would still be better than doing nothing - simply because "not being sure" is not as bad as "being confidently incorrect". That said, once you model *all* methods of the smart pointers, you would probably no longer need to invalidate the Store, because it will never be actually accessed during analysis. So my conclusion here is:

• For this first patch invalidating the Store is probably not worth it but let's add a FIXME.
• We should make sure that we eventually add the invalidation if we don't have time to model all methods.

Now, you're calling this same function for .release() as well. And for .release() there is another important thing that we're responsible for: model the return value. The .release() method returns the raw pointer - which is something this checker is accidentally an expert on! - so let's BindExpr() the value of the call-expression to the value of the raw pointer. If you want to delay this exercise until later patches, i recommend to temporarily modeling the return value as a conjured symbol instead, but we cannot completely drop this part.

Another useful thing that we should do with .release() in the future is to tell MallocChecker to start tracking the raw pointer. This will allow us to emit memory leak warnings against it. Let's add a TODO for that as well.

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

35

Nit: We do not use hypens/dashes in diagnostic names.

53

It might be just a personal preference but I tend to put these at the top of the file for easier discoverability. Feel free to ignore this comment unless other reviewers have the same preference as me.

81

Don't we want this to be also protected by the isStdSmartPointerClass check?

115

I wonder if making isStdSmartPointerClass operate on CallEvent would simllify the call sites of this function.

137

In LLVM we often omit braces for small single statement branches. Also, Artem mentioned that we could interact with the malloc checker. Maybe it is worth considering to notify the malloc checker to mark the appropriate region as deallocated? This would help find double release errors, avoid spurious leak errors and so on.

155

Early returns can decrease the indentation.

213

I wonder if we should use makeNullWithType. @NoQ what do you think? I am always confused when should we prefer one over the other.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

124

Actually, let's add an assertion into updateTrackedRegion that the value that's put into the map is of pointer type. This would give us an automatic notification when we make such mistakes.

137

Maybe it is worth considering to notify the malloc checker to mark the appropriate region as deallocated?

This happens in destructor.

213

I think makeNull() produces a null pointer (as opposed to numeric zero produced by makeZeroVal(Ty)) and there's not much choice when it comes to types of pointers.

Like, i mean, there were a few attempts to add support for architectures in which pointers come in different sizes but i don't think we care about that situation yet.

[vrnithinkumar]

Addressing review comments

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

35

Fixed the format

53

Thanks!
Moved to top.

81

added isStdSmartPointerClass check in the beginning.

115

Yeah. Thanks!
That makes it better.
Made changes to pass CallEvent to isStdSmartPointerClass

124

• Added check to filter out copy/move constructor
• Added assert to check the value is of type pointer.

137

removed the braces

155

Updated with early return.
Thanks!

175

• Added TODO for reset()
• Added BindExpr() for the value of the call-expression to the value of the raw pointer.
• Added TODO for release() to track raw pointer in future.

[vrnithinkumar]

In D81315#2079457, @Szelethus wrote:

Best of luck on your GSoC! I don't have much else to add to your patch, but you seem to have made good progress already!

Thanks!

In D81315#2078043, @xazax.hun wrote:

some folks have automated scripts based on that tag to add themselves as subscriber/reviewer.

Hope you don't mind my intrusion :)

Not at all.

I am not sure about whether I should use eval::Call or both check::PreCall and check::PostCall. In the eval::Call documentation I found this "Note, that only one checker can evaluate a call.". So I am little bit confused about using it.

Inlining (when we model a function call, https://youtu.be/yh2qdnJjizE?t=238) is rather expensive. Creating a new stack frame, parameters, new ExplodedNodes, running checkers, etc., eats memory for breakfast, is slow and limits how deep the analysis can go. Worse still, the analysis could lose precision if the called function's definition isn't available. eval::Call serves to circumvent this by allowing the analyzer to give a precise summary of the function. StreamChecker, for instance, uses this for functions such as clearerr() -- the C standard defines how this function should behave, so upon encountering a call to it, we don't need all the extra work regular inlining demands, just ask StreamChecker to model it for us.

Use eval::Call if you can provide a precise model for a function. Only a single checker is allowed to do that -- you can see that it returns with a boolean value to sign whether the checker could provide an evaluation, and as far as I know, the first checker that returns true will be doing it.

I think it would be appropriate in this instance, because we're modeling a well established API. In general, I think we should use it when appropriate more often, like in MallocChecker.

Thank you for the detailed help.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

219–220

By returning "true" here you're saying that operators * and -> do nothing. That's not quite right; they return a value. You should not return true from evalCall unless you fully model the call.

I wouldn't mind moving precondition checks such as this one into checkPreCall, even if you do evalCall later. That would clearly separate concerns and avoid the need to figure out whether we still need to model the call after the bug-reporting routine finishes (the engine will do that for you automatically).

In fact i wouldn't mind moving all bug reporting functionality into a separate checker. But that's a story for another time^^

[vrnithinkumar]

Moving dereference precondition checks into checkPreCall.

[vrnithinkumar]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

219–220

By returning "true" here you're saying that operators * and -> do nothing. That's not quite right; they return a value. You should not return true from evalCall unless you fully model the call.

Thanks!
I completely missed this point.

I wouldn't mind moving precondition checks such as this one into checkPreCall, even if you do evalCall later. That would clearly separate concerns and avoid the need to figure out whether we still need to model the call after the bug-reporting routine finishes (the engine will do that for you automatically).

Made changes to move dereference precondition checks into checkPreCall

In fact i wouldn't mind moving all bug reporting functionality into a separate checker. But that's a story for another time^^

Cool!
I will keep this in mind.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

127

This doesn't seem to be guarded by ShouldCheckSmartPtrDereference.

Consider adding a LIT test for the flag to make sure we're not leaking our project to the public until it's ready ^.^ Say, you could make two run-lines in your test like this:

// RUN: %clang_analyze_cc1 -verify=expected -analyzer-config cplusplus.SmartPtr:CheckSmartPtrDereference=true \
// RUN: ...
// RUN: %clang_analyze_cc1 -verify=unexpected \
// RUN: ...

// unexpected-no-diagnostics

(see https://clang.llvm.org/doxygen/classclang_1_1VerifyDiagnosticConsumer.html for more fancy tricks with -verify)

[Szelethus]

I have a few pending patches that enforce some long desired restrictions on which checkers can emit diagnostics, and I'd prefer not to mess with your checker after you land this :/

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

577–583 ↗

(On Diff #273841)

This goes against D81750 -- Sorry for not bringing this up earlier, but you can't emit diagnostics from a checker that is hidden :/

The proper solution would be to create a non-hidden checker, and emit a diagnostic under its name. You can take inspiration from MallocChecker, NullabilityChecker, and a variety of other sound in the stack of this patch: D77845

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

118

You should never get null here due to isStdSmartPointerClass guarding above. I think the null check could be transformed into an assert.

120

You have a CXXMemberOperatorCall which already represents an overloaded operator. This check is either redundant or could be an assert instead.

123

In case CXXMemberOperatorCall class does not have a way to query the overloaded operator kind maybe it would be worth to add a helper method to that class. So you can do most of the checking using one interface only.

188

It is possible that both updateTrackedRegion and this adds a transition. Both transition will have the same predecessor, i.e. you introduce a split in the exploded graph. Is this intended?

[NoQ]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

577–583 ↗

(On Diff #273841)

Aha, ok, that's what i meant at the end of D81315#inline-760088. @Szelethus insists, so let's turn ourselves into a separate checker right away!

In fact, let's *not* take the inspiration from poor-man's solutions in MallocChecker etc., but instead let's try a full-on modular approach:

• Make a separate .cpp file and a separate Checker class for emitting diagnostics.
  • Remove the checker option and instead add a separate checker entry in Checkers.td.
• Subscribe the new checker on PreCall only and move all the bug reporting logic there.
• Keep all the modeling logic in the old checker (it's called SmartPtrModeling for a reason!).
• Put common functionality into a header file shared between the two checkers.
  • In particular, the GDM trait should be only accessed through such getter.
    • Take some inspiration from Taint.h.

[Szelethus]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

577–583 ↗

(On Diff #273841)

Sounds great! And of course, say the word if any help is needed! :)

[vrnithinkumar]

• Created a new checker for smart point derference diagnostic
• Moved checking part to this checker
• Kept all the modeling in SmartPtrModeling
• Created a header file for shared API -calls

[vrnithinkumar]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

577–583 ↗

(On Diff #273841)

Thanks...!

Created a new checker alpha.cplusplus.SmartPtr to emit the SmartPtr dereference diagnostic.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

118

Changed to dyn_cast.
Added assert

120

Removed the redundant check

123

Added getOverloadedOperator() in CXXMemberOperatorCall class

127

Added one more run to verify it.

188

This was not intended.

• Fixed to avoid split in the exploded graph.
• Updated test to check with clang_analyzer_numTimesReached

[vrnithinkumar]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

574 ↗

(On Diff #275076)

Changed the modeling to SmartPtrModeling and named checker to SmartPtr

579 ↗

(On Diff #275076)

Using this flag to enable and disable modeling of SmartPtr null dereferences

[Szelethus]

I only looked at the checker naming issue, and that seems to have been resolved perfectly, thanks! :)

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

582 ↗

(On Diff #275076)

Let's Hide this in addition.

clang/test/Analysis/smart-ptr.cpp

5

alpha.cplusplus.SmartPtr on its own should be sufficient, dependencies ensure that cplusplus.SmartPtrModeling will be enabled and registered beforehand.

[vrnithinkumar]

Addressing review comments

[vrnithinkumar]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

582 ↗

(On Diff #275076)

Added Hide

clang/test/Analysis/smart-ptr.cpp

5

Removed cplusplus.SmartPtrModeling

[NoQ]

This looks good at a glance to me! I only have minor nits left.

It looks like we still need the option to be in place even though we moved the warnings into a separate checker, so that not to accidentally behave worse than with conservative modeling, but it becomes much less important.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

792–794 ↗

(On Diff #275076)

Nice!

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp

41 ↗

(On Diff #275254)

Looks like dead code.

72 ↗

(On Diff #275254)

This is probably unnecessary. The default SourceRange highlighted in path-sensitive report is the error node's statement and it should be exactly the same.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

49

Looks like dead code.

74–87

Ok, so the normal solution to this problem is to make this logic a part of your CallDescriptionMap:

CallDescriptionMap<SmartPtrMethodHandlerFn> SmartPtrMethodHandlers{

  {{"std", "unique_ptr", "reset"}, &SmartPtrModeling::handleReset},
  {{"std", "unique_ptr", "release"}, &SmartPtrModeling::handleRelease},
  {{"std", "unique_ptr", "swap", 1}, &SmartPtrModeling::handleSwap},

  {{"std", "shared_ptr", "reset"}, &SmartPtrModeling::handleReset},
  {{"std", "shared_ptr", "release"}, &SmartPtrModeling::handleRelease},
  {{"std", "shared_ptr", "swap", 1}, &SmartPtrModeling::handleSwap},

  {{"std", "weak_ptr", "reset"}, &SmartPtrModeling::handleReset},
  {{"std", "weak_ptr", "release"}, &SmartPtrModeling::handleRelease},
  {{"std", "weak_ptr", "swap", 1}, &SmartPtrModeling::handleSwap},
};

It looks like you're not doing this because CallDescriptionMap doesn't support operator calls yet. In fact, it looks like it doesn't support them because i'm not doing my homework by reviewing D81059. I just tried to catch up on it.

The other potential reason not to use CallDescriptionMap would be that you'll have to duplicate the list of methods for every smart pointer class you want to support. I don't think it's necessarily bad though, because different classes may in fact require different handling.

The downside of your solution, though, is that you're manually implementing the name matching logic that has been already implemented for you in CallDescriptionMap. And the reason we made CallDescriptionMap was because we really really wanted to re-use this logic because it's surprisingly difficult to implement correctly. One potential problem i see with your implementation is that you don't account for inline namespaces such as [[ https://github.com/llvm/llvm-project/blob/master/clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp#L127 | libcxx's __1 ]]. CallDescriptionMap knows about these things but they're almost impossible to discover independently when you're matching names by hand.

Let's leave this code as-is for now but try to get rid of this function as soon as possible (i.e., when D81059 lands).

81

Uh-oh, i'm shocked this wasn't in place before. Maybe we should do some code review or something. What idiot wrote this code anyway? Wait, it was me.

clang/test/Analysis/smart-ptr.cpp

4

This second run-line no longer tests the option. The checker is disabled, of course there won't be any diagnostics. I think we should remove the second run-line now and i don't have much better tests in mind that we won't eventually remove as we write more code.

[vrnithinkumar]

addressing review comments

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp

41 ↗

(On Diff #275254)

Thanks!
removed.

72 ↗

(On Diff #275254)

Removed.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

49

Thanks!
removed.

74–87

Thanks for the details.
If I recall correctly, I also found that CallDescriptionMap was not supporting constructors also.
I am not sure why.
Is it because constructor name is a special name?

81

:)

clang/test/Analysis/smart-ptr.cpp

4

Okay thanks
removed the second line.

[NoQ]

This looks great to me as long as other reviewers are happy. @vrnithinkumar, I think you should ask for commit access and honorably push the patch to master yourself ^.^

I guess we'll talk about this more, but i think the really good next step would be to implenent the checkRegionChanges callback for the checker so that to destroy the tracked raw pointer data whenever something unknown happens to the smart pointer. We'll need this anyway, i.e. when a pointer is passed by a non-const reference into a function, we no longer know the raw pointer value. But as a side effect, it'll also destroy tracked data on any smart pointer method that's not modeled yet. That'll allow the modeling checker to behave correctly (i.e., not perfectly but avoid being confidently incorrect) even if not all methods are modeled. That, in turn, will allow us to remove the option and enable the checker by default even without fully implementing all methods.

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp

32–33 ↗

(On Diff #275434)

Those strings are user-facing. We need to not forget to come up with a more humane explanation of the bug.

[Szelethus]

LGTM! You packed a lot of punch into this patch, and I like it very much. I read the code and everything looks great. I nitpicked on one thing, the updateTrackedRegion function is a bit awkward, but if you place a TODO there, I'm happy to have that addressed in a later patch. I don't think this is reason to postpone the landing of this any further.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

266–283

Hmm, this function feels clunky. So, if the call has no arguments, we set the smart pointer to null, otherwise if its a single-argument then we set it to whatever the argument is?

How about operator[], that also takes a single argument, but isn't a memory region? get, get_deleter don't take any arguments, but they don't set the internal pointee to null either. The name updateTrackedRegion however suggests that whatever operation was done, this is the one-tool-to-solve-it-all function to take care of it.

I think this function handles too many things as once, and the name and lack of documentation obfuscates its purpose. How about we put the relevant code to handleRelease, and repurpose the rest of the function like this:

updateOwnedRegion(CallEvent, CheckerContext, MemRegion of the smart pointer, MemRegion to take ownership of)

What do you think?

[NoQ]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

266–283

Yup, I completely agree. I think this structure will naturally evolve into something cleaner once more modeling gets added.

[vrnithinkumar]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

266–283

Thanks for the detailed comment.

I totally agree this method is handling many things.
For methods like get, get_deleter this makes no sense at all.

I will add TODO there, will address this in a later patch once we get a clear picture after more modeling added.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

 // SmartPtrModeling.cpp - Model behavior of C++ smart pointers - C++ ------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines a checker that models various aspects of
 // C++ smart pointer behavior.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Move.h"
 
+#include "clang/AST/DeclCXX.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/Type.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
-class SmartPtrModeling : public Checker<eval::Call> {
+class SmartPtrModeling
+    : public Checker<eval::Call, check::PostCall, check::DeadSymbols> {
   bool isNullAfterMoveMethod(const CallEvent &Call) const;
+  BugType NullDereferenceBugType{this, "Null-smartPtr-deref",
+                                 "C++ smart pointer"};
 
 public:
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+  void checkPostCall(const CallEvent &MC, CheckerContext &C) const;
+  void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
+
+private:
+  void reportBug(CheckerContext &C, const CallEvent &Call) const;
+  bool isStdSmartPointerClass(const CXXRecordDecl *RD) const;
+  void updateTrackedRegion(const CallEvent &Call, CheckerContext &C,
+                           const MemRegion *Region) const;
+  void handleReset(const CallEvent &Call, CheckerContext &C) const;
+  void handleSwap(const CallEvent &Call, CheckerContext &C) const;
+  void checkDeferenceOps(const CallEvent &Call, CheckerContext &C) const;
+
+  // Set of STL smart pointer class which we are trying to model.
+  const llvm::StringSet<> StdSmartPtrs = {
+      "shared_ptr",
+      "unique_ptr",
+      "weak_ptr",
+  };
+
+  using SmartPtrMethodHandlerFn =
+      void (SmartPtrModeling::*)(const CallEvent &Call, CheckerContext &) const;
+  CallDescriptionMap<SmartPtrMethodHandlerFn> SmartPtrMethodHandlers{
+      {{"reset"}, &SmartPtrModeling::handleReset},
+      {{"release"}, &SmartPtrModeling::handleReset},
+      {{"swap", 1}, &SmartPtrModeling::handleSwap}};
 };
 } // end of anonymous namespace
 
+REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, SVal)
+
 bool SmartPtrModeling::isNullAfterMoveMethod(const CallEvent &Call) const {
   // TODO: Update CallDescription to support anonymous calls?
   // TODO: Handle other methods, such as .get() or .release().
   // But once we do, we'd need a visitor to explain null dereferences
   // that are found via such modeling.
   const auto *CD = dyn_cast_or_null<CXXConversionDecl>(Call.getDecl());
   return CD && CD->getConversionType()->isBooleanType();
 }
 
 bool SmartPtrModeling::evalCall(const CallEvent &Call,
                                 CheckerContext &C) const {
-  if (!isNullAfterMoveMethod(Call))
-    return false;
-
+  if (isNullAfterMoveMethod(Call)) {
     ProgramStateRef State = C.getState();
     const MemRegion *ThisR =
         cast<CXXInstanceCall>(&Call)->getCXXThisVal().getAsRegion();
 
     if (!move::isMovedFrom(State, ThisR)) {
       // TODO: Model this case as well. At least, avoid invalidation of globals.
       return false;
     }
 
     // TODO: Add a note to bug reports describing this decision.
     C.addTransition(
         State->BindExpr(Call.getOriginExpr(), C.getLocationContext(),
                         C.getSValBuilder().makeZeroVal(Call.getResultType())));
     return true;
   }
 
+  const auto *MethodDecl = dyn_cast<CXXMethodDecl>(Call.getDecl());
+  if (!MethodDecl || !isStdSmartPointerClass(MethodDecl->getParent()))
+    return false;
+
+  checkDeferenceOps(Call, C);
+
+  const SmartPtrMethodHandlerFn *Handler = SmartPtrMethodHandlers.lookup(Call);
+  if (!Handler)
+    return false;
+  (this->**Handler)(Call, C);
+
+  return C.isDifferent();
+}
+
+void SmartPtrModeling::checkPostCall(const CallEvent &Call,
+                                     CheckerContext &C) const {
+  const auto *MethodDecl = dyn_cast<CXXMethodDecl>(Call.getDecl());
+  if (!MethodDecl || !isStdSmartPointerClass(MethodDecl->getParent()))
+    return;
+
+  const auto *CC = dyn_cast<CXXConstructorCall>(&Call);
+  if (!CC)
+    return;
+  const MemRegion *ThisValRegion = CC->getCXXThisVal().getAsRegion();
+  if (!ThisValRegion)
+    return;
+  updateTrackedRegion(Call, C, ThisValRegion);
+}
+
+void SmartPtrModeling::checkDeadSymbols(SymbolReaper &SymReaper,
+                                        CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  // Clean up dead regions from the region map.
+  TrackedRegionMapTy TrackedRegions = State->get<TrackedRegionMap>();
+  for (auto E : TrackedRegions) {
+    const MemRegion *Region = E.first;
+    bool IsRegDead = !SymReaper.isLiveRegion(Region);
+
+    if (IsRegDead) {
+      State = State->remove<TrackedRegionMap>(Region);
+    }
+  }
+  C.addTransition(State);
+}
+
+void SmartPtrModeling::checkDeferenceOps(const CallEvent &Call,
+                                         CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  const auto *OC = dyn_cast<CXXMemberOperatorCall>(&Call);
+  if (!OC)
+    return;
+  const MemRegion *ThisRegion = OC->getCXXThisVal().getAsRegion();
+  if (!ThisRegion)
+    return;
+
+  const auto *MethodDecl = dyn_cast_or_null<CXXMethodDecl>(OC->getDecl());
+
+  if (MethodDecl && MethodDecl->isOverloadedOperator()) {
+    OverloadedOperatorKind OOK = MethodDecl->getOverloadedOperator();
+    if (OOK == OO_Star || OOK == OO_Arrow) {
+      const auto *InnerPointVal = State->get<TrackedRegionMap>(ThisRegion);
+      if (InnerPointVal && InnerPointVal->isZeroConstant()) {
+        reportBug(C, Call);
+      }
+    }
+  }
+}
+
+void SmartPtrModeling::handleReset(const CallEvent &Call,
+                                   CheckerContext &C) const {
+  const auto *IC = dyn_cast<CXXInstanceCall>(&Call);
+  if (!IC)
+    return;
+
+  const MemRegion *ThisValRegion = IC->getCXXThisVal().getAsRegion();
+  if (!ThisValRegion)
+    return;
+  updateTrackedRegion(Call, C, ThisValRegion);
+}
+
+void SmartPtrModeling::handleSwap(const CallEvent &Call,
+                                  CheckerContext &C) const {
+  // TODO: Add support to handle swap method.
+}
+
+bool SmartPtrModeling::isStdSmartPointerClass(
+    const CXXRecordDecl *RecordDec) const {
+  if (!RecordDec || !RecordDec->getDeclContext()->isStdNamespace())
+    return false;
+
+  if (RecordDec->getDeclName().isIdentifier()) {
+    return StdSmartPtrs.count(RecordDec->getName().lower());
+  }
+  return false;
+}
+
+void SmartPtrModeling::reportBug(CheckerContext &C,
+                                 const CallEvent &Call) const {
+  ExplodedNode *ErrNode = C.generateErrorNode();
+  if (!ErrNode)
+    return;
+
+  auto R = std::make_unique<PathSensitiveBugReport>(
+      NullDereferenceBugType, "Dereference of null smart pointer", ErrNode);
+  R->addRange(Call.getSourceRange());
+  C.emitReport(std::move(R));
+}
+
+void SmartPtrModeling::updateTrackedRegion(
+    const CallEvent &Call, CheckerContext &C,
+    const MemRegion *ThisValRegion) const {
+  auto State = C.getState();
+  auto NumArgs = Call.getNumArgs();
+
+  if (NumArgs == 0) {
+    auto NullSVal = C.getSValBuilder().makeNull();
+    State = State->set<TrackedRegionMap>(ThisValRegion, NullSVal);
+  } else if (NumArgs == 1) {
+    auto ArgVal = Call.getArgSVal(0);
+    State = State->set<TrackedRegionMap>(ThisValRegion, ArgVal);
+  }
+
+  C.addTransition(State);
+}
+
 void ento::registerSmartPtrModeling(CheckerManager &Mgr) {
   Mgr.registerChecker<SmartPtrModeling>();
 }
 
 bool ento::shouldRegisterSmartPtrModeling(const CheckerManager &mgr) {
   const LangOptions &LO = mgr.getLangOpts();
   return LO.CPlusPlus;
 }

clang/test/Analysis/Inputs/system-header-simulator-cxx.h

 
 }
 
 #if __cplusplus >= 201103L
 namespace std {
   template <typename T> // TODO: Implement the stub for deleter.
   class unique_ptr {
   public:
+    unique_ptr() {}
+    unique_ptr(T *) {}
     unique_ptr(const unique_ptr &) = delete;
     unique_ptr(unique_ptr &&);
 
     T *get() const;
+    T *release() const;
+    void reset(T *p = nullptr) const;
+    void swap(unique_ptr<T> &p) const;
 
     typename std::add_lvalue_reference<T>::type operator*() const;
     T *operator->() const;
     operator bool() const;
   };
 }
 #endif
 

clang/test/Analysis/smart-ptr.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection\
 // RUN:   -analyzer-checker cplusplus.Move,cplusplus.SmartPtr\
 // RUN:   -std=c++11 -verify %s
 
 #include "Inputs/system-header-simulator-cxx.h"
 
 void clang_analyzer_warnIfReached();
 
 void derefAfterMove(std::unique_ptr<int> P) {
   std::unique_ptr<int> Q = std::move(P);
   if (Q)
     clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
-  *Q.get() = 1; // no-warning
+  *Q.get() = 1;                     // no-warning
   if (P)
     clang_analyzer_warnIfReached(); // no-warning
   // TODO: Report a null dereference (instead).
   *P.get() = 1; // expected-warning {{Method called on moved-from object 'P'}}
 }
 
 // Don't crash when attempting to model a call with unknown callee.
 namespace testUnknownCallee {
 struct S {
   void foo();
 };
 void bar(S *s, void (S::*func)(void)) {
   (s->*func)(); // no-crash
 }
 } // namespace testUnknownCallee
+
+class A {
+public:
+  A(){};
+  void foo();
+};
+
+A *return_null() {
+  return nullptr;
+}
+
+void derefAfterValidCtr() {
+  std::unique_ptr<A> P(new A());
+  P->foo(); // No warning.
+}
+
+void derefAfterDefaultCtr() {
+  std::unique_ptr<A> P;
+  P->foo(); // expected-warning {{Dereference of null smart pointer [cplusplus.SmartPtr]}}
+}
+
+void derefAfterCtrWithNull() {
+  std::unique_ptr<A> P(nullptr);
+  *P; // expected-warning {{Dereference of null smart pointer [cplusplus.SmartPtr]}}
+}
+
+void derefAfterCtrWithNullReturnMethod() {
+  std::unique_ptr<A> P(return_null());
+  P->foo(); // expected-warning {{Dereference of null smart pointer [cplusplus.SmartPtr]}}
+}
+
+void derefAfterRelease() {
+  std::unique_ptr<A> P(new A());
+  P.release();
+  P->foo(); // expected-warning {{Dereference of null smart pointer [cplusplus.SmartPtr]}}
+}
+
+void derefAfterReset() {
+  std::unique_ptr<A> P(new A());
+  P.reset();
+  P->foo(); // expected-warning {{Dereference of null smart pointer [cplusplus.SmartPtr]}}
+}
+
+void derefAfterResetWithNull() {
+  std::unique_ptr<A> P(new A());
+  P.reset(nullptr);
+  P->foo(); // expected-warning {{Dereference of null smart pointer [cplusplus.SmartPtr]}}
+}
+
+void derefAfterResetWithNonNull() {
+  std::unique_ptr<A> P;
+  P.reset(new A());
+  P->foo(); // No warning.
+}
+
+void derefAfterReleaseAndResetWithNonNull() {
+  std::unique_ptr<A> P(new A());
+  P.release();
+  P.reset(new A());
+  P->foo(); // No warning.
+}
 No newline at end of file