[cfe-commits] r86464 - in /cfe/trunk: include/clang/Analysis/LocalCheckers.h include/clang/Frontend/Analyses.def lib/Analysis/CheckSizeofPointer.cpp lib/Frontend/AnalysisConsumer.cpp
Ted Kremenek
kremenek at apple.com
Mon Nov 9 17:40:54 PST 2009
Hi Zhongxing,
I think there is an overall design question about what we desire in
checks.
To me, we should aspire to have most checks turned on by default.
This imposes two requirements. First, a low false positive rate
(ideally less than 10-20%). The second requirement is that we are
warning about something that people care about. That is, we aren't
just telling them their code needs to be fixed, but that they will go
and fix it. Finding problems is nice, but fixing bugs is what is
important.
With that perspective, even if it is a lightweight check that requires
a separate option, nobody will use it if it has too many false
positives. Ultimately, if nobody uses it, what's the point of having
it? I also think the diagnostic should discuss the actual problem.
The problem isn't that taking sizeof(pointer) is an illegal operation,
its that people taking sizeof(pointer) mistakenly believe they are
getting the size of the buffer. Having a clear explanation and having
razor sharp precision with the warning itself will make people trust
the warning and go and fix their code. That's the desired result. If
the needed precision requires this check being path-sensitive, then
that's what we should do.
That said, if the flow-insensitive version does the job, I'm happy.
For me a check is only worth adding if we can deliver a high-quality
experience to the user. Mediocre checks detract from the awesome ones
because there is extra layers of crap that people have to wade through
to get to the good results. The checks we turn on by default are the
ones that we are saying "here are a list of issues reported by the
analyzer, you should go and fix all of them." For the checks that
don't fall into that category, my question is why didn't they? Is it
because we didn't make the check smart enough, or is it that the check
isn't that useful. For the former, we can provide an initial
implementation of the check that is mediocre and gradually make it
better. For the latter, we shouldn't implement it at all.
Thoughts?
On Nov 9, 2009, at 4:16 PM, Zhongxing Xu wrote:
> I later modified the diagnostic. I admit this could produce a lot of
> false positives. But I think checking for malloced pointer does not
> make much more sense than checking all pointers. This version is very
> fast, and is triggered by a separate command line option, so it can be
> treated like a very light weight check, similar to a syntactic check.
> Is this reasonable?
>
> 2009/11/10 Ted Kremenek <kremenek at apple.com>:
>> Hi Zhongxing,
>>
>> I'm a little concerned about the promiscuity of this check in
>> general, as it
>> seems like it could produce a lot of false positives. Have you tried
>> running this check over several real-world projects to get a feel
>> on the
>> false positive rate (or even the number of times this check
>> triggers)?
>>
>> It also seems to me that this check needs to be flow/path
>> sensitive. The
>> diagnostic says that the pointer is "malloced", but in truth the
>> check will
>> fire when taking the sizeof of any pointer expression. In order
>> for the
>> check to be accurate, it seems like we would need to track if
>> symbol was
>> actually returned from malloc() and then see if it is passed to
>> sizeof().
>> This seems possible by adding a new PreVisit method for
>> SizeOfAlignOfExpr,
>> and checking if the SymbolicRegion wraps a conjured symbol that is
>> associated with a CallExpr for malloc(). That would make the check
>> more
>> precise, and at least the diagnostic would match with the actual
>> code.
>>
>> What do you think?
>>
>> Ted
>>
>> On Nov 8, 2009, at 5:10 AM, Zhongxing Xu wrote:
>>
>>> Author: zhongxingxu
>>> Date: Sun Nov 8 07:10:34 2009
>>> New Revision: 86464
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=86464&view=rev
>>> Log:
>>> Add a checker for CWE-467: Use of sizeof() on a Pointer Type.
>>>
>>> Added:
>>> cfe/trunk/lib/Analysis/CheckSizeofPointer.cpp
>>> Modified:
>>> cfe/trunk/include/clang/Analysis/LocalCheckers.h
>>> cfe/trunk/include/clang/Frontend/Analyses.def
>>> cfe/trunk/lib/Frontend/AnalysisConsumer.cpp
>>>
>>> Modified: cfe/trunk/include/clang/Analysis/LocalCheckers.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/LocalCheckers.h?rev=86464&r1=86463&r2=86464&view=diff
>>>
>>>
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> ====================================================================
>>> --- cfe/trunk/include/clang/Analysis/LocalCheckers.h (original)
>>> +++ cfe/trunk/include/clang/Analysis/LocalCheckers.h Sun Nov 8
>>> 07:10:34
>>> 2009
>>> @@ -53,7 +53,7 @@
>>>
>>> void CheckSecuritySyntaxOnly(const Decl *D, BugReporter &BR);
>>>
>>> -
>>> +void CheckSizeofPointer(const Decl *D, BugReporter &BR);
>>> } // end namespace clang
>>>
>>> #endif
>>>
>>> Modified: cfe/trunk/include/clang/Frontend/Analyses.def
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/Analyses.def?rev=86464&r1=86463&r2=86464&view=diff
>>>
>>>
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> ====================================================================
>>> --- cfe/trunk/include/clang/Frontend/Analyses.def (original)
>>> +++ cfe/trunk/include/clang/Frontend/Analyses.def Sun Nov 8
>>> 07:10:34 2009
>>> @@ -48,6 +48,9 @@
>>> ANALYSIS(CheckerCFRef, "checker-cfref",
>>> "Run the [Core] Foundation reference count checker", Code)
>>>
>>> +ANALYSIS(WarnSizeofPointer, "warn-sizeof-pointer",
>>> + "Warn about unintended use of sizeof() on pointer
>>> expressions",
>>> Code)
>>> +
>>> ANALYSIS(InlineCall, "inline-call",
>>> "Experimental transfer function inling callees when its
>>> definition"
>>> " is available.", TranslationUnit)
>>>
>>> Added: cfe/trunk/lib/Analysis/CheckSizeofPointer.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CheckSizeofPointer.cpp?rev=86464&view=auto
>>>
>>>
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> ====================================================================
>>> --- cfe/trunk/lib/Analysis/CheckSizeofPointer.cpp (added)
>>> +++ cfe/trunk/lib/Analysis/CheckSizeofPointer.cpp Sun Nov 8
>>> 07:10:34 2009
>>> @@ -0,0 +1,58 @@
>>> +//==- CheckSizeofPointer.cpp - Check for sizeof on pointers ------
>>> *- C++
>>> -*-==//
>>> +//
>>> +// The LLVM Compiler Infrastructure
>>> +//
>>> +// This file is distributed under the University of Illinois Open
>>> Source
>>> +// License. See LICENSE.TXT for details.
>>> +//
>>>
>>> +//
>>> =
>>> =
>>> =
>>> ----------------------------------------------------------------------=
>>> ==//
>>> +//
>>> +// This file defines a check for unintended use of sizeof() on
>>> pointer
>>> +// expressions.
>>> +//
>>>
>>> +//
>>> =
>>> =
>>> =
>>> ----------------------------------------------------------------------=
>>> ==//
>>> +
>>> +#include "clang/Analysis/PathSensitive/BugReporter.h"
>>> +#include "clang/AST/StmtVisitor.h"
>>> +#include "clang/Analysis/LocalCheckers.h"
>>> +#include "llvm/Support/Compiler.h"
>>> +
>>> +using namespace clang;
>>> +
>>> +namespace {
>>> +class VISIBILITY_HIDDEN WalkAST : public StmtVisitor<WalkAST> {
>>> + BugReporter &BR;
>>> +
>>> +public:
>>> + WalkAST(BugReporter &br) : BR(br) {}
>>> + void VisitSizeOfAlignOfExpr(SizeOfAlignOfExpr *E);
>>> + void VisitStmt(Stmt *S) { VisitChildren(S); }
>>> + void VisitChildren(Stmt *S);
>>> +};
>>> +}
>>> +
>>> +void WalkAST::VisitChildren(Stmt *S) {
>>> + for (Stmt::child_iterator I = S->child_begin(), E = S->child_end
>>> ();
>>> I!=E; ++I)
>>> + if (Stmt *child = *I)
>>> + Visit(child);
>>> +}
>>> +
>>> +// CWE-467: Use of sizeof() on a Pointer Type
>>> +void WalkAST::VisitSizeOfAlignOfExpr(SizeOfAlignOfExpr *E) {
>>> + if (!E->isSizeOf())
>>> + return;
>>> +
>>> + QualType T = E->getTypeOfArgument();
>>> + if (T->isPointerType()) {
>>> + SourceRange R = E->getArgumentExpr()->getSourceRange();
>>> + BR.EmitBasicReport("Potential unintended use of sizeof() on
>>> pointer
>>> type",
>>> + "Logic",
>>> + "The code calls sizeof() on a malloced
>>> pointer
>>> type, which always returns the wordsize/8. This can produce an
>>> unexpected
>>> result if the programmer intended to determine how much memory has
>>> been
>>> allocated.",
>>> + E->getLocStart(), &R, 1);
>>> + }
>>> +}
>>> +
>>> +void clang::CheckSizeofPointer(const Decl *D, BugReporter &BR) {
>>> + WalkAST walker(BR);
>>> + walker.Visit(D->getBody());
>>> +}
>>>
>>> Modified: cfe/trunk/lib/Frontend/AnalysisConsumer.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/AnalysisConsumer.cpp?rev=86464&r1=86463&r2=86464&view=diff
>>>
>>>
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> ====================================================================
>>> --- cfe/trunk/lib/Frontend/AnalysisConsumer.cpp (original)
>>> +++ cfe/trunk/lib/Frontend/AnalysisConsumer.cpp Sun Nov 8
>>> 07:10:34 2009
>>> @@ -412,6 +412,11 @@
>>> CheckObjCInstMethSignature(cast<ObjCImplementationDecl>(D), BR);
>>> }
>>>
>>> +static void ActionWarnSizeofPointer(AnalysisManager &mgr, Decl
>>> *D) {
>>> + BugReporter BR(mgr);
>>> + CheckSizeofPointer(D, BR);
>>> +}
>>> +
>>> static void ActionInlineCall(AnalysisManager &mgr, Decl *D) {
>>> if (!D)
>>> return;
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
More information about the cfe-commits
mailing list