[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

31

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

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

46

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

60

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

65

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

119

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

134

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.

150

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.

195

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.

225

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

72–76

Please consider CallDescription and CallDescriptionMap!

81

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.

213–214

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

46

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

31

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.

33

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

119

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.

160–187

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.

230

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

233

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

233

NOTE: 👏 Call 👏 Description 👏 Map 👏

[vrnithinkumar]

Addressing the review comments

[vrnithinkumar]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

31

With the new changes, the struct is removed.

31

With the new changes, the struct is removed.

33

With the new changes, the enum is removed.

46

with new change only one anonymous namespace is there

60

updated to initialize bugtype immediately

65

Updated the comments as LLVM style

72–76

Made changes to use CallDescription and CallDescriptionMap

81

Removed this map.
Now maping region to SVal

119

fixed

119

Okay.
Will try to use longer names in future.

150

Added the check for early return

160–187

Merged

213–214

updated to initialize bugtype immediately

233

Changed to use CallDescription

233

Changed to use CallDescription

233

Changed to use CallDescription

[vrnithinkumar]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

195

This map is removed in the new changes

225

Added std namespace check

[NoQ]

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

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

108

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.

122

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.

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

52

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

68

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.

97

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

111

Early returns can decrease the indentation.

113

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

133

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

135

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.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

122

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.

133

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.

135

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

This happens in destructor.

[vrnithinkumar]

Addressing review comments

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

52

Fixed the format

68

Thanks!
Moved to top.

97

added isStdSmartPointerClass check in the beginning.

108

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

111

Updated with early return.
Thanks!

113

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

122

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

135

removed the braces

[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

112–113

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

112–113

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

125

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

116

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

118

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

121

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.

144

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

116

Changed to dyn_cast.
Added assert

118

Removed the redundant check

121

Added getOverloadedOperator() in CXXMemberOperatorCall class

125

Added one more run to verify it.

144

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

6 ↗

(On Diff #275076)

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

6 ↗

(On Diff #275076)

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

64

Looks like dead code.

86–99

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

97

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

5 ↗

(On Diff #275254)

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

64

Thanks!
removed.

86–99

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?

97

:)

clang/test/Analysis/smart-ptr.cpp

5 ↗

(On Diff #275254)

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

186–203

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

186–203

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

186–203

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/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> {
+struct RegionState {
+private:
+  enum Kind { Null, NonNull, Unknown } K;
+  RegionState(Kind InK) : K(InK) {}
+
+public:
+  bool isNull() const { return K == Null; }
+
+  static RegionState getNull() { return RegionState(Null); }
+  static RegionState getNonNull() { return RegionState(NonNull); }
+  static RegionState getUnKnown() { return RegionState(Unknown); }
+
+  bool operator==(const RegionState &X) const { return K == X.K; }
+  void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddInteger(K); }
+};
+} // end of anonymous namespace
+
+namespace {
+class SmartPtrModeling : public Checker<eval::Call, check::PreCall,
+                                        check::PostCall, check::DeadSymbols> {
   bool isNullAfterMoveMethod(const CallEvent &Call) const;
 
 public:
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+  void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
+
+private:
+  mutable std::unique_ptr<BugType> BT;
+  void reportBug(CheckerContext &C, const CallEvent &Call) const;
+  bool isSmartPointer(const CXXRecordDecl *RD) const;
+  bool isResetMethod(const CXXMethodDecl *MethodDec) const;
+
+  // STL smart pointers which we are trying to model
+  const llvm::StringSet<> StdSmartPtrs = {
+      "shared_ptr",
+      "unique_ptr",
+      "weak_ptr",
+  };
+
+  // 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"};
 };
 } // end of anonymous namespace
 
+// 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()
+// method
+REGISTER_MAP_WITH_PROGRAMSTATE(SymbolRegionMap, const SymbolRef, MemRegion)
+
 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;
 
   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;
 }
 
+void SmartPtrModeling::checkPreCall(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->isOverloadedOperator()) {
+    OverloadedOperatorKind OOK = MethodDecl->getOverloadedOperator();
+    if (OOK == OO_Star || OOK == OO_Arrow) {
+      const RegionState *RS = State->get<TrackedRegionMap>(ThisRegion);
+      if (RS && RS->isNull()) {
+        reportBug(C, Call);
+      }
+    }
+  }
+}
+
+void SmartPtrModeling::checkPostCall(const CallEvent &Call,
+                                     CheckerContext &C) const {
+  auto State = C.getState();
+
+  // Default constructor being null
+  if (const auto *CC = dyn_cast<CXXConstructorCall>(&Call)) {
+    const auto *CtorDec = CC->getDecl();
+    auto isEmptyCtr = CC->getNumArgs() == 0;
+    const MemRegion *ThisValRegion = CC->getCXXThisVal().getAsRegion();
+    if (ThisValRegion && CtorDec && isEmptyCtr) {
+      const CXXRecordDecl *RD = CtorDec->getParent();
+      if (isSmartPointer(RD)) {
+        State =
+            State->set<TrackedRegionMap>(ThisValRegion, RegionState::getNull());
+        C.addTransition(State);
+        return;
+      }
+    }
+  }
+
+  // calling reset or making it empty
+  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)) {
+      const MemRegion *ThisValRegion = IC->getCXXThisVal().getAsRegion();
+      State =
+          State->set<TrackedRegionMap>(ThisValRegion, RegionState::getNull());
+      C.addTransition(State);
+      return;
+    }
+
+    // in case of reset to null or proper value
+    if (ArgsNum == 1 && isResetMethod(MethodDecl)) {
+      const MemRegion *ThisValRegion = IC->getCXXThisVal().getAsRegion();
+      auto ArgVal = IC->getArgSVal(0);
+      if (ArgVal.isZeroConstant()) {
+        State =
+            State->set<TrackedRegionMap>(ThisValRegion, RegionState::getNull());
+      } else {
+        State = State->set<TrackedRegionMap>(ThisValRegion,
+                                             RegionState::getNonNull());
+      }
+      C.addTransition(State);
+      return;
+    }
+  }
+}
+
+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::reportBug(CheckerContext &C,
+                                 const CallEvent &Call) const {
+  ExplodedNode *ErrNode = C.generateErrorNode();
+  if (!ErrNode)
+    return;
+
+  if (!BT)
+    BT.reset(new BugType(this, "unique_ptr", "Dont call unique_ptr"));
+  auto R = std::make_unique<PathSensitiveBugReport>(
+      *BT, "Dereference of uninitialized smart pointer", ErrNode);
+  R->addRange(Call.getSourceRange());
+  C.emitReport(std::move(R));
+}
+
+bool SmartPtrModeling::isSmartPointer(const CXXRecordDecl *RecordDec) const {
+  if (!RecordDec)
+    return false;
+  if (RecordDec->getDeclName().isIdentifier()) {
+    return StdSmartPtrs.count(RecordDec->getName().lower());
+  }
+  return false;
+}
+
+bool SmartPtrModeling::isResetMethod(const CXXMethodDecl *MethodDec) const {
+  if (!MethodDec)
+    return false;
+  if (MethodDec->getDeclName().isIdentifier()) {
+    return ResetMethods.count(MethodDec->getName().lower());
+  }
+  return false;
+}
+
 void ento::registerSmartPtrModeling(CheckerManager &Mgr) {
   Mgr.registerChecker<SmartPtrModeling>();
 }
 
 bool ento::shouldRegisterSmartPtrModeling(const CheckerManager &mgr) {
   const LangOptions &LO = mgr.getLangOpts();
   return LO.CPlusPlus;
 }