This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by tripleCC on Jun 6 2023, 7:12 AM.

Details

Summary

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

I don't have commit access.

Diff Detail

Event Timeline

tripleCC created this revision.Jun 6 2023, 7:12 AM
Herald added a project: Restricted Project. · View Herald Transcript
tripleCC requested review of this revision.Jun 6 2023, 7:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2023, 7:12 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tripleCC retitled this revision from [StaticAnalyzer] Fix false negtive on NilArgChecker when creating literal object to [StaticAnalyzer] Fix false negative on NilArgChecker when creating literal object.Jun 6 2023, 7:16 AM
tripleCC edited the summary of this revision. (Show Details)
tripleCC edited the summary of this revision. (Show Details)
steakhal added inline comments.Jun 6 2023, 7:35 AM
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 added inline comments.Jun 6 2023, 8:28 AM
clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
151–164

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

tripleCC updated this revision to Diff 529116.Jun 6 2023, 6:21 PM

[StaticAnalyzer] add might be null test case for NilArgChecker

tripleCC added inline comments.Jun 6 2023, 7:08 PM
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 added inline comments.Jun 6 2023, 7:51 PM
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);
    }
  }
}

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

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 added a comment.EditedJun 7 2023, 7:47 AM

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

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?

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 added a comment.EditedJun 7 2023, 9:00 AM

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).

T

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 updated this revision to Diff 529583.Jun 8 2023, 7:19 AM

taking steakhal's advice

tripleCC updated this revision to Diff 529584.Jun 8 2023, 7:23 AM

clang-format BasicObjCFoundationChecks

This revision was not accepted when it landed; it landed in state Needs Review.Jun 8 2023, 7:49 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.