[NFC] Fix potential nullptr dereference.

[zahiraam]

A nullptr dereference is detected by the code sanitizer. Adding an assert to make sure it's caught.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Zahira Ammarguellat (zahiraam)

Changes

A nullptr dereference is detected by the code sanitizer. Adding an assert to make sure it's caught.

---

Full diff: https://github.com/llvm/llvm-project/pull/138283.diff

1 Files Affected:

• (modified) clang-tools-extra/clang-tidy/readability/RedundantFunctionPtrDereferenceCheck.cpp (+1)

diff --git a/clang-tools-extra/clang-tidy/readability/RedundantFunctionPtrDereferenceCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantFunctionPtrDereferenceCheck.cpp
index 247d287be2b36..e90170a6591da 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantFunctionPtrDereferenceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantFunctionPtrDereferenceCheck.cpp
@@ -25,6 +25,7 @@ void RedundantFunctionPtrDereferenceCheck::registerMatchers(MatchFinder *Finder)
 
 void RedundantFunctionPtrDereferenceCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Operator = Result.Nodes.getNodeAs<UnaryOperator>("op");
+  assert(Operator && "Operator is null");
   diag(Operator->getOperatorLoc(),
        "redundant repeated dereference of function pointer")
       << FixItHint::CreateRemoval(Operator->getOperatorLoc());

[vbvictor]

Did you run code sanitizer over all clang-tidy code or you only interested in this particular file?
I think there are many more places where such nullptr dereference can happen, e.g.

llvm-project/clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.cpp

Lines 39 to 43 in 52d2b58

	const auto *MemberCall =
	Result.Nodes.getNodeAs<CXXMemberCallExpr>("erase");
	const auto *EndExpr =
	Result.Nodes.getNodeAs<CXXMemberCallExpr>("end");
	const SourceLocation Loc = MemberCall->getBeginLoc();

llvm-project/clang-tools-extra/clang-tidy/bugprone/IncorrectRoundingsCheck.cpp

Lines 51 to 52 in 52d2b58

	const auto *CastExpr = Result.Nodes.getNodeAs<ImplicitCastExpr>("CastExpr");
	diag(CastExpr->getBeginLoc(),

[zahiraam]

Did you run code sanitizer over all clang-tidy code or you only interested in this particular file? I think there are many more places where such nullptr dereference can happen, e.g.

llvm-project/clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.cpp

Lines 39 to 43 in 52d2b58

	const auto *MemberCall =
	Result.Nodes.getNodeAs<CXXMemberCallExpr>("erase");
	const auto *EndExpr =
	Result.Nodes.getNodeAs<CXXMemberCallExpr>("end");
	const SourceLocation Loc = MemberCall->getBeginLoc();

llvm-project/clang-tools-extra/clang-tidy/bugprone/IncorrectRoundingsCheck.cpp

Lines 51 to 52 in 52d2b58

	const auto *CastExpr = Result.Nodes.getNodeAs<ImplicitCastExpr>("CastExpr");
	diag(CastExpr->getBeginLoc(),

Probably. But we usually stick to the potential issues suggested by the sanitizer.

[nicovank]

This is not needed IMO. This is a simple matcher, op is expected to always be bound.

One might say that it can't hurt to add extra checks, but I don't think that's good justification for adding more code / bloat, even if in this case it's a single line. Still, 5% more code for this check counting non-empty non-comment lines!

Happy to disagree if any of the maintainers think otherwise.

[zahiraam]

This is not needed IMO. This is a simple matcher, op is expected to always be bound.

One might say that it can't hurt to add extra checks, but I don't think that's good justification for adding more code / bloat, even if in this case it's a single line. Still, 5% more code for this check counting non-empty non-comment lines!

Happy to disagree if any of the maintainers think otherwise.

The sanitizer is complaining about the possibility of having Operator be null. op might always be bound. But it might not be a UnaryOperator, no?

[vbvictor]

I don't have a strong opinion on this, but this assert may be a sign of "false safety".
In release builds, it's a no-op, so end users will experience a regular Segfault, e.g. like this issue #135665.
Debug builds with asserts are usually used in development process where tests run regularly, so all nullptr dereferences must be caught there.
TLDR, adding this assert "retrospectively" probably have no impact.

[nicovank]

But it might not be a UnaryOperator, no?

unaryOperator(...).bind("op") implies op will always be a UnaryOperator.

[zahiraam]

Thanks for your input. I will close this PR.