2009/11/10 Ted Kremenek <[email protected]>: > 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 <[email protected]>: >>> >>> 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 <[email protected]>: >>>>> >>>>> 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 >>>>>> [email protected] >>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>> >>>>> >>> >>> > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
