Quantcast

[clang-tidy] Check for paranoic code (pointer checking)

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[clang-tidy] Check for paranoic code (pointer checking)

Daniel Marjamäki via cfe-dev
Hi all,

I started exploring clang-tidy to create checks for a large C++ code base we have at work, with many developers with C background doing C++.

After doing the first tutorial on VirtualShadowingCheck, I wrote a check to detect paranoic code using pointers. By paranoic, I mean this:

static void foo(T* p, ...) {
    if (!p) return;
    // do something
}

The method is doing the right thing to check the input, but the interface itself could've been written with a reference, thus delegating to the caller the responsability of doing the null check.

This is not at all always possible, since this style can be valid for several reasons: pointers can refer diverse conceptual objects, e.g. arrays), the function could be virtual, thus forced to follow the same parameters, and so on.

But still, controversial as it is, I ran this check on LLVM code base.

llvm/tools/clang/lib/Sema/SemaLookup.cpp:3453 is one example. (the check finds over 15 violations, but I haven't analyzed them all)

This function is one example that could be ignoring a potential mistake. The caller could expect a valid pointer in at least some of those calls. The convenience of writing:

foo(something->getPointer(), ...);

can stimulate the developer to not write checks/assertions for some cases that he/she expects the pointer to be valid.

All that being said, I have some questions:

1 - Is the community receptive to such controversial checks? Or is clang-tidy only interested in low false positive/ non controversial checks?
2 - Any feedback on this specific check?
3 - Are there more check ideas waiting for someone to implement so I could pick up, learn more and actually contribute to the code base?


Thanks,
Breno Rodrigues Guimarães




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

Re: [clang-tidy] Check for paranoic code (pointer checking)

Daniel Marjamäki via cfe-dev

Hello!

 

I agree it’s controversial. I would say that some people would like that and others don’t. In my humble opinion it should be ok to add this check to clang-tidy.

 

Possibly to avoid some spurious warnings you could warn for const pointers only. I personally think that parameters that are written should be passed by pointer “getvalue(&x)” is more explicit imho than “getvalue(x)”.

 

> 3 - Are there more check ideas waiting for someone to implement so I could pick up, learn more and actually contribute to the code base?

 

I don’t know if there is an official list anywhere.

 

But I would assume there are many ideas.

 

Personally I would like that more types of UB is detected. Here is some testcode with various UB, Clang detects some of the UB but not all:

 

void alias() { int x; int *ip=&x; float *fp = (float *)ip; }

 

int buffer_overflow() { int x[10]={0}; return x[100]; }

 

int dead_pointer(int a) { int *p=&a; if (a) { int x=0; p = &x; } return *p; }

 

int division_by_zero() { return 100 / 0; }

 

int float_to_int() { double d=1E100; return (int)d; }

 

void negative_size(int sz) { if (sz < 0) { int buf[sz]; } }

 

int no_return() {}

 

int null_pointer() { int *p = 0; return *p; }

 

int *pointer_arithmetic() { static int buf[10]; return buf + 100; }

 

unsigned char pointer_to_u8() { static int buf[10]; return (int*)buf; }

 

int pointer_subtraction() { char a[10]; char b[10]; return b-a; }

 

int pointer_comparison() { char a[10]; char b[10]; return b<a; }

 

int shift_overrun(int x) { return x << 123; }

 

int shift_negative() { return -1 << 1; }

 

int int_overflow() { int intmax = (~0U) >> 1; return intmax * 2; }

 

void string_literal() { *((char *)"hello") = 0; }

 

int uninit() { int x; return x + 2; }

 

 

Best regards,

Daniel Marjamäki

 

 

From: cfe-dev [[hidden email]] On Behalf Of Breno Guimarães via cfe-dev
Sent: Thursday, March 16, 2017 5:55 AM
To: [hidden email]
Subject: [cfe-dev] [clang-tidy] Check for paranoic code (pointer checking)

 

Hi all,

 

I started exploring clang-tidy to create checks for a large C++ code base we have at work, with many developers with C background doing C++.

 

After doing the first tutorial on VirtualShadowingCheck, I wrote a check to detect paranoic code using pointers. By paranoic, I mean this:

 

static void foo(T* p, ...) {
    if (!p) return;
    // do something
}

 

The method is doing the right thing to check the input, but the interface itself could've been written with a reference, thus delegating to the caller the responsability of doing the null check.

This is not at all always possible, since this style can be valid for several reasons: pointers can refer diverse conceptual objects, e.g. arrays), the function could be virtual, thus forced to follow the same parameters, and so on.

 

But still, controversial as it is, I ran this check on LLVM code base.

llvm/tools/clang/lib/Sema/SemaLookup.cpp:3453 is one example. (the check finds over 15 violations, but I haven't analyzed them all)

 

This function is one example that could be ignoring a potential mistake. The caller could expect a valid pointer in at least some of those calls. The convenience of writing:

 

foo(something->getPointer(), ...);

 

can stimulate the developer to not write checks/assertions for some cases that he/she expects the pointer to be valid.

 

All that being said, I have some questions:

 

1 - Is the community receptive to such controversial checks? Or is clang-tidy only interested in low false positive/ non controversial checks?

2 - Any feedback on this specific check?

3 - Are there more check ideas waiting for someone to implement so I could pick up, learn more and actually contribute to the code base?

 

 

Thanks,

Breno Rodrigues Guimarães

 

 

 


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

Re: [clang-tidy] Check for paranoic code (pointer checking)

Daniel Marjamäki via cfe-dev
In reply to this post by Daniel Marjamäki via cfe-dev
Hello,

i think this check should be in clang-tidy. More checks are better and one should configure clang-tidy in my opinion.

On the topic of other ideas for checks:

- Yes. There is a buglist with open ideas.
- There is currently work on a new `safety`-module that tries to implement parts of the High Integrity C++ Codingstandard and other usefull checks for code safety.
 -> my personal TODO List on that: https://github.com/JonasToth/HighIntegrityTooling
- Other Coding Standards have there part as well
- the CppCoreGuidelines are IMHO an interesting topic as well.

You can look at each module and try start with simpler checks if you want. I think there is a lot of low hanging fruit left.
But consider looking for already existing checks and modify them, make them configurable and so on :)

All the best. Jonas

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

Re: [clang-tidy] Check for paranoic code (pointer checking)

Daniel Marjamäki via cfe-dev
In reply to this post by Daniel Marjamäki via cfe-dev
On Thu, Mar 16, 2017 at 12:54 AM, Breno Guimarães via cfe-dev
<[hidden email]> wrote:

> Hi all,
>
> I started exploring clang-tidy to create checks for a large C++ code base we
> have at work, with many developers with C background doing C++.
>
> After doing the first tutorial on VirtualShadowingCheck, I wrote a check to
> detect paranoic code using pointers. By paranoic, I mean this:
>
> static void foo(T* p, ...) {
>     if (!p) return;
>     // do something
> }
>
> The method is doing the right thing to check the input, but the interface
> itself could've been written with a reference, thus delegating to the caller
> the responsability of doing the null check.
>
> This is not at all always possible, since this style can be valid for
> several reasons: pointers can refer diverse conceptual objects, e.g.
> arrays), the function could be virtual, thus forced to follow the same
> parameters, and so on.
>
> But still, controversial as it is, I ran this check on LLVM code base.
>
> llvm/tools/clang/lib/Sema/SemaLookup.cpp:3453 is one example. (the check
> finds over 15 violations, but I haven't analyzed them all)
>
> This function is one example that could be ignoring a potential mistake. The
> caller could expect a valid pointer in at least some of those calls. The
> convenience of writing:
>
> foo(something->getPointer(), ...);
>
> can stimulate the developer to not write checks/assertions for some cases
> that he/she expects the pointer to be valid.
>
> All that being said, I have some questions:
>
> 1 - Is the community receptive to such controversial checks? Or is
> clang-tidy only interested in low false positive/ non controversial checks?

Clang-tidy checks can have a higher false-positive rate than, say,
compiler warnings, but only up to a point. We tend to avoid checks
that are overly chatty where developers are apt to want to disable
them by default.

> 2 - Any feedback on this specific check?

Having a check that blindly suggests removing checks for pointer
validity seems like a low-value check. Those pointer checks may or may
not be valid -- if they are not valid, the check is useful, but if
they are valid, the check is now suggesting something *very* dangerous
to the user. This means for each diagnostic, the user has to do a lot
of work to figure out whether the diagnostic is a false-positive or
not.

Adding some intelligence to the check so that it suggests removing the
validity check, or suggests replacing the pointer with a reference,
when sensible to do so would make this a much better check IMO.
However, I'm not certain it's appropriate for a clang-tidy check due
to it being path sensitive in nature. It seems like a more natural fit
for a static analyzer check if going that route.

> 3 - Are there more check ideas waiting for someone to implement so I could
> pick up, learn more and actually contribute to the code base?

One good place to start is with the checks for specific coding
standards. We currently have a module for CERT secure coding
guidelines (C and C++) and one for the C++ Core Guidelines. We're also
in the process of starting to add checks for the High Integrity C++
coding standard. The nice thing about adding checks for existing
coding standards is that the checks have a reasonably good
specification for what needs to be checked, and a built-in
justification for why it needs to be checked.

~Aaron

>
>
> Thanks,
> Breno Rodrigues Guimarães
>
>
>
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [clang-tidy] Check for paranoic code (pointer checking)

Daniel Marjamäki via cfe-dev

[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <[hidden email]>,
    Aaron Ballman via cfe-dev <[hidden email]> writes:

> On Thu, Mar 16, 2017 at 12:54 AM, Breno Guimarães via cfe-dev
> <[hidden email]> wrote:
>
> > 2 - Any feedback on this specific check?
>
> Adding some intelligence to the check so that it suggests removing the
> validity check, or suggests replacing the pointer with a reference,
> when sensible to do so would make this a much better check IMO.

One very common case from programmers with a C background is to check
the result of new against NULL/nullptr/0.  Other static analyzers like
cppcheck have such a check already.  If clang/clang-tidy doesn't
already warn on code like this, that would be a good addition.

Another check in the same spirit is where code checks for non-nullptr
at some point and then checks against nullptr again.  This usually
happens in large functions where the earlier check against nullptr is
"out of sight, out of mind" when someone is adding the second check
against nullptr.

Again, this would be a welcome addition to clang/clang-tidy if we
don't already have something like that.
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
            The Terminals Wiki <http://terminals-wiki.org>
     The Computer Graphics Museum <http://ComputerGraphicsMuseum.org>
  Legalize Adulthood! (my blog) <http://LegalizeAdulthood.wordpress.com>

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

Re: [clang-tidy] Check for paranoic code (pointer checking)

Daniel Marjamäki via cfe-dev
Hi guys,

Thanks for all the suggestions! I'm starting to run my check in our code base (around 2M C++ lines) and the ratio of good findings and noise.

But as many of you said, there are already several well agreed guidelines that can be checked, and I will start picking some of them and produce the checks. Hopefully I can contribute very soon (I can only play with this at nights and weekends, but still...).

I also liked the idea of adding configurations and tweaks to existing checks. That's a good way to see how the APIs are used and what's available.

I appreciate the responses!

Best regards,
Breno G.



On Thu, Mar 16, 2017 at 6:51 PM, Richard via cfe-dev <[hidden email]> wrote:

[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <[hidden email]>,
    Aaron Ballman via cfe-dev <[hidden email]> writes:

> On Thu, Mar 16, 2017 at 12:54 AM, Breno Guimarães via cfe-dev
> <[hidden email]> wrote:
>
> > 2 - Any feedback on this specific check?
>
> Adding some intelligence to the check so that it suggests removing the
> validity check, or suggests replacing the pointer with a reference,
> when sensible to do so would make this a much better check IMO.

One very common case from programmers with a C background is to check
the result of new against NULL/nullptr/0.  Other static analyzers like
cppcheck have such a check already.  If clang/clang-tidy doesn't
already warn on code like this, that would be a good addition.

Another check in the same spirit is where code checks for non-nullptr
at some point and then checks against nullptr again.  This usually
happens in large functions where the earlier check against nullptr is
"out of sight, out of mind" when someone is adding the second check
against nullptr.

Again, this would be a welcome addition to clang/clang-tidy if we
don't already have something like that.
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
            The Terminals Wiki <http://terminals-wiki.org>
     The Computer Graphics Museum <http://ComputerGraphicsMuseum.org>
  Legalize Adulthood! (my blog) <http://LegalizeAdulthood.wordpress.com>

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




--
Breno Rodrigues Guimarães
Universidade Federal de Minas Gerais - UFMG, Brasil
(Federal University of Minas Gerais, Brazil)



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