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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Nice!~ I'm glad this is getting sorted out.



================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:103
+        if (const MemRegion *LeftMR = LeftV.getAsRegion())
+          IsLhsPtr = LeftMR->getSymbolicBase();
+        if (const MemRegion *RightMR = RightV.getAsRegion())
----------------
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.


================
Comment at: clang/test/Analysis/symbol-escape.cpp:2
 // RUN: %clang_analyze_cc1 \
 // RUN:  -analyzer-checker=cplusplus.NewDeleteLeaks \
 // RUN:  -verify %s
----------------
Pls include `core` as well, just in case, because running path-sensitive analysis without `core` checkers is unsupported.


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


================
Comment at: clang/test/Analysis/symbol-escape.cpp:7
+
 #include <stdint.h>
 
----------------
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;
```


================
Comment at: clang/test/Analysis/symbol-escape.cpp:15
                                ~static_cast<uintptr_t>(0x1)) |
                               (reinterpret_cast<uintptr_t>(Bar) & 0x1));
   (void)Bar;
----------------
`Bar` is reduced to one bit here. It's a legit leak. I think you meant to swap `Foo` and `Bar`.


================
Comment at: clang/test/Analysis/symbol-escape.cpp:20
 
   delete Bar;
 }
----------------
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).


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

https://reviews.llvm.org/D63720