[PATCH] D77062: [analyzer] Added check for unacceptable equality operation between Loc and NonLoc types

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov marked 3 inline comments as done.
ASDenysPetrov added a comment.

@NoQ You concerns are absolutly justified. I agree with you. Let me tell what I'm thinking in inlines.

If you are interested in why the assertion happens, please, look D77062#1977613 <https://reviews.llvm.org/D77062#1977613>



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:273
   Optional<DefinedSVal> val = V.getAs<DefinedSVal>();
-  if (!val)
-    return std::pair<ProgramStateRef , ProgramStateRef >(state, state);
+  if (val && !V.getAs<nonloc::LazyCompoundVal>()) {
+     // return pair shall be {null, non-null} so reorder states
----------------
NoQ wrote:
> Basically `SVal::getAs<>` should not be used for discovering the type of the value; only the exact representation that's used for the value of the type that's already in your possession. Say, it's ok to use it to discriminate between, say, compound value and lazy compound value. It's ok to use it to discriminate between a concrete integer and a symbol. It's ok to use it do discriminate between a known value and an unknown value. But if it's used for discriminating between a compound value and a numeric symbol, i'm 99% sure it's incorrect. You should already know the type from the AST before you even obtain the value. It doesn't make sense to run the checker at all if the function receives a structure. And if it doesn't receive the structure but the run-time value is of structure type, then either the checker isn't obtaining the value correctly or there's bug in path-sensitive analysis. That's why i still believe you're only treating the symptoms. There's nothing normal in the situation where "strcpy suddenly accepts a structure (or an array) by value".
I'll remove `V.getAs<nonloc::LazyCompoundVal>()`. I mistakenly added this `V.getAs<nonloc::LazyCompoundVal>()` to keep code safe regardless of the checker intention and sanity. The point is that `state->assume(*val)` calls `ConstraintMgr->assumeDual` which finally calls `SimpleConstraintManager::assumeAux`. `assumeAux` handles all NonLoc kinds except **LazyCompoundValKind**and **CompoundValKind**(I missed to check it). In case of these two kinds it leads to `llvm_unreachable("'Assume' not implemented for this NonLoc");`. But for now I understand that I failed because **unreachable** means I might not have check for those kinds, since the function already takes on this work. 

However it is not an actual fix. The fix is `std::tie(states.second, states.first) = state->assume(*val);`. You can see the summary above.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2270-2278
   // Pro-actively check that argument types are safe to do arithmetic upon.
   // We do not want to crash if someone accidentally passes a structure
   // into, say, a C++ overload of any of these functions. We could not check
   // that for std::copy because they may have arguments of other types.
   for (auto I : CE->arguments()) {
     QualType T = I->getType();
     if (!T->isIntegralOrEnumerationType() && !T->isPointerType())
----------------
This check prevents passing **structures**. From this point we are sure to work with **pointers **and **integral **types in arguments:
E.g. `char *strcpy(int i1, int i2);`, `char *strcpy(char *c1, char *c2);`, `char *strcpy(Kind t1, Kind t2);`
This confirms @NoQ's supposition that checker narrows argument types.
P.S. Honestly I'd narrow this more aggressively to just a **char pointer **type, anyway it doesn't relate to the bug and I wouldn't add this to a single patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77062/new/

https://reviews.llvm.org/D77062