[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 18:06:42 PST 2009
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).
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