[Analyzer] Placement new related checker

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[Analyzer] Placement new related checker

David Zarzycki via cfe-dev
Hi,

I am working on a SEI CERT checker (Provide placement new with properly aligned pointers to sufficient storage capacity). This should give reports on the following:

#include <new>
void f() {
  short s;
  long *lp = ::new (&s) long; // warning: insufficient storage
}

First I thought, the best place for the implementation would be in the existing MallocChecker.cpp.
However, there are a bunch of classes (e.g. the MallocBugVisitor) which seems to be unrelated and not needed for this new check. Now I am fidgeting because maybe it would be better to have a self-contained independent .cpp file for the implementation. What do you think, what do you suggest?

Thanks,
Gabor

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [Analyzer] Placement new related checker

David Zarzycki via cfe-dev
Hey!

A new idea is rising nowadays to split the checkers into two main
parts: one for modeling, one for checking for certain issues. In this
new form the MallocChecker.cpp needs to only contain the modeling. I
like the idea of having a separated checker for checking.


On Sat, Dec 7, 2019 at 4:58 PM Gábor Márton via cfe-dev
<[hidden email]> wrote:

>
> Hi,
>
> I am working on a SEI CERT checker (Provide placement new with properly aligned pointers to sufficient storage capacity). This should give reports on the following:
>
> #include <new>
> void f() {
>   short s;
>   long *lp = ::new (&s) long; // warning: insufficient storage
> }
>
> First I thought, the best place for the implementation would be in the existing MallocChecker.cpp.
> However, there are a bunch of classes (e.g. the MallocBugVisitor) which seems to be unrelated and not needed for this new check. Now I am fidgeting because maybe it would be better to have a self-contained independent .cpp file for the implementation. What do you think, what do you suggest?
>
> Thanks,
> Gabor
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [Analyzer] Placement new related checker

David Zarzycki via cfe-dev
I don't see much overlap between your checker and MallocChecker. You
simply subscribe for PreStmt<CXXNewExpr> and introspect the argument
region (check extent, maybe origins as well), possibly add a visitor to
track it. The only thing i see here is that you might want to add
MallocChecker's visitor to highlight the allocation site for a heap
memory chunk (if you're placement-new-ing into a heap memory chunk) but
that visitor is already shared across multiple checkers; and even then,
you might be able to get away with trackExpressionValue() instead.

On 12/9/19 5:05 AM, Csaba Dabis wrote:

> Hey!
>
> A new idea is rising nowadays to split the checkers into two main
> parts: one for modeling, one for checking for certain issues. In this
> new form the MallocChecker.cpp needs to only contain the modeling. I
> like the idea of having a separated checker for checking.
>
>
> On Sat, Dec 7, 2019 at 4:58 PM Gábor Márton via cfe-dev
> <[hidden email]> wrote:
>> Hi,
>>
>> I am working on a SEI CERT checker (Provide placement new with properly aligned pointers to sufficient storage capacity). This should give reports on the following:
>>
>> #include <new>
>> void f() {
>>    short s;
>>    long *lp = ::new (&s) long; // warning: insufficient storage
>> }
>>
>> First I thought, the best place for the implementation would be in the existing MallocChecker.cpp.
>> However, there are a bunch of classes (e.g. the MallocBugVisitor) which seems to be unrelated and not needed for this new check. Now I am fidgeting because maybe it would be better to have a self-contained independent .cpp file for the implementation. What do you think, what do you suggest?
>>
>> Thanks,
>> Gabor
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [Analyzer] Placement new related checker

David Zarzycki via cfe-dev
Ok,

Thanks guys, then I am going to put this checker in its own implementation file.

Gabor

On Mon, Dec 9, 2019 at 9:54 PM Artem Dergachev <[hidden email]> wrote:
I don't see much overlap between your checker and MallocChecker. You
simply subscribe for PreStmt<CXXNewExpr> and introspect the argument
region (check extent, maybe origins as well), possibly add a visitor to
track it. The only thing i see here is that you might want to add
MallocChecker's visitor to highlight the allocation site for a heap
memory chunk (if you're placement-new-ing into a heap memory chunk) but
that visitor is already shared across multiple checkers; and even then,
you might be able to get away with trackExpressionValue() instead.

On 12/9/19 5:05 AM, Csaba Dabis wrote:
> Hey!
>
> A new idea is rising nowadays to split the checkers into two main
> parts: one for modeling, one for checking for certain issues. In this
> new form the MallocChecker.cpp needs to only contain the modeling. I
> like the idea of having a separated checker for checking.
>
>
> On Sat, Dec 7, 2019 at 4:58 PM Gábor Márton via cfe-dev
> <[hidden email]> wrote:
>> Hi,
>>
>> I am working on a SEI CERT checker (Provide placement new with properly aligned pointers to sufficient storage capacity). This should give reports on the following:
>>
>> #include <new>
>> void f() {
>>    short s;
>>    long *lp = ::new (&s) long; // warning: insufficient storage
>> }
>>
>> First I thought, the best place for the implementation would be in the existing MallocChecker.cpp.
>> However, there are a bunch of classes (e.g. the MallocBugVisitor) which seems to be unrelated and not needed for this new check. Now I am fidgeting because maybe it would be better to have a self-contained independent .cpp file for the implementation. What do you think, what do you suggest?
>>
>> Thanks,
>> Gabor
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev