[cfe-commits] r86464 - in /cfe/trunk: include/clang/Analysis/LocalCheckers.h include/clang/Frontend/Analyses.def lib/Analysis/CheckSizeofPointer.cpp lib/Frontend/AnalysisConsumer.cpp
Zhongxing Xu
xuzhongxing at gmail.com
Mon Nov 9 17:55:50 PST 2009
I agree to all these points.
For this specific checker, if people want to know the size of a
malloc'ed buffer, they shouldn't use sizeof() at all. Is this the
reason we should make this check specific about 'malloc'? And allow
other cases of using sizeof() on pointers.
2009/11/10 Ted Kremenek <kremenek at apple.com>:
> 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