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 <[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
