[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 18:11:48 PST 2009
2009/11/10 Ted Kremenek <kremenek at apple.com>:
> I wasn't arguing that we make the check malloc() specific, and I don't think
> we should because the problem is more general than just looking at pointers
> returned from malloc(). We just need to make it smart enough that when it
> gives a warning, it is:
>
> (a) correct most of the time
>
> (b) gives a meaningful diagnostic that people understand how they screwed up
> and how they can fix the issue
>
> I don't think we need to make it malloc() specific, but we can educate the
> check about malloc() if that helps accomplish (a) and (b).
Make sense. Let's start with malloc. I'm fine to remove this too general check.
>
> On Nov 9, 2009, at 5:55 PM, Zhongxing Xu wrote:
>
>> 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