Page MenuHomePhabricator

[clang-tidy] Added PlacementNewStorageCheck
Needs RevisionPublic

Authored by f00kat on Mar 16 2020, 5:57 AM.

Details

Summary

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 :)

Diff Detail

Unit TestsFailed

TimeTest
110 msClang Tools.clang-tidy/checkers::cert-mem54-cpp.cpp
Script: -- : 'RUN: at line 1'; /usr/bin/python /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang-tools-extra/test/clang-tidy/checkers/cert-mem54-cpp.cpp cert-mem54-cpp /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/tools/clang/tools/extra/test/clang-tidy/checkers/Output/cert-mem54-cpp.cpp.tmp
220 msClang.Analysis::checker-plugins.c
Script: -- : 'RUN: at line 7'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/clang/11.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -verify /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/Analysis/checker-plugins.c -load /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./lib/SampleAnalyzerPlugin.so -analyzer-checker='example.MainCallChecker'

Event Timeline

f00kat created this revision.Mar 16 2020, 5:57 AM
Eugene.Zelenko added inline comments.
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.

f00kat updated this revision to Diff 250555.Mar 16 2020, 7:14 AM
  1. Static functions instead of anonymous namespaces.
  2. const auto*.
  3. Added double back-ticks.
f00kat updated this revision to Diff 250556.Mar 16 2020, 7:16 AM

Fix 'const auto*' in function getDescendantValueDecl

Eugene.Zelenko added inline comments.Mar 16 2020, 7:47 AM
clang-tools-extra/clang-tidy/cert/PlacementNewStorageCheck.cpp
43

Unnecessary empty line.

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

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.

lebedev.ri requested changes to this revision.Mar 16 2020, 2:18 PM
lebedev.ri added a subscriber: lebedev.ri.

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;
  ^

https://godbolt.org/z/9VX5WW

This revision now requires changes to proceed.Mar 16 2020, 2:18 PM

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.

I have never try to write static analyzer before. Okay, I will look at examples of how to work with dataflow.

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;
  ^

https://godbolt.org/z/9VX5WW

Oh no..It is sad :)

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;
  ^

https://godbolt.org/z/9VX5WW

But it seems like not all of my tests pass on static analyzer?

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;
  ^

https://godbolt.org/z/9VX5WW

But it seems like not all of my tests pass on static analyzer?

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?

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;
  ^

https://godbolt.org/z/9VX5WW

But it seems like not all of my tests pass on static analyzer?

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?

Yeah. You are right. I will try to improve exist checker.
What we gonna do with this patch? Remove it?

NoQ added a reviewer: martong.Mar 17 2020, 4:06 AM

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.

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.

NoQ added a comment.Mar 17 2020, 7:59 AM

Sure np, was just curious^^

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.

I have never try to write static analyzer before. Okay, I will look at examples of how to work with dataflow.

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.

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.

I have never try to write static analyzer before. Okay, I will look at examples of how to work with dataflow.

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.

Thank you!

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).

NoQ added a comment.Mar 23 2020, 8:35 PM

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).

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.