Skip to content

Commit

Permalink
[nullability] Fix a crash when calling builtin functions.
Browse files Browse the repository at this point in the history
Despite its name, the result type of `BuiltinFnToFnPtr` cast is a function, not
a function pointer. This means that the callee of a `CallExpr` to a builtin
function is not a pointer and so does not have nullability information
associated with it. We therefore crashed when trying to access this nullability
information.

We fix this by only checking for nullability if we have an indirect callee.
Direct callees are always non-null, and it isn't possible to take the address of
a builtin function, so calls to builtin functions are always direct.

We also fix the implementation of the `BuiltinFnToFnPtr` cast so that it doesn't
prepend an additional `NullabilityKind::NonNull`.

The implementation of the `BuiltinFnToFnPtr` cast in the dataflow framework will
also need to be changed, as it currently creates a pointer. However, the current
fix will prevent the crash even before the dataflow framework is changed because
the nullability checker will now never attempt to query the value associated
with the callee to a builtin function.

PiperOrigin-RevId: 543360562
  • Loading branch information
martinboehme authored and Copybara-Service committed Jun 26, 2023
1 parent 000d1c6 commit 381e2b9
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 8 deletions.
6 changes: 5 additions & 1 deletion nullability/pointer_nullability_analysis.cc
Original file line number Diff line number Diff line change
Expand Up @@ -526,10 +526,14 @@ void transferNonFlowSensitiveCastExpr(
// Decayed objects are never null.
case CK_ArrayToPointerDecay:
case CK_FunctionToPointerDecay:
case CK_BuiltinFnToFnPtr:
return prepend(NullabilityKind::NonNull,
getNullabilityForChild(CE->getSubExpr(), State));

// Despite its name, the result type of `BuiltinFnToFnPtr` is a function,
// not a function pointer, so nullability doesn't change.
case CK_BuiltinFnToFnPtr:
return getNullabilityForChild(CE->getSubExpr(), State);

// TODO: what is our model of member pointers?
case CK_BaseToDerivedMemberPointer:
case CK_DerivedToBaseMemberPointer:
Expand Down
14 changes: 7 additions & 7 deletions nullability/pointer_nullability_diagnosis.cc
Original file line number Diff line number Diff line change
Expand Up @@ -172,13 +172,13 @@ bool diagnoseAssertNullabilityCall(
std::optional<CFGElement> diagnoseCallExpr(
const CallExpr* CE, const MatchFinder::MatchResult& Result,
const TransferStateForDiagnostics<PointerNullabilityLattice>& State) {
// Emit a warning for a nullable callee. We don't do this for member functions
// because in this case the callee can't be null. If we're calling a
// pointer-to-member-function, the callee is a `.*` or `->*` `BinaryOperator`,
// which itself can never be null. A nullable pointer-to-member-function will
// manifest as a nullable RHS of this `BinaryOperator` and should be diagnosed
// there.
if (!isa<CXXMemberCallExpr>(CE) &&
// Check whether the callee is null.
// - Skip direct callees to avoid handling builtin functions, which don't
// decay to pointer.
// - Skip member callees, as they are not pointers at all (rather "bound
// member function type").
// Note that in `(obj.*nullable_pmf)()` the deref is *before* the call.
if (!CE->getDirectCallee() && !isa<CXXMemberCallExpr>(CE) &&
isNullableOrUntracked(CE->getCallee(), State.Env)) {
return std::optional<CFGElement>(CFGStmt(CE->getCallee()));
}
Expand Down
7 changes: 7 additions & 0 deletions nullability/test/function_calls.cc
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,13 @@ TEST(PointerNullabilityTest, CallFunctionTemplate_PartiallyDeduced) {
)cc"));
}

TEST(PointerNullabilityTest, CallBuiltinFunction) {
// Crash repro.
EXPECT_TRUE(checkDiagnostics(R"cc(
void target() { __builtin_operator_new(0); }
)cc"));
}

} // namespace
} // namespace nullability
} // namespace tidy
Expand Down

0 comments on commit 381e2b9

Please sign in to comment.