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
