[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

167–180

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

339–340

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

167–180

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

339–340

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

167–180

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;
public:
     void checkPostStmt(const ObjCArrayLiteral *AL,
                        CheckerContext &C) const;
   };
 } // 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()) {
+  auto Location = C.getSVal(E).getAs<Loc>();
+  if (!Location)
+    return;
+  
+  auto [NonNull, Null] = C.getState()->assume(*Location);
   
+  // If it's known to be null.
+  if (!NonNull && Null) {
     if (ExplodedNode *N = C.generateErrorNode()) {
       generateBugReport(N, Msg, E->getSourceRange(), E, C);
+      return;
+    }
+  }
+  
+  // If it might be null, assume that it cannot after this operation.
+  if (Null) {
+    // One needs to make sure the pointer is non-null to be used here.
+    if (ExplodedNode *N = C.generateSink(Null, C.getPredecessor())) {
+      dispatchEvent({*Location, /*IsLoad=*/false, N, &C.getBugReporter(),
+                    /*IsDirectDereference=*/false});
     }
+    C.addTransition(NonNull);
   }
 }
 
 void NilArgChecker::warnIfNilArg(CheckerContext &C,
                                  const ObjCMethodCall &msg,
                                  unsigned int Arg,
                                  FoundationClass Class,
                                  bool CanBeSubscript) const {
   // Check if the argument is nil.
   ProgramStateRef State = C.getState();
   if (!State->isNull(msg.getArgSVal(Arg)).isConstrainedTrue())
       return;
 
   // NOTE: We cannot throw non-fatal errors from warnIfNilExpr,
   // because it's called multiple times from some callers, so it'd cause
   // an unwanted state split if two or more non-fatal errors are thrown
   // within the same checker callback. For now we don't want to, but
   // it'll need to be fixed if we ever want to.
   if (ExplodedNode *N = C.generateErrorNode()) {
     SmallString<128> sbuf;
     llvm::raw_svector_ostream os(sbuf);
 
     if (CanBeSubscript && msg.getMessageKind() == OCM_Subscript) {
 

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 -Wno-objc-root-class -fobjc-arc \
+// RUN:   -analyzer-checker=core,osx.cocoa,nullability \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-checker=debug.ExprInspection -verify %s
 
 void clang_analyzer_eval(int);
 
 #define nil ((id)0)
 
 typedef unsigned long NSUInteger;
 typedef signed char BOOL;
 typedef struct _NSZone NSZone;

 @interface NSObject <NSObject> {}
 - (id)init;
 + (id)alloc;
 
 - (id)mutableCopy;
 @end
 
 typedef struct {
     unsigned long state;
-  id *itemsPtr;
-  unsigned long *mutationsPtr;
+    id __unsafe_unretained _Nullable * _Nullable itemsPtr;
+    unsigned long * _Nullable mutationsPtr;
     unsigned long extra[5];
 } NSFastEnumerationState;
 @protocol NSFastEnumeration
-- (NSUInteger)countByEnumeratingWithState:(NSFastEnumerationState *)state objects:(id [])buffer count:(NSUInteger)len;
+- (NSUInteger)countByEnumeratingWithState:(NSFastEnumerationState *)state objects:(id __unsafe_unretained _Nullable [_Nonnull])buffer count:(NSUInteger)len;
 @end
 
 @interface NSArray : NSObject <NSCopying, NSMutableCopying, NSSecureCoding, NSFastEnumeration>
 - (NSUInteger)count;
 - (id)objectAtIndex:(NSUInteger)index;
 @end
 
 @interface NSArray (NSExtendedArray)
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 * getMightBeNullFoo();
+Foo * _Nonnull getNonnullFoo();
+Foo * _Nullable getNullableFoo();
+
+void testCreateDictionaryLiteralWithNullableArg() {
+  Foo *p1 = getMightBeNullFoo();
+  Foo *p2 = getNonnullFoo();
+  Foo *p3 = getNullableFoo();
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{UNKNOWN}}
+
+  (void)@{@"abc" : p1}; // no-warning
+  (void)@{@"abc" : p2}; // no-warning
+  (void)@{@"abc" : p3}; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null}}
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{FALSE}}
+}
+
+void testCreateArrayLiteralWithNullableArg() {
+  Foo *p1 = getMightBeNullFoo();
+  Foo *p2 = getNonnullFoo();
+  Foo *p3 = getNullableFoo();
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{UNKNOWN}}
+
+  (void)@[p1]; // no-warning
+  (void)@[p2]; // no-warning
+  (void)@[p3]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null}}
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{FALSE}}
+}
 No newline at end of file