[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, george.karpenkov.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware.

This false negative bug was exposed by https://reviews.llvm.org/D18860. The checker tries to detect the situation when a symbol previously passed through a `_Nonnull`-annotated function parameter is constrained to null later on the path, and suppress its warnings on such paths by setting a state-wide flag `<InvariantViolated>`. The detection for symbols being constrained to null is done, in particular, in `checkDeadSymbols()`, because that's the last moment when we see the symbol and therefore we have perfect knowledge of its constraints at this moment. The detection works by ascending the stack frame chain and seeing if any of the `_Nonnull` parameters has a null value in the current program state.

It turned out that such invariant violation detection in checkDeadSymbols() races with checkPreCall(). Suppose that we're stuffing a concrete null (rather than a symbol constrained to null) into the function. Depending on the result of the race, there would be two possible scenarios:

1. `checkPreCall()` fires first. In this case a valid warning will be issued that null is being passed through a parameter that is annotated as `_Nonnull`.
2. `checkDeadSymbols()` fires first. In this case it'll notice that a `_Nonnull` parameter of the call that we almost entered is equal to null and raise the `<InvariantViolated>` flag, suppressing all nullability warnings.

Which means that depending on "something" we either see or don't see a warning.

What is this "something"? Why are callbacks were called in different order? Easy: it's because `checkDeadSymbols()` weren't firing at all when there were no dead symbols. Now that zombie symbol bugs are fixed, we realize that it's always important to call `checkDeadSymbols()`, because we've no idea what symbols the checker may track. So after fixing zombie symbols in https://reviews.llvm.org/D18860, the race started resolving to scenario 2, resulting in more false negatives.

The bug is fixed by restricting the check to work with symbols only, not with concrete values. This works because by the time we reach the call, symbolic values should be either not constrained to null, or already replaced with concrete `0 (Loc)` values. Though generally the code is a bit weird and might require more thought.


Repository:
  rC Clang

https://reviews.llvm.org/D54017

Files:
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  test/Analysis/nullability-arc.mm
  test/Analysis/nullability.mm


Index: test/Analysis/nullability.mm
===================================================================
--- test/Analysis/nullability.mm
+++ test/Analysis/nullability.mm
@@ -1,5 +1,7 @@
 // RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced -DNOSYSTEMHEADERS=0 -verify %s
 // RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced -analyzer-config nullability:NoDiagnoseCallsToSystemHeaders=true -DNOSYSTEMHEADERS=1 -verify %s
+// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced -DNOSYSTEMHEADERS=0 -verify %s -fobjc-arc
+// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced -analyzer-config nullability:NoDiagnoseCallsToSystemHeaders=true -DNOSYSTEMHEADERS=1 -verify %s -fobjc-arc
 
 #include "Inputs/system-header-simulator-for-nullability.h"
 
Index: test/Analysis/nullability-arc.mm
===================================================================
--- /dev/null
+++ test/Analysis/nullability-arc.mm
@@ -0,0 +1,37 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,nullability -analyzer-output=text -verify %s
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,nullability -analyzer-output=text -verify %s -fobjc-arc
+
+#if !__has_feature(objc_arc)
+// expected-no-diagnostics
+#endif
+
+
+#define nil ((id)0)
+
+ at interface Param
+ at end
+
+ at interface Base
+- (void)foo:(Param *_Nonnull)param;
+ at end
+
+ at interface Derived : Base
+ at end
+
+ at implementation Derived
+- (void)foo:(Param *)param {
+  // FIXME: Why do we not emit the warning under ARC?
+  [super foo:param];
+#if __has_feature(objc_arc)
+  // expected-warning at -2{{nil passed to a callee that requires a non-null 1st parameter}}
+  // expected-note at -3   {{nil passed to a callee that requires a non-null 1st parameter}}
+#endif
+
+  [self foo:nil];
+#if __has_feature(objc_arc)
+  // expected-note at -2{{Calling 'foo:'}}
+  // expected-note at -3{{Passing nil object reference via 1st parameter 'param'}}
+#endif
+}
+ at end
+
Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -340,8 +340,11 @@
   if (!RegionVal)
     return false;
 
-  auto StoredVal =
-  State->getSVal(RegionVal->getRegion()).getAs<DefinedOrUnknownSVal>();
+  // There should have originally been a symbol. If it was a concrete value
+  // to begin with, the violation should have been handled immediately
+  // rather than delayed until constraints are updated. Why "symbol" rather than
+  // "region"? Because only symbolic regions can be null.
+  auto StoredVal = State->getSVal(*RegionVal).getAs<loc::MemRegionVal>();
   if (!StoredVal)
     return false;
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D54017.172274.patch
Type: text/x-patch
Size: 3415 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181102/7a577c77/attachment.bin>