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)
+
+@interface Param
+@end
+
+@interface Base
+- (void)foo:(Param *_Nonnull)param;
+@end
+
+@interface Derived : Base
+@end
+
+@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@-2{{nil passed to a callee that requires a non-null 1st
parameter}}
+ // expected-note@-3 {{nil passed to a callee that requires a non-null 1st
parameter}}
+#endif
+
+ [self foo:nil];
+#if __has_feature(objc_arc)
+ // expected-note@-2{{Calling 'foo:'}}
+ // expected-note@-3{{Passing nil object reference via 1st parameter 'param'}}
+#endif
+}
+@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;
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)
+
+@interface Param
+@end
+
+@interface Base
+- (void)foo:(Param *_Nonnull)param;
+@end
+
+@interface Derived : Base
+@end
+
+@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@-2{{nil passed to a callee that requires a non-null 1st parameter}}
+ // expected-note@-3 {{nil passed to a callee that requires a non-null 1st parameter}}
+#endif
+
+ [self foo:nil];
+#if __has_feature(objc_arc)
+ // expected-note@-2{{Calling 'foo:'}}
+ // expected-note@-3{{Passing nil object reference via 1st parameter 'param'}}
+#endif
+}
+@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;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
