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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
