r176612 - [analyzer] Warn on passing a reference to null pointer as an argument in a call
Anna Zaks
ganna at apple.com
Thu Mar 7 09:34:55 PST 2013
I ran the TOT analyzer over WebKit and looks like the null reference returned + null reference as argument changes from yesterday resulted in 45% of warnings being removed(226 ->148). I did not look at the diff, but most warnings were false positives to begin with, so this probably means that our suppression mechanisms/bug reporter did not track references to null pointers correctly. (This also explains why spurious "null pointer dereferences" were more of a problem for C++ codebases.)
Cheers,
Anna.
On Mar 6, 2013, at 7:09 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> For the record, it's unclear whether the standard actually allows this. Null lvalues seem to be allowed, while null references seem to be disallowed, but neither is definitively in or out.
>
> http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#232
>
> In the analyzer, as Anna says, we're going to take a hard line and guess people would rather be warned.
>
>
> On Mar 6, 2013, at 19:02 , Anna Zaks <ganna at apple.com> wrote:
>
>> Author: zaks
>> Date: Wed Mar 6 21:02:36 2013
>> New Revision: 176612
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=176612&view=rev
>> Log:
>> [analyzer] Warn on passing a reference to null pointer as an argument in a call
>>
>> Warn about null pointer dereference earlier when a reference to a null pointer is
>> passed in a call. The idea is that even though the standard might allow this, reporting
>> the issue earlier is better for diagnostics (the error is reported closer to the place where
>> the pointer was set to NULL). This also simplifies analyzer’s diagnostic logic, which has
>> to track “where the null came from”. As a consequence, some of our null pointer
>> warning suppression mechanisms started triggering more often.
>>
>> TODO: Change the name of the file and class to reflect the new check.
>>
>> Modified:
>> cfe/trunk/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp
>> cfe/trunk/test/Analysis/initializer.cpp
>> cfe/trunk/test/Analysis/reference.cpp
>>
>> Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp?rev=176612&r1=176611&r2=176612&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp (original)
>> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp Wed Mar 6 21:02:36 2013
>> @@ -26,10 +26,16 @@ using namespace ento;
>> namespace {
>> class AttrNonNullChecker
>> : public Checker< check::PreCall > {
>> - mutable OwningPtr<BugType> BT;
>> + mutable OwningPtr<BugType> BTAttrNonNull;
>> + mutable OwningPtr<BugType> BTNullRefArg;
>> public:
>>
>> void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
>> +
>> + BugReport *genReportNullAttrNonNull(const ExplodedNode *ErrorN,
>> + const Expr *ArgE) const;
>> + BugReport *genReportReferenceToNullPointer(const ExplodedNode *ErrorN,
>> + const Expr *ArgE) const;
>> };
>> } // end anonymous namespace
>>
>> @@ -40,26 +46,38 @@ void AttrNonNullChecker::checkPreCall(co
>> return;
>>
>> const NonNullAttr *Att = FD->getAttr<NonNullAttr>();
>> - if (!Att)
>> - return;
>>
>> ProgramStateRef state = C.getState();
>>
>> - // Iterate through the arguments of CE and check them for null.
>> - for (unsigned idx = 0, count = Call.getNumArgs(); idx != count; ++idx) {
>> - if (!Att->isNonNull(idx))
>> + CallEvent::param_type_iterator TyI = Call.param_type_begin(),
>> + TyE = Call.param_type_end();
>> +
>> + for (unsigned idx = 0, count = Call.getNumArgs(); idx != count; ++idx){
>> +
>> + // Check if the parameter is a reference. We want to report when reference
>> + // to a null pointer is passed as a paramter.
>> + bool haveRefTypeParam = false;
>> + if (TyI != TyE) {
>> + haveRefTypeParam = (*TyI)->isReferenceType();
>> + TyI++;
>> + }
>> +
>> + bool haveAttrNonNull = Att && Att->isNonNull(idx);
>> +
>> + if (!haveRefTypeParam && !haveAttrNonNull)
>> continue;
>>
>> + // If the value is unknown or undefined, we can't perform this check.
>> + const Expr *ArgE = Call.getArgExpr(idx);
>> SVal V = Call.getArgSVal(idx);
>> Optional<DefinedSVal> DV = V.getAs<DefinedSVal>();
>> -
>> - // If the value is unknown or undefined, we can't perform this check.
>> if (!DV)
>> continue;
>>
>> - const Expr *ArgE = Call.getArgExpr(idx);
>> -
>> - if (!DV->getAs<Loc>()) {
>> + // Process the case when the argument is not a location.
>> + assert(!haveRefTypeParam || DV->getAs<Loc>());
>> +
>> + if (haveAttrNonNull && !DV->getAs<Loc>()) {
>> // If the argument is a union type, we want to handle a potential
>> // transparent_union GCC extension.
>> if (!ArgE)
>> @@ -100,21 +118,15 @@ void AttrNonNullChecker::checkPreCall(co
>> // we cache out.
>> if (ExplodedNode *errorNode = C.generateSink(stateNull)) {
>>
>> - // Lazily allocate the BugType object if it hasn't already been
>> - // created. Ownership is transferred to the BugReporter object once
>> - // the BugReport is passed to 'EmitWarning'.
>> - if (!BT)
>> - BT.reset(new BugType("Argument with 'nonnull' attribute passed null",
>> - "API"));
>> -
>> - BugReport *R =
>> - new BugReport(*BT, "Null pointer passed as an argument to a "
>> - "'nonnull' parameter", errorNode);
>> + BugReport *R = 0;
>> + if (haveAttrNonNull)
>> + R = genReportNullAttrNonNull(errorNode, ArgE);
>> + else if (haveRefTypeParam)
>> + R = genReportReferenceToNullPointer(errorNode, ArgE);
>>
>> // Highlight the range of the argument that was null.
>> R->addRange(Call.getArgSourceRange(idx));
>> - if (ArgE)
>> - bugreporter::trackNullOrUndefValue(errorNode, ArgE, *R);
>> +
>> // Emit the bug report.
>> C.emitReport(R);
>> }
>> @@ -134,6 +146,45 @@ void AttrNonNullChecker::checkPreCall(co
>> C.addTransition(state);
>> }
>>
>> +BugReport *AttrNonNullChecker::genReportNullAttrNonNull(
>> + const ExplodedNode *ErrorNode, const Expr *ArgE) const {
>> + // Lazily allocate the BugType object if it hasn't already been
>> + // created. Ownership is transferred to the BugReporter object once
>> + // the BugReport is passed to 'EmitWarning'.
>> + if (!BTAttrNonNull)
>> + BTAttrNonNull.reset(new BugType(
>> + "Argument with 'nonnull' attribute passed null",
>> + "API"));
>> +
>> + BugReport *R = new BugReport(*BTAttrNonNull,
>> + "Null pointer passed as an argument to a 'nonnull' parameter",
>> + ErrorNode);
>> + if (ArgE)
>> + bugreporter::trackNullOrUndefValue(ErrorNode, ArgE, *R);
>> +
>> + return R;
>> +}
>> +
>> +BugReport *AttrNonNullChecker::genReportReferenceToNullPointer(
>> + const ExplodedNode *ErrorNode, const Expr *ArgE) const {
>> + if (!BTNullRefArg)
>> + BTNullRefArg.reset(new BuiltinBug("Dereference of null pointer"));
>> +
>> + BugReport *R = new BugReport(*BTNullRefArg,
>> + "Forming reference to null pointer",
>> + ErrorNode);
>> + if (ArgE) {
>> + const Expr *ArgEDeref = bugreporter::getDerefExpr(ArgE);
>> + if (ArgEDeref == 0)
>> + ArgEDeref = ArgE;
>> + bugreporter::trackNullOrUndefValue(ErrorNode,
>> + ArgEDeref,
>> + *R);
>> + }
>> + return R;
>> +
>> +}
>> +
>> void ento::registerAttrNonNullChecker(CheckerManager &mgr) {
>> mgr.registerChecker<AttrNonNullChecker>();
>> }
>>
>> Modified: cfe/trunk/test/Analysis/initializer.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/initializer.cpp?rev=176612&r1=176611&r2=176612&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/Analysis/initializer.cpp (original)
>> +++ cfe/trunk/test/Analysis/initializer.cpp Wed Mar 6 21:02:36 2013
>> @@ -68,8 +68,7 @@ void testReferenceMember() {
>>
>> void testReferenceMember2() {
>> int *p = 0;
>> - // FIXME: We should warn here, since we're creating the reference here.
>> - RefWrapper X(*p); // expected-warning at -12 {{Dereference of null pointer}}
>> + RefWrapper X(*p); // expected-warning {{Forming reference to null pointer}}
>> }
>>
>>
>>
>> Modified: cfe/trunk/test/Analysis/reference.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/reference.cpp?rev=176612&r1=176611&r2=176612&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/Analysis/reference.cpp (original)
>> +++ cfe/trunk/test/Analysis/reference.cpp Wed Mar 6 21:02:36 2013
>> @@ -149,16 +149,78 @@ void testReturnReference() {
>> clang_analyzer_eval(&refFromPointer() == 0); // expected-warning{{FALSE}}
>> }
>>
>> +void intRefParam(int &r) {
>> + ;
>> +}
>> +
>> +void test(int *ptr) {
>> + clang_analyzer_eval(ptr == 0); // expected-warning{{UNKNOWN}}
>> +
>> + extern void use(int &ref);
>> + use(*ptr);
>> +
>> + clang_analyzer_eval(ptr == 0); // expected-warning{{FALSE}}
>> +}
>> +
>> +void testIntRefParam() {
>> + int i = 0;
>> + intRefParam(i); // no-warning
>> +}
>>
>> -// ------------------------------------
>> -// False negatives
>> -// ------------------------------------
>> +int refParam(int &byteIndex) {
>> + return byteIndex;
>> +}
>> +
>> +void testRefParam(int *p) {
>> + if (p)
>> + ;
>> + refParam(*p); // expected-warning {{Forming reference to null pointer}}
>> +}
>> +
>> +int ptrRefParam(int *&byteIndex) {
>> + return *byteIndex; // expected-warning {{Dereference of null pointer}}
>> +}
>> +void testRefParam2() {
>> + int *p = 0;
>> + int *&rp = p;
>> + ptrRefParam(rp);
>> +}
>> +
>> +int *maybeNull() {
>> + extern bool coin();
>> + static int x;
>> + return coin() ? &x : 0;
>> +}
>> +
>> +void use(int &x) {
>> + x = 1; // no-warning
>> +}
>> +
>> +void testSuppression() {
>> + use(*maybeNull());
>> +}
>>
>> namespace rdar11212286 {
>> class B{};
>>
>> B test() {
>> B *x = 0;
>> - return *x; // should warn here!
>> + return *x; // expected-warning {{Forming reference to null pointer}}
>> + }
>> +
>> + B testif(B *x) {
>> + if (x)
>> + ;
>> + return *x; // expected-warning {{Forming reference to null pointer}}
>> + }
>> +
>> + void idc(B *x) {
>> + if (x)
>> + ;
>> + }
>> +
>> + B testidc(B *x) {
>> + idc(x);
>> + return *x; // no-warning
>> }
>> }
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130307/362b9bf2/attachment.html>
More information about the cfe-commits
mailing list