[StaticAnalyzer] Fix false negative on NilArgChecker when creating literal object

[tripleCC]

Summary

Fix false negative on NilArgChecker when creating literal object, such as @[nullableObject].

I don't have commit access.

[steakhal]

clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

151–164

This means that we would raise an issue if ArgSVal might be null. We usually warn if we are sure there is a bug, aka. if it must be null. Consequently, the condition should be rather StNull && !StNonNull instead of just StNull.

clang/test/Analysis/NSContainers.m

336–337

How about the case when it calls a Foo * getMightBeNullFoo();? I guess, it would still raise an issue, even though we couldn't prove that it must be null.

[steakhal]

clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

151–164

Ah I see now. My bad, thats the whole point of this :D

[tripleCC]

[StaticAnalyzer] add might be null test case for NilArgChecker

[tripleCC]

clang/test/Analysis/NSContainers.m

336–337

I have added the Foo * getMightBeNullFoo(); test case. It would not raise an issue actually because without tracked nullability, the NullabilityChecker does not produce warnings. You can check the logic in the checkEvent function for this part

[tripleCC]

clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

151–164

Not exactly, this meas that if ArgSVal might be null, we dispatch an "implicit" null event to NullabilityChecker. NullabilityChecker would emit a warning if the pointer is nullable in checkEvent function:

c++
/// This callback triggers when a pointer is dereferenced and the analyzer does
/// not know anything about the value of that pointer. When that pointer is
/// nullable, this code emits a warning.
void NullabilityChecker::checkEvent(ImplicitNullDerefEvent Event) const {
  if (Event.SinkNode->getState()->get<InvariantViolated>())
    return;

  const MemRegion *Region =
      getTrackRegion(Event.Location, /*CheckSuperRegion=*/true);
  if (!Region)
    return;

  ProgramStateRef State = Event.SinkNode->getState();
  const NullabilityState *TrackedNullability =
      State->get<NullabilityMap>(Region);

  if (!TrackedNullability)
    return;

  if (ChecksEnabled[CK_NullableDereferenced] &&
      TrackedNullability->getValue() == Nullability::Nullable) {
    BugReporter &BR = *Event.BR;
    // Do not suppress errors on defensive code paths, because dereferencing
    // a nullable pointer is always an error.
    if (Event.IsDirectDereference)
      reportBug("Nullable pointer is dereferenced",
                ErrorKind::NullableDereferenced, CK_NullableDereferenced,
                Event.SinkNode, Region, BR);
    else {
      reportBug("Nullable pointer is passed to a callee that requires a "
                "non-null",
                ErrorKind::NullablePassedToNonnull, CK_NullableDereferenced,
                Event.SinkNode, Region, BR);
    }
  }
}

[steakhal]

I had some improvement opportunities in mind scattered, so I decided to do them, and here is how the diff looks for me now:

recommendation.patch59 KBDownload

Basically, I reworked the two branches a bit to express the intent more strait forward.
I also enable all osx.cocoa checkers, as this file is all about objc stuff anyway - this also meant to diagnose two non-fatal leak errors, which are not tied to this patch otherwise.

I'm also testing that the "assumption" after the objc call thingie (message passing?) the pointer is assumed to be "nonnull". For this, I'm using the eagerly-assume=false to ensure we don't split null and non-null states eagerly when we encounter the p == nil inside clang_analyzer_eval() call argument.

I think all my changes are aligned with your intent, right?

One more thing I was thinking of was to make this ObjC null checking checker depend on the "nullability" checker package, but it turns out we can only depend on individual checkers as of now. I tried some ideas there, e.g. depending on the base checker of that package but it didn't work, so I dropped the idea. (clang/include/clang/StaticAnalyzer/Checkers/Checkers.td)

Do you plan to contribute more ObjC-related fixes in the future?
Please note that I have absolutely no experience with ObjC stuff.

And I also wanted to thank you for the high-quality patches you submitted so far!

[tripleCC]

In D152269#4402593, @steakhal wrote:

I had some improvement opportunities in mind scattered, so I decided to do them, and here is how the diff looks for me now:

recommendation.patch59 KBDownload

Basically, I reworked the two branches a bit to express the intent more strait forward.
I also enable all osx.cocoa checkers, as this file is all about objc stuff anyway - this also meant to diagnose two non-fatal leak errors, which are not tied to this patch otherwise.

I'm also testing that the "assumption" after the objc call thingie (message passing?) the pointer is assumed to be "nonnull". For this, I'm using the eagerly-assume=false to ensure we don't split null and non-null states eagerly when we encounter the p == nil inside clang_analyzer_eval() call argument.

I think all my changes are aligned with your intent, right?

One more thing I was thinking of was to make this ObjC null checking checker depend on the "nullability" checker package, but it turns out we can only depend on individual checkers as of now. I tried some ideas there, e.g. depending on the base checker of that package but it didn't work, so I dropped the idea. (clang/include/clang/StaticAnalyzer/Checkers/Checkers.td)

Do you plan to contribute more ObjC-related fixes in the future?
Please note that I have absolutely no experience with ObjC stuff.

And I also wanted to thank you for the high-quality patches you submitted so far!

Yes, your changes are aligned with my intent. It seems like you have made excellent optimizations to this patch.
To eliminate the following two warnings, I add the -fobjc-arc(enable Automatic Reference Counting) compilation option for NSContainers test .

+  MyView *view = b ? [[MyView alloc] init] : 0; // expected-warning {{Potential leak of an object stored into 'view'}}
+  NSMutableArray *subviews = [[view subviews] mutableCopy]; // expected-warning {{Potential leak of an object stored into 'subviews'}}

I will continue to contribute more ObjC-related fixes in the future, and currently, my work is also related to this.
Thank you very much for your review. Would you mind if I merge your recommendations?

[steakhal]

In D152269#4403282, @tripleCC wrote:

Yes, your changes are aligned with my intent. It seems like you have made excellent optimizations to this patch.
To eliminate the following two warnings, I add the -fobjc-arc(enable Automatic Reference Counting) compilation option for NSContainers test .

+  MyView *view = b ? [[MyView alloc] init] : 0; // expected-warning {{Potential leak of an object stored into 'view'}}
+  NSMutableArray *subviews = [[view subviews] mutableCopy]; // expected-warning {{Potential leak of an object stored into 'subviews'}}

I think we shouldn't add the -fobj-arc as these two new issues are considered TPs, and meaningful for the test file they are part of. It's just that they are not that meaningful in the scope of this patch, but that shouldn't hold us back from improving what the test covers and demonstrates, so I'm fine if those two appear as part of this patch.

I will continue to contribute more ObjC-related fixes in the future, and currently, my work is also related to this.

Sounds good. Thanks for clarifying.
If that's the case, after a few more quality patches I think it would make sense to request you commit access. Let's keep this in mind now.

Thank you very much for your review. Would you mind if I merge your recommendations?

I don't mind. You don't need to give attribution. Keep parts of the whole as you wish.

[tripleCC]

In D152269#4403404, @steakhal wrote:

In D152269#4403282, @tripleCC wrote:

Yes, your changes are aligned with my intent. It seems like you have made excellent optimizations to this patch.
To eliminate the following two warnings, I add the -fobjc-arc(enable Automatic Reference Counting) compilation option for NSContainers test .

+  MyView *view = b ? [[MyView alloc] init] : 0; // expected-warning {{Potential leak of an object stored into 'view'}}
+  NSMutableArray *subviews = [[view subviews] mutableCopy]; // expected-warning {{Potential leak of an object stored into 'subviews'}}

I think we shouldn't add the -fobj-arc as these two new issues are considered TPs, and meaningful for the test file they are part of. It's just that they are not that meaningful in the scope of this patch, but that shouldn't hold us back from improving what the test covers and demonstrates, so I'm fine if those two appear as part of this patch.

I will continue to contribute more ObjC-related fixes in the future, and currently, my work is also related to this.

Sounds good. Thanks for clarifying.
If that's the case, after a few more quality patches I think it would make sense to request you commit access. Let's keep this in mind now.

Thank you very much for your review. Would you mind if I merge your recommendations?

I don't mind. You don't need to give attribution. Keep parts of the whole as you wish.

Thanks.

By the way , there are already other unit tests focus on leak scenarios, such as the retain-release*.m test cases. Should we focus on testing container issues here? Currently, Objective-C code almost always uses ARC (Automatic Reference Counting).

[steakhal]

T

In D152269#4403491, @tripleCC wrote:

In D152269#4403404, @steakhal wrote:

In D152269#4403282, @tripleCC wrote:

Yes, your changes are aligned with my intent. It seems like you have made excellent optimizations to this patch.
To eliminate the following two warnings, I add the -fobjc-arc(enable Automatic Reference Counting) compilation option for NSContainers test .

+  MyView *view = b ? [[MyView alloc] init] : 0; // expected-warning {{Potential leak of an object stored into 'view'}}
+  NSMutableArray *subviews = [[view subviews] mutableCopy]; // expected-warning {{Potential leak of an object stored into 'subviews'}}

I think we shouldn't add the -fobj-arc as these two new issues are considered TPs, and meaningful for the test file they are part of. It's just that they are not that meaningful in the scope of this patch, but that shouldn't hold us back from improving what the test covers and demonstrates, so I'm fine if those two appear as part of this patch.

I will continue to contribute more ObjC-related fixes in the future, and currently, my work is also related to this.

Sounds good. Thanks for clarifying.
If that's the case, after a few more quality patches I think it would make sense to request you commit access. Let's keep this in mind now.

Thank you very much for your review. Would you mind if I merge your recommendations?

I don't mind. You don't need to give attribution. Keep parts of the whole as you wish.

Thanks.

By the way , there are already other unit tests focus on leak scenarios, such as the retain-release*.m test cases. Should we focus on testing container issues here? Currently, Objective-C code almost always uses ARC (Automatic Reference Counting).

Both makes sense. You can probably better judge this. I'm totally fine with any of the two options.

One note, clang-format the affected lines.

[tripleCC]

taking steakhal's advice

[tripleCC]

clang-format BasicObjCFoundationChecks

clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

 
 //===----------------------------------------------------------------------===//
 // NilArgChecker - Check for prohibited nil arguments to ObjC method calls.
 //===----------------------------------------------------------------------===//
 
 namespace {
   class NilArgChecker : public Checker<check::PreObjCMessage,
                                        check::PostStmt<ObjCDictionaryLiteral>,
-                                       check::PostStmt<ObjCArrayLiteral> > {
+                                       check::PostStmt<ObjCArrayLiteral>,
+                                       EventDispatcher<ImplicitNullDerefEvent>> {
     mutable std::unique_ptr<APIMisuse> BT;
 
     mutable llvm::SmallDenseMap<Selector, unsigned, 16> StringSelectors;
     mutable Selector ArrayWithObjectSel;
     mutable Selector AddObjectSel;
     mutable Selector InsertObjectAtIndexSel;
     mutable Selector ReplaceObjectAtIndexWithObjectSel;
     mutable Selector SetObjectAtIndexedSubscriptSel;
namespace {
   };
 } // end anonymous namespace
 
 void NilArgChecker::warnIfNilExpr(const Expr *E,
                                   const char *Msg,
                                   CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   if (State->isNull(C.getSVal(E)).isConstrainedTrue()) {
-
     if (ExplodedNode *N = C.generateErrorNode()) {
       generateBugReport(N, Msg, E->getSourceRange(), E, C);
+      return;
+    }
+  }
+
+  auto ArgSVal = C.getSVal(E).getAs<DefinedOrUnknownSVal>();
+  if (!ArgSVal)
+    return;
+
+  ProgramStateRef StNonNull, StNull;
+  std::tie(StNonNull, StNull) = State->assume(*ArgSVal);
+
+  if (StNull) {
+    if (ExplodedNode *N = C.generateSink(StNull, C.getPredecessor())) {
+      ImplicitNullDerefEvent event = {*ArgSVal, false, N, &C.getBugReporter(),
+                                      /*IsDirectDereference=*/false};
+      dispatchEvent(event);
     }
   }
+
+  if (StNonNull)
+    State = StNonNull;
+
+  C.addTransition(State);
 }
 
 void NilArgChecker::warnIfNilArg(CheckerContext &C,
                                  const ObjCMethodCall &msg,
                                  unsigned int Arg,
                                  FoundationClass Class,
                                  bool CanBeSubscript) const {
   // Check if the argument is nil.

clang/test/Analysis/NSContainers.m

-// RUN: %clang_analyze_cc1  -Wno-objc-literal-conversion -analyzer-checker=core,osx.cocoa.NonNilReturnValue,osx.cocoa.NilArg,osx.cocoa.Loops,debug.ExprInspection -verify -Wno-objc-root-class %s
+// RUN: %clang_analyze_cc1 -Wno-objc-literal-conversion -analyzer-checker=core,osx.cocoa.NonNilReturnValue,osx.cocoa.NilArg,osx.cocoa.Loops,debug.ExprInspection,nullability -verify -Wno-objc-root-class %s
 
 void clang_analyzer_eval(int);
 
 #define nil ((id)0)
 
 typedef unsigned long NSUInteger;
 typedef signed char BOOL;
 typedef struct _NSZone NSZone;
if (!string)
     return nil;
   return @"New String";
 }
 void testInlinedDefensiveCheck(NSMutableDictionary *dict, id obj) {
   // The check in getStringFromString() is not a good indication
   // that 'obj' can be nil in this context.
   dict[obj] = getStringFromString(obj); // no-warning
 }
+
+Foo * _Nonnull getNonnullFoo();
+Foo * _Nullable getNullableFoo();
+Foo * getMightBeNullFoo();
+
+void testCreateDictionaryLiteralWithNullableArg() {
+  (void)@{@"abc" : getMightBeNullFoo()}; // no warning
+  (void)@{@"abc" : getNonnullFoo()}; // no warning
+  (void)@{@"abc" : getNullableFoo()}; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null}}
+}
+
+void testCreateArrayLiteralWithNullableArg() {
+  (void)@[getMightBeNullFoo()]; // no warning
+  (void)@[getNonnullFoo()]; // no warning
+  (void)@[getNullableFoo()]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null}}
+}