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