[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

Commit rGde2547329b41ad6ea4ea876d12731bde5a6b64c5 
 
accidentally refers to this review, but in fact it belongs to D148355 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135375

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

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Szelethus marked an inline comment as done.
Closed by commit rGa504ddc8bf9d: [analyzer] Initialize regions returned by 
CXXNew to undefined (authored by Szelethus).

Changed prior to commit:
  https://reviews.llvm.org/D135375?vs=465779=470821#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135375

Files:
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/test/Analysis/NewDelete-checker-test.cpp
  clang/test/Analysis/cxx-member-initializer-const-field.cpp
  clang/test/Analysis/new-ctor-conservative.cpp
  clang/test/Analysis/new-ctor-recursive.cpp
  clang/test/Analysis/new.cpp
  clang/test/Analysis/placement-new.cpp
  clang/test/Analysis/reinterpret-cast.cpp
  clang/test/Analysis/uninit-const.cpp

Index: clang/test/Analysis/uninit-const.cpp
===
--- clang/test/Analysis/uninit-const.cpp
+++ clang/test/Analysis/uninit-const.cpp
@@ -21,9 +21,11 @@
 
 int f10(void) {
   int *ptr;
-
-  ptr = new int; //
-  if(*ptr) {
+ // FIXME: The message is misleading -- we should state that
+ // a pointer to an uninitialized value is stored.
+  ptr = new int; // expected-note{{Storing uninitialized value}}
+  if(*ptr) { // expected-warning{{Branch condition evaluates to a garbage value [core.uninitialized.Branch]}}
+ // expected-note@-1 {{Branch condition evaluates to a garbage value}}
 doStuff4(*ptr);
   }
   delete ptr;
@@ -32,10 +34,12 @@
 
 int f9(void) {
   int *ptr;
-
-  ptr = new int; //
-
-  doStuff_uninit(ptr); // no warning
+ // FIXME: The message is misleading -- we should state that
+ // a pointer to an uninitialized value is stored.
+  ptr = new int; // expected-note{{Storing uninitialized value}}
+ // expected-note@-1{{Value assigned to 'ptr'}}
+  doStuff_uninit(ptr); // expected-warning{{1st function call argument is a pointer to uninitialized value [core.CallAndMessage]}}
+   // expected-note@-1{{1st function call argument is a pointer to uninitialized value}}
   delete ptr;
   return 0;
 }
Index: clang/test/Analysis/reinterpret-cast.cpp
===
--- clang/test/Analysis/reinterpret-cast.cpp
+++ clang/test/Analysis/reinterpret-cast.cpp
@@ -77,15 +77,11 @@
   class Derived : public Base {};
 
   void test() {
-	Derived* p;
-	*(reinterpret_cast()) = new C;
-	p->f();
-
-// We should still be able to do some reasoning about bindings.
-p->x = 42;
-clang_analyzer_eval(p->x == 42); // expected-warning{{TRUE}}
+Derived* p;
+*(reinterpret_cast()) = new C;
+p->f(); // expected-warning{{Called function pointer is an uninitialized pointer value [core.CallAndMessage]}}
   };
-}
+} // namespace PR15345
 
 int trackpointer_std_addressof() {
   int x;
Index: clang/test/Analysis/placement-new.cpp
===
--- clang/test/Analysis/placement-new.cpp
+++ clang/test/Analysis/placement-new.cpp
@@ -112,6 +112,10 @@
 namespace testHeapAllocatedBuffer {
 void g2() {
   char *buf = new char[2]; // expected-note {{'buf' initialized here}}
+   // FIXME: The message is misleading -- we should
+   // state that a pointer to an uninitialized value
+   // is stored.
+   // expected-note@-4{{Storing uninitialized value}}
   long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
   (void)lp;
 }
Index: clang/test/Analysis/new.cpp
===
--- clang/test/Analysis/new.cpp
+++ clang/test/Analysis/new.cpp
@@ -177,15 +177,10 @@
   clang_analyzer_eval(p.y == 2); // expected-warning{{TRUE}}
 }
 
-//
-// Incorrectly-modelled behavior
-//
-
 int testNoInitialization() {
   int *n = new int;
 
-  // Should warn that *n is uninitialized.
-  if (*n) { // no-warning
+  if (*n) { // expected-warning{{Branch condition evaluates to a garbage value [core.uninitialized.Branch]}}
 delete n;
 return 0;
   }
@@ -193,6 +188,10 @@
   return 1;
 }
 
+//===--===//
+// Incorrectly-modelled behavior.
+//===--===//
+
 int testNoInitializationPlacement() {
   int n;
   new () int;
Index: clang/test/Analysis/new-ctor-recursive.cpp
===
--- clang/test/Analysis/new-ctor-recursive.cpp
+++ clang/test/Analysis/new-ctor-recursive.cpp
@@ -51,11 +51,9 

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

In D135375#3882491 , @Szelethus wrote:

> Seems like the issues mentioned above are real, but orthogonal to this patch. 
> Would it be okay to address them in followup patches? @martong @NoQ

I am okay with it. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135375

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

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Seems like the issues mentioned above are real, but orthogonal to this patch. 
Would it be okay to address them in followup patches? @martong @NoQ




Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:927
 SVal RetVal = State->getSVal(CNE, LCtx);
+State = State->bindDefaultInitial(RetVal, UndefinedVal{}, LCtx);
 

steakhal wrote:
> Yeey, finally we will have this :D
> 
> I wonder if we could query from the `ASTContext` if we have a trivially 
> constructible class typeor something as a first approximation.
And a result skip the rest of this function?



Comment at: clang/test/Analysis/NewDelete-checker-test.cpp:388-392
+  ~DerefClass() {
+int i = 0;
+x = 
+*x = 1;
+  }

steakhal wrote:
> This change seems unrelated.
> Could you elaborate on that?
The test case in its original version would have emitted an uninitialized use 
report, which is fine, but the intention is to test double deletes, not 
uninitialized use, hence the seemingly unrelated change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135375

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

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Yay I'm glad that you got to implement that!!

> I'd appreciate if the "storing uninitialized value" was placed inside the 
> notes about the call to QScopedArrayPointer's constructor.

It should not be placed inside the notes about the call to constructor, because 
it doesn't happen during constructor invocation. It happens during `operator 
new` invocation, which is strictly before the constructor.

I guess we could improve the note to specify that it was operator new that 
stored the value; this could help in general case as well.

Separately, there *should* be notes inside the constructor as well, about the 
fact that the constructor *did not* initialize the fields, even though it 
*could have* (it literally had one job!) - You know, your favorite problem for 
at least two unrelated reasons =)

> Messag 10 talks about what happens in applyColorTransform, but the important 
> thing happens at the evaluation of the argument, which is not described, so 
> this isn't a very good bug report, can't tell whether its false.

Yeah looks like `trackExpressionValue()` wasn't able to track it inside 
`transformationToColorSpace()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135375

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

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

I am happy with the change and with the result. However, [[ 
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed=Confirmed%20bug=New=Reopened=Unresolved=on=%2acxxnew_baseline=%2acxxnew_patched=New=0fa7641b6d8982587cef9512fa7121bb=%2aqimage.cpp=460856
 | this `colorSpace` related report ]] is a bit concerning. Could you please 
check if `colorSpace` at step 1 is evaluated as Undefined? Could that happen? 
Or that is (should be) `Unknown` since that is a parameter of a top-level 
analyzed function? Did you have this report before your change? I think this 
would worth to be further analyzed, maybe just directing the analyzer to 
analyze only the `QImage::convertedToColorSpace` top-level function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135375

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

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D135375#3849261 , @Szelethus wrote:

> Some early results:
>
> https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?review-status=Unreviewed=Confirmed%20bug=New=Reopened=Unresolved=on=%2acxxnew_baseline=%2acxxnew_patched=New
>
> I'm still waiting on a few projects, but this is something. A few reports 
> from `alpha.security.ArrayBound`, I still need to look into that.
>
> This 
> 
>  report from `core.uninitialized.Assign` is interesting. First off, I'd 
> appreciate if the "storing uninitialized value" was placed //inside// the 
> notes about the call to `QScopedArrayPointer`'s constructor. Otherwise, the 
> report looks correct.
>
> Not too happy about this 
> 
>  report by `core.CallAndMessage`. Message 9 is placed on the line
>
>   applyColorTransform(d->colorSpace.transformationToColorSpace(colorSpace));
>
> Messag 10 talks about what happens in `applyColorTransform`, but the 
> important thing happens at the evaluation of the argument, which is not 
> described, so this isn't a very good bug report, can't tell whether its false.

Yes, I agree, this bug-report is hard to follow. Seems like the `colorSpace` 
argument from step 1 is flowing through step 10. I wonder if `colorSpace` at 
step 1 is evaluated as `Undefined`? Could that happen? Did you have this report 
before your change?

> Reports from `core.UndefinedBinaryOperatorResult` here 
> 

This seems to be justified, at step 11 we initialize the variable with a 
negative value. I think, this has nothing to do with your changes.

> and `alpha.core.Conversion` here 
> 
>  are interesting, because there doesn't need to be caused by uninitialized 
> dynamic memory, but rather the fact that the memory is being modeled at all. 
> I suspect this is due us maybe discarding array size information or something 
> similar when the value held in `UnknownVal`?
>
> The rest of the reports seem nobrainder gains.
>
> Interstingly, 3 reports are lost 
> 
>  from `alpha.security.ArrayBound`, but I'm not sure how seriously do we take 
> this checker.

I think these new reports and the lost reports are the result of the changed 
analysis paths. With your patch, there are new bug reports which introduce sink 
nodes which were not there previously. Consequently, the set of analyzed paths 
and thus the new results can be different.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135375

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

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Some early results:

https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?review-status=Unreviewed=Confirmed%20bug=New=Reopened=Unresolved=on=%2acxxnew_baseline=%2acxxnew_patched=New

I'm still waiting on a few projects, but this is something. A few reports from 
`alpha.security.ArrayBound`, I still need to look into that.

This 

 report from `core.uninitialized.Assign` is interesting. First off, I'd 
appreciate if the "storing uninitialized value" was placed //inside// the notes 
about the call to `QScopedArrayPointer`'s constructor. Otherwise, the report 
looks correct.

Not too happy about this 

 report by `core.CallAndMessage`. Message 9 is placed on the line

  applyColorTransform(d->colorSpace.transformationToColorSpace(colorSpace));

Messag 10 talks about what happens in `applyColorTransform`, but the important 
thing happens at the evaluation of the argument, which is not described, so 
this isn't a very good bug report, can't tell whether its false.

Reports from `core.UndefinedBinaryOperatorResult` here 

 and `alpha.core.Conversion` here 

 are interesting, because there doesn't need to be caused by uninitialized 
dynamic memory, but rather the fact that the memory is being modeled at all. I 
suspect this is due us maybe discarding array size information or something 
similar when the value held in `UnknownVal`?

The rest of the reports seem nobrainder gains.

Interstingly, 3 reports are lost 

 from `alpha.security.ArrayBound`, but I'm not sure how seriously do we take 
this checker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135375

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

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

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

steakhal added a comment.

Awesome!
Have you measured how often would this change introduce new garbage value 
warnings?
At the other side of the spectrum it could also hide reports, because it sinks 
the path too soon due to the falsely binding uninitialized value there.
WDYT?




Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:927
 SVal RetVal = State->getSVal(CNE, LCtx);
+State = State->bindDefaultInitial(RetVal, UndefinedVal{}, LCtx);
 

Yeey, finally we will have this :D

I wonder if we could query from the `ASTContext` if we have a trivially 
constructible class typeor something as a first approximation.



Comment at: clang/test/Analysis/NewDelete-checker-test.cpp:388-392
+  ~DerefClass() {
+int i = 0;
+x = 
+*x = 1;
+  }

This change seems unrelated.
Could you elaborate on that?



Comment at: clang/test/Analysis/new.cpp:180-182
 //
 // Incorrectly-modelled behavior
 //

You should probably adjust this comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135375

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

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Just a note on the test files -- I've diverged from the usual stance of just 
changing what the new output is, to modifying the test files. The reason is 
that reading an undefined value is a fatal error, leading to the analyzer to 
stop analyzing prematurely, and I think these cases were trying to test 
something else, not uninitialized value usage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135375

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

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, martong, steakhal, balazske, isuckatcs.
Szelethus added a project: clang.
Herald added subscribers: manas, ASDenysPetrov, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: All.
Szelethus requested review of this revision.
Herald added a subscriber: cfe-commits.

Discourse mail: 
https://discourse.llvm.org/t/analyzer-why-do-we-suck-at-modeling-c-dynamic-memory/65667

`malloc()` returns a piece of uninitialized dynamic memory. We were (almost) 
always to model this behaviour. Its C++ counterpart, `operator new` is a lot 
more complex, because it allows for initialization, the most complicated of 
which the usage of constructors.

We gradually became better in modeling constructors, but for some reason, most 
likely for reasons lost in history, we never actually modeled the case when the 
memory returned by `operator new` was just simply uninitialized. This patch 
(attempts) to fix this tiny little error.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135375

Files:
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/test/Analysis/NewDelete-checker-test.cpp
  clang/test/Analysis/cxx-member-initializer-const-field.cpp
  clang/test/Analysis/new-ctor-conservative.cpp
  clang/test/Analysis/new-ctor-recursive.cpp
  clang/test/Analysis/new.cpp
  clang/test/Analysis/placement-new.cpp
  clang/test/Analysis/reinterpret-cast.cpp
  clang/test/Analysis/uninit-const.cpp

Index: clang/test/Analysis/uninit-const.cpp
===
--- clang/test/Analysis/uninit-const.cpp
+++ clang/test/Analysis/uninit-const.cpp
@@ -21,9 +21,11 @@
 
 int f10(void) {
   int *ptr;
-
-  ptr = new int; //
-  if(*ptr) {
+ // FIXME: The message is misleading -- we should state that
+ // a pointer to an uninitialized value is stored.
+  ptr = new int; // expected-note{{Storing uninitialized value}}
+  if(*ptr) { // expected-warning{{Branch condition evaluates to a garbage value [core.uninitialized.Branch]}}
+ // expected-note@-1 {{Branch condition evaluates to a garbage value}}
 doStuff4(*ptr);
   }
   delete ptr;
@@ -32,10 +34,12 @@
 
 int f9(void) {
   int *ptr;
-
-  ptr = new int; //
-
-  doStuff_uninit(ptr); // no warning
+ // FIXME: The message is misleading -- we should state that
+ // a pointer to an uninitialized value is stored.
+  ptr = new int; // expected-note{{Storing uninitialized value}}
+ // expected-note@-1{{Value assigned to 'ptr'}}
+  doStuff_uninit(ptr); // expected-warning{{1st function call argument is a pointer to uninitialized value [core.CallAndMessage]}}
+   // expected-note@-1{{1st function call argument is a pointer to uninitialized value}}
   delete ptr;
   return 0;
 }
Index: clang/test/Analysis/reinterpret-cast.cpp
===
--- clang/test/Analysis/reinterpret-cast.cpp
+++ clang/test/Analysis/reinterpret-cast.cpp
@@ -77,15 +77,11 @@
   class Derived : public Base {};
 
   void test() {
-	Derived* p;
-	*(reinterpret_cast()) = new C;
-	p->f();
-
-// We should still be able to do some reasoning about bindings.
-p->x = 42;
-clang_analyzer_eval(p->x == 42); // expected-warning{{TRUE}}
+Derived* p;
+*(reinterpret_cast()) = new C;
+p->f(); // expected-warning{{Called function pointer is an uninitialized pointer value [core.CallAndMessage]}}
   };
-}
+} // namespace PR15345
 
 int trackpointer_std_addressof() {
   int x;
Index: clang/test/Analysis/placement-new.cpp
===
--- clang/test/Analysis/placement-new.cpp
+++ clang/test/Analysis/placement-new.cpp
@@ -112,6 +112,10 @@
 namespace testHeapAllocatedBuffer {
 void g2() {
   char *buf = new char[2]; // expected-note {{'buf' initialized here}}
+   // FIXME: The message is misleading -- we should
+   // state that a pointer to an uninitialized value
+   // is stored.
+   // expected-note@-4{{Storing uninitialized value}}
   long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
   (void)lp;
 }
Index: clang/test/Analysis/new.cpp
===
--- clang/test/Analysis/new.cpp
+++ clang/test/Analysis/new.cpp
@@ -184,8 +184,7 @@
 int testNoInitialization() {
   int *n = new int;
 
-  // Should warn that *n is uninitialized.
-  if (*n) { // no-warning
+  if (*n) { // expected-warning{{Branch condition evaluates to a garbage value [core.uninitialized.Branch]}}
 delete n;
 return 0;
   }
Index: