[clang][dataflow]Use cast_or_null instead of cast to prevent crash

[jcsxky]

getStorageLocation may return nullptr and this will produce crash when use cast, use dyn_cast_or_null instead. I test it locally using FTXUI and it may be the cause of issue issue, but I am not sure.

[llvmbot]

@llvm/pr-subscribers-clang-tidy
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Changes

getStorageLocation may return nullptr and this will produce crash when use cast, use dyn_cast_or_null instead. I test it locally using FTXUI and it may be the cause of issue issue, but I am not sure.

---

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

1 Files Affected:

• (modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+2-1)

diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index f61f26ff27804ec..dda1c6cfca017d0 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -26,6 +26,7 @@
 #include "clang/Analysis/FlowSensitive/NoopLattice.h"
 #include "clang/Analysis/FlowSensitive/StorageLocation.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
+#include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
@@ -599,7 +600,7 @@ void transferAssignment(const CXXOperatorCallExpr *E, BoolValue &HasValueVal,
                         LatticeTransferState &State) {
   assert(E->getNumArgs() > 0);
 
-  if (auto *Loc = cast<RecordStorageLocation>(
+  if (auto *Loc = dyn_cast_or_null<RecordStorageLocation>(
           State.Env.getStorageLocation(*E->getArg(0)))) {
     createOptionalValue(*Loc, HasValueVal, State.Env);
 

[Xazax-hun]

The fix looks great to me! On the other hand, we usually try to add a regression test for each of the fixes. Any chance you could get a minimal reproducer from the codebase you are looking at?

[jcsxky]

The fix looks great to me! On the other hand, we usually try to add a regression test for each of the fixes. Any chance you could get a minimal reproducer from the codebase you are looking at?

Thanks for your suggestion! Test case has been added to reproduce this issue.

[HerrCai0907]

please update release note also

[jcsxky]

please update release note also

Although bugprone-unchecked-optional-access is used to test this issue, root cause is in FlowSensitiveAnalysis. So, I am confused about which file should be updated about release not. Look forward to your suggestion! Thank you.

[PiotrZSL]

LGTM,
When release notes could be nice (clang-tools-extra/doc/ReleaseNotest.rst) with something like: Improved bugprone-unchecked-optional-acces check to not crash during handling of optional values or to not crash i certain situations it may also not be so necessary, as this is just very quick fix.

[jcsxky]

LGTM, When release notes could be nice (clang-tools-extra/doc/ReleaseNotest.rst) with something like: Improved bugprone-unchecked-optional-acces check to not crash during handling of optional values or to not crash i certain situations it may also not be so necessary, as this is just very quick fix.

Release notes have been updated according to your suggestion. Thank you for detail guidance!

[ymand]

Thanks for this fix! Unfortunately, I wasn't able to repro the crash in godbolt: https://godbolt.org/z/s741z5djY. Can you double check that the check crashes on that example without your fix?

[jcsxky]

Thanks for this fix! Unfortunately, I wasn't able to repro the crash in godbolt: https://godbolt.org/z/s741z5djY. Can you double check that the check crashes on that example without your fix?

The test case is different from that in this patch. Please Use float instead of int in optional as template type. But in godbolt it doesn't crash, probably doesn't show the crash stack or not latest version? I don't know. You could test it locally after changing template argument of optional from int to float and it will crash.
The reason why int type doesn't trigger crash is that

unless(hasDeclaration(cxxMethodDecl(
          anyOf(isCopyAssignmentOperator(), isMoveAssignmentOperator()))))

in isOptionalValueOrConversionAssignment() can't match

vec[0].x = 0;

so, flow can't enter into transferValueOrConversionAssignment() and produce crash.

[jcsxky]

@ymand Could you take a look at this pr?

[ymand]

Thanks, looks good!

You can submit as is, but if you're up for it, it would actually be better to add the new test case directly to the model's unittests. Something like this test (though just one case is enough -- please put it in a separate TEST_P):

llvm-project/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Line 1966 in a3a0f59

TEST_P(UncheckedOptionalAccessTest, ValueAssignment) {

In general, the lit tests for the clang tidy check only do minimal testing to ensure the check is properly calling the model.

[jcsxky]

Thanks, looks good!

You can submit as is, but if you're up for it, it would actually be better to add the new test case directly to the model's unittests. Something like this test (though just one case is enough -- please put it in a separate TEST_P):

llvm-project/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Line 1966 in a3a0f59

TEST_P(UncheckedOptionalAccessTest, ValueAssignment) {

In general, the lit tests for the clang tidy check only do minimal testing to ensure the check is properly calling the model.

Thanks for your suggestion! New test case has been added to unittests.