[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal

> Till then, I recommend you to follow my effort at D88477 
> .

I'm aleady on this way. I'm debugging the Store. I think we load a wrong type 
because we store a wrong type.


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D77062#2305856 , @ASDenysPetrov 
wrote:

> @steakhal
>
>> @ASDenysPetrov Do you still want to rework the API of the `assumeZero`?
>
> This patch more looks like NFC, being just refactored.

Fine.

> Actually I see that if we find and fix the root cause, we can freely refuse 
> this patch.
> Another thing I see is that this patch will work after a root cause fix as 
> well.

Yes, I think so.

> And the last one is that as long a root cause stays unfixed, clang will emit 
> an assertion without this patch, at least for my testcase.
>
> So, I can't really evaluate this objectively. What do you think?

Actually, I'm expecting to fix this in the need future. I'm really doing the 
extra mile to come up with a proper fix.
Unfortunately, it's deeply in the Core and requires a bunch of stuff to work 
out how to achieve this.
It will be a nice first-time experience though - I've already learned a lot 
just by debugging it.

Till then, I recommend you to follow my effort at D88477 
.


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal

> @ASDenysPetrov Do you still want to rework the API of the `assumeZero`?

This patch more looks like NFC, being just refactored.
Actually I see that if we find and fix the root cause, we can freely refuse 
this patch.
Another thing I see is that this patch will work after a root cause fix as well.
And the last one is that as long a root cause stays unfixed, clang will emit an 
assertion without this patch, at least for my testcase.

So, I can't really evaluate this objectively. What do you think?


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D77062#2298663 , @ASDenysPetrov 
wrote:

> @steakhal
> You told that you suppose a potential fix. It would be nice, if you share the 
> patch to review.

I mean, I already told you my suggestion, there was no concrete fix in my mind 
at the time.

In D77062#2298748 , @NoQ wrote:

> Nice, very interesting!
>
> The contract of RegionStore with respect to type punning is that it stores 
> the value //as written//, even if its type doesn't match the storage type, 
> but then it casts the value to the correct type upon reading by invoking 
> `CastRetrievedVal()` on it. That's where the fix should probably be.

Hm, thank you for the pointer.
I'm not sure if I fixed this in the right way - I'm still puzzled by the 
spaghetti code :D
Never the less, here is my proposed fix for this: D88477 


---

@ASDenysPetrov Do you still want to rework the API of the `assumeZero`?


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Nice, very interesting!

The contract of RegionStore with respect to type punning is that it stores the 
value //as written//, even if its type doesn't match the storage type, but then 
it casts the value to the correct type upon reading by invoking 
`CastRetrievedVal()` on it. That's where the fix should probably be.


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal

> If we get the value of `**b`, we get a //NonLoc// of type //unsigned char//.
> The dump of `**b` confirms this: `reg_$4 Element{SymRegion{reg_$0},0 S64b,unsigned char}>`, which is a 
> `NonLoc` in deed.

Exactly. That's what I've been trying to explaine above!
This happens because of the casts, after which CSA stores operates with the 
symbol (b) as it points to `int*` (though, it really is `char**`).

> IMO we should fix the root cause of this in the Core.

I can't see a direct fix for now and feel quite unconfident in that part of 
code. That's why I suggested to accept this change as what makes CSA codebase 
resistant to current bugs in the Core, which we are not able to fix for now. We 
can change Summary and the name of this revision for acception without 
objections.

@steakhal
You told that you suppose a potential fix. It would be nice, if you share the 
patch to review.


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D77062#2294516 , @martong wrote:

> Though, the fix probably will not be simple, because the issue itself always 
> requires a 3x indirection. The code that is presented by @steakhal is the 
> least minimal example to get this crash. The reason why we cannot have a 
> crash with a `**` is a mystic at the moment.

I think probably the representation of casts is behind this.

Eg. if you reinterpret cast `b` to `int**`, and make the type pun that way, we 
don't crash.

  template  void clang_analyzer_dump(T);
  void test(int *a, char ***b) {
*(int **)b = a; // only this line changed!
clang_analyzer_dump(**b); // &SymRegion{reg_$2},0 S64b,char *}>}
if (**b == nullptr) // will-not-crash
  ;
  }


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Beware, Phabricator ruins the visual experience of this nice analysis. E.g 
`//char ***//` is visible as an italic `char *`.

> I think we should have a symbolic cast back to the static type before doing 
> anything with the SVal (iff the BaseKind differs).
> If we do this, we will get a Loc as expected - and neither this bug nor your 
> original bug would fire.

I fully agree, I think this is the way.

Though, the fix probably will not be simple, because the issue itself always 
requires a 3x indirection. The code that is presented by @steakhal is the least 
minimal example to get this crash. The reason why we cannot have a crash with a 
`**` is a mystic at the moment.


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

And of course, repro:

  ./bin/clang -cc1 -analyze -setup-static-analyzer -analyzer-checker=core 
example.c
  
  Assertion `op == BO_Add' failed
  
   #0 0x7f5bea743904 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
/home/elnbbea/git/llvm-project/build/debug/../../llvm/lib/Support/Unix/Signals.inc:563:0
   #1 0x7f5bea7439a8 PrintStackTraceSignalHandler(void*) 
/home/elnbbea/git/llvm-project/build/debug/../../llvm/lib/Support/Unix/Signals.inc:627:0
   #2 0x7f5bea741759 llvm::sys::RunSignalHandlers() 
/home/elnbbea/git/llvm-project/build/debug/../../llvm/lib/Support/Signals.cpp:70:0
   #3 0x7f5bea743286 SignalHandler(int) 
/home/elnbbea/git/llvm-project/build/debug/../../llvm/lib/Support/Unix/Signals.inc:405:0
   #4 0x7f5be9b19fd0 (/lib/x86_64-linux-gnu/libc.so.6+0x3efd0)
   #5 0x7f5be9b19f47 raise 
/build/glibc-2ORdQG/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
   #6 0x7f5be9b1b8b1 abort 
/build/glibc-2ORdQG/glibc-2.27/stdlib/abort.c:81:0
   #7 0x7f5be9b0b42a __assert_fail_base 
/build/glibc-2ORdQG/glibc-2.27/assert/assert.c:89:0
   #8 0x7f5be9b0b4a2 (/lib/x86_64-linux-gnu/libc.so.6+0x304a2)
   #9 0x7f5bdece2000 
clang::ento::SValBuilder::evalBinOp(llvm::IntrusiveRefCntPtr, clang::BinaryOperatorKind, clang::ento::SVal, clang::ento::SVal, 
clang::QualType) 
/home/elnbbea/git/llvm-project/build/debug/../../clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:439:0
  #10 0x7f5bdebd28ae 
clang::ento::ExprEngine::evalBinOp(llvm::IntrusiveRefCntPtr, clang::BinaryOperatorKind, clang::ento::SVal, clang::ento::SVal, 
clang::QualType) 
/home/elnbbea/git/llvm-project/build/debug/../../clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:631:0
  #11 0x7f5bdebeb031 
clang::ento::ExprEngine::VisitBinaryOperator(clang::BinaryOperator const*, 
clang::ento::ExplodedNode*, clang::ento::ExplodedNodeSet&) 
/home/elnbbea/git/llvm-project/build/debug/../../clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:100:0
  #12 0x7f5bdebc0aa5 clang::ento::ExprEngine::Visit(clang::Stmt const*, 
clang::ento::ExplodedNode*, clang::ento::ExplodedNodeSet&) 
/home/elnbbea/git/llvm-project/build/debug/../../clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:1573:0
  #13 0x7f5bdebbca10 clang::ento::ExprEngine::ProcessStmt(clang::Stmt 
const*, clang::ento::ExplodedNode*) 
/home/elnbbea/git/llvm-project/build/debug/../../clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:792:0


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Finally, I made my investigations and I come up with this code:

  void strcpy(char *, char *);
  void test(int *a, char ***b) {
*(unsigned char **)b = (unsigned char*)a; // #1
if (**b == nullptr) // will-crash
  ;
  }

So, this issue does not relate to CStringChecker. It will crash at 
`ExprEngineC.cpp:100`.
It seems that we have to make the choice of how to model type punning.
As you can see in the example, we overwrite the pointer value of `*b` to point 
to an //unsigned char// value //(#1)//.
The static type of `b` (//char***//) does not reflect the associated value's 
type which (//unsigned char**//) - //(note the number of indirections!)// in 
other words, an obvious type pun happened at that line.
If we get the value of `**b`, we get a //NonLoc// of type //unsigned char//.
The dump of `**b` confirms this: `reg_$4},0 S64b,unsigned char}>`, which is a `NonLoc` 
in deed.

IMO we should fix the root cause of this in the Core.
I think we should have a symbolic cast back to the static type before doing 
anything with the SVal (iff the BaseKind differs).
If we do this, we will get a Loc as expected - and neither this bug nor your 
original bug would fire.
WDYT @NoQ @martong @ASDenysPetrov @Szelethus?


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal

> I would not accept this patch unless this investigation is done. However, I'm 
> not inherently against this change.

Actually I've done the investigation. You can find it here 
https://reviews.llvm.org/D77062#1977613

The bug is somewhere deep in the core. I see two solutions:

1. Leave the crash presented in the clang and dig deep into the core trying to 
solve the root cause.
2. Accept the change with a TODO promt and dig deep into the core trying to 
solve the root cause.




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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D77062#1976414 , @Szelethus wrote:

> I think what what be great to see here (and this seems to be the thing @NoQ 
> is fishing for) is not to change an if branch and avoid running into the 
> crash, but rather find out why `assumeZero` was ever called with a `nonloc` 
> value, which shouldn't really happen. We're treating the symptom, not curing 
> the illness, if you will. The `SVal` (if its `DefinedSVal`) is supposed to be 
> always `MemRegionVal` here, is that right? Maybe if we tried to add an assert 
> here, that could help nail where the actual issue is coming from. It's 
> probably in `evalStrcpyCommon`, judging from the bug report you linked in 
> your summary.

I would not accept this patch unless this investigation is done. However, I'm 
not inherently against this change.


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

One more notification. Please, somebody look at this patch.


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

A gentle notification :-)


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

One more ping


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ just one more ping. You accepted it and then I just revised it again. 
Could you, please, take a minute and look at it.
I'd close it with a great pleasure. It's been hanging too long.


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.
Herald added a subscriber: steakhal.

Ping!




Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:199
   // Utility methods
-  std::pair
-  static assumeZero(CheckerContext &C,
-ProgramStateRef state, SVal V, QualType Ty);
+  std::pair static assumeZero(
+  ProgramStateRef state, SVal V);

vsavchenko wrote:
> Can you please put `static` before return type, it will be more consistent 
> with other static methods nearby.
Sure, I just didn't pay attention to that.


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

https://reviews.llvm.org/D77062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits