Added new checker 'cert-mem54-cpp' that checks for CERT rule MEM54-CPP.
I have troubles writing English descriptions for the release notes, so I need all the help I can get :)
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| clang-tools-extra/clang-tidy/cert/PlacementNewStorageCheck.cpp | ||
|---|---|---|
| 20 | Please use static instead of anonymous namespaces for functions. See LLVM Coding Guidelines. | |
| 22 | const auto *. Same in other places. | |
| 48 | Belongs to isLanguageVersionSupported() now. | |
| 70 | const auto *. Same below. | |
| clang-tools-extra/docs/ReleaseNotes.rst | ||
| 95 | Please highlight new with double back-ticks. | |
| clang-tools-extra/docs/clang-tidy/checks/cert-mem54-cpp.rst | ||
| 7 | Please highlight new with double back-ticks. | |
Comment Actions
- Static functions instead of anonymous namespaces.
- const auto*.
- Added double back-ticks.
| clang-tools-extra/clang-tidy/cert/PlacementNewStorageCheck.cpp | ||
|---|---|---|
| 43 | Unnecessary empty line. | |
Comment Actions
Also fix the test case, premerge found a failure
| clang-tools-extra/clang-tidy/cert/PlacementNewStorageCheck.cpp | ||
|---|---|---|
| 23 | For better or worse children can sometimes contain nullptr, best to check for that first | |
| 52 | Not gonna say this is blocking anything, but this is quite a large function that could be made smaller (more readable) by moving some of those lambdas out of the function | |
Comment Actions
Have you considered making this a static analyzer check as opposed to a clang-tidy check? I think using dataflow analysis will really reduce the false positives because it will be able to track the allocation and alignment data across control flow.
Comment Actions
This seems to be already handled by clang static analyzer? (clang-analyzer-cplusplus.PlacementNew)
<source>:19:5: warning: Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes [clang-analyzer-cplusplus.PlacementNew]
::new (&s1) long;
^
<source>:19:5: note: Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes
<source>:64:3: warning: Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes [clang-analyzer-cplusplus.PlacementNew]
::new (buffer3) long;
^Comment Actions
I have never try to write static analyzer before. Okay, I will look at examples of how to work with dataflow.
Comment Actions
I have not really worked with static analyzer code, but assuming that those cases
that are no longer diagnosed as compared to this clang-tidy checks *should* be diagnosed
(i.e. diagnosing them isn't false-positive), then i'd think that static analyzer check
simply needs some work?
Comment Actions
Yeah. You are right. I will try to improve exist checker.
What we gonna do with this patch? Remove it?
Comment Actions
First of all, the static analyzer doesn't warn on everything at once; it stops the analysis after a fatal error (such as UB) is found, so it doesn't explore the execution path further. This is the reason why some tests look like they don't warn, but they will if you comment out the previous tests in the same function.
// bad (short is aligned to 2). short* s6 = nullptr; ::new (s6) long; // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's6' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
Null pointer is obviously well-aligned, so this is a false positive / incorrect test (though for a syntactic check that focuses on a single statement it's a perfectly normal test).
// bad (just check for pointer to pointer). short **s7; ::new (*s7) long; // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's7' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
There's another checker that warns here: warning: Dereference of undefined pointer value. So the analysis is interrupted before new is reached.
// bad (buffer is aligned to 'short' instead of 'long'). alignas(short) unsigned char buffer2[sizeof(long)]; ::new (buffer2) long; // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'buffer2' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
Hmm, i think we don't have an insufficient alignment check yet, only an insufficient storage check. Same goes for all of the test3(). These shouldn't be too hard to add so that, at least, to pass the missing tests.
short* test4_f1() { return nullptr; } void test4() { ::new (test4_f1()) long; // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'test4_f1' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
Again, the static analyzer is inter-procedural so it knows that it's a null pointer.
short* (*p1)(); p1 = test4_f1; ::new (p1()) long; // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'p1' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
We can even see through your obfuscation via function pointer! It's still null.
struct S { short a; }; // bad (not enough space). const unsigned N = 32; alignas(S) unsigned char buffer1[sizeof(S) * N]; ::new (buffer1) S[N]; // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 64 bytes are not enough for array allocation which requires 64 bytes [cert-mem54-cpp] // maybe ok but we need to warn. alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)]; ::new (buffer2) S[N]; // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: possibly not enough 68 bytes for array allocation which requires 64 bytes. current overhead requires the size of 4 bytes [cert-mem54-cpp]
Ah yes, placement array-new, we're still missing it but it shouldn't be hard to add.
Comment Actions
NoQ, I don`t want to say that existing static alalyzer is bad written or something. I just noticed that it does not yet support arrays and alignment.
Anyway thank you for your detailed feedback.
Comment Actions
To getting started with the Analyzer please visit @NoQ's (tiny bit outdated) booklet: https://github.com/haoNoQ/clang-analyzer-guide
Advertisement:
If D69726 lands we will have an arsenal of APIs for helping dynamic size based checkers.
If D73521 lands it should be easier to getting started with the Analyzer, but now you could visit what is our current API.
Comment Actions
Please note about the alignment requirements that they are not checked in the static analyzer for a reason. To track the alignment data across the data flow, probably we should modify the analyzer's core structure (e.g. MemRegion) and some modeling checkers like the MallocChecker. That is not an impossible task but also far from easy IMHO (would require several patches built on top of each other). Just as an example for the subtleties: think about the nuances of operator news default alignment and specific C++17 rules (https://reviews.llvm.org/D67545).
Comment Actions
This checker's tests only cover VarRegion-based regions. It should be easy to pick up the alignment from the AST for the VarDecl it corresponds to.