[PATCH] D63720: [analyzer] ExprEngine: Escape pointers in bitwise operations

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked an inline comment as done.
Charusso added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:103
+        if (const MemRegion *LeftMR = LeftV.getAsRegion())
+          IsLhsPtr = LeftMR->getSymbolicBase();
+        if (const MemRegion *RightMR = RightV.getAsRegion())
----------------
NoQ wrote:
> How about the following test case in which not `Bar` but `&Bar` gets bitwise-operated upon?
> 
> ```lang=c++
> C **test() {
>   C *Bar = new C;
>   C **Baz = &Bar;
>   Baz = reinterpret_cast<C **>(reinterpret_cast<uintptr_t>(Baz) | 0x1);
>   Baz = reinterpret_cast<C **>(reinterpret_cast<uintptr_t>(Baz) & ~static_cast<uintptr_t>(0x1));
>   delete *Baz;
> }
> ```
> 
> The difference is that in this case the escaping region doesn't have a symbolic base. And i believe that symbolic regions aren't special here in any way.
> 
> I suggest doing an escape when the resulting value is unknown after `evalBinOp` but regardless of any other conditions that you mentioned. Simply because there's a loss of information.
That is a great idea! I wanted to make it more generic. Thanks you!


================
Comment at: clang/test/Analysis/symbol-escape.cpp:5-6
 
+// expected-no-diagnostics
+
 #include <stdint.h>
----------------
NoQ wrote:
> I think something went wrong while uploading the patch. This diff should add this whole test file, not update it.
This is a test-driven-development-driven patch, first make the test fail, then make it pass.


================
Comment at: clang/test/Analysis/symbol-escape.cpp:7
+
 #include <stdint.h>
 
----------------
NoQ wrote:
> Relying on everybody's system headers is super flaky, don't do this in tests.
> 
> Please define stuff that you use directly like other tests do:
> ```lang=c++
> typedef unsigned __INTPTR_TYPE__ uintptr_t;
> ```
It is a predefined header in Modules so Windows will not break.

Please note that only one of the test files (malloc.c) use that definition, most of them using that I have used in D62926, so I believe this header is the correct one as it is in four tests and Windows-approved by D62926.


================
Comment at: clang/test/Analysis/symbol-escape.cpp:15
                                ~static_cast<uintptr_t>(0x1)) |
                               (reinterpret_cast<uintptr_t>(Bar) & 0x1));
   (void)Bar;
----------------
NoQ wrote:
> `Bar` is reduced to one bit here. It's a legit leak. I think you meant to swap `Foo` and `Bar`.
I assumed that we are doing our low-bits magic, where we have two identical objects. The SymRegion sets up the HeapSymRegion's property, where no leak happen. We just could change the first low-bit and set up some crazy flag, like `isChecked()`, so we do not lost the tracking. Also we have four bits to change, so there is no edge case going on.


================
Comment at: clang/test/Analysis/symbol-escape.cpp:20
 
   delete Bar;
 }
----------------
NoQ wrote:
> In any case, passing a pointer to `delete` that wasn't obtained from `new` is undefined behavior. In order to produce a test that's also a correct code, i think we should either undo our bitwise operations, or perform an escape in a different manner (say, return the pointer to the caller).
Whoops! I really wanted to make the test look great.


================
Comment at: clang/test/Analysis/symbol-escape.cpp:26
 		               ~static_cast<uintptr_t>(0x1));
-  // expected-warning at -2 {{Potential leak of memory pointed to by 'Qux'}}
 
----------------
I have made it fail on master.


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

https://reviews.llvm.org/D63720