[clang][dataflow] Fix crash when operator= result type is not destination type.
#90898
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nation type. The existing code was full of comments about how we assume this is always the case, but it's not mandated by the standard, and there is code out there that returns a different type. So check that the result type is in fact the same as the destination type before attempting to copy to the result. To make sure that we don't bail out in more cases than intended, I've extended existing tests to verify that in the common case, we do return the destination object (by reference or value, as the case may be).
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (martinboehme) ChangesThe existing code was full of comments about how we assume this is always the To make sure that we don't bail out in more cases than intended, I've extended Full diff: https://github.com/llvm/llvm-project/pull/90898.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index fd224aeb79b151..5a57f11b000d8a 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -556,14 +556,23 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
copyRecord(*LocSrc, *LocDst, Env);
- // If the expr is a glvalue, we can reasonably assume the operator is
- // returning T& and thus we can assign it `LocDst`.
- if (S->isGLValue()) {
+ // The assignment operator can have an arbitrary return type. We model the
+ // return value only if the return type is the same as or a base class of
+ // the destination type.
+ if (S->getType().getCanonicalType().getUnqualifiedType() !=
+ LocDst->getType().getCanonicalType().getUnqualifiedType()) {
+ auto ReturnDecl = S->getType()->getAsCXXRecordDecl();
+ auto DstDecl = LocDst->getType()->getAsCXXRecordDecl();
+ if (ReturnDecl == nullptr || DstDecl == nullptr)
+ return;
+ if (!DstDecl->isDerivedFrom(ReturnDecl))
+ return;
+ }
+
+ if (S->isGLValue())
Env.setStorageLocation(*S, *LocDst);
- } else if (S->getType()->isRecordType()) {
- // Assume that the assignment returns the assigned value.
+ else
copyRecord(*LocDst, Env.getResultObjectLocation(*S), Env);
- }
return;
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 301bec32c0cf1d..0b441faa6c76da 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -2228,7 +2228,7 @@ TEST(TransferTest, AssignmentOperator) {
A Foo = { 1 };
A Bar = { 2 };
// [[p1]]
- Foo = Bar;
+ A &Rval = (Foo = Bar);
// [[p2]]
Foo.Baz = 3;
// [[p3]]
@@ -2274,6 +2274,7 @@ TEST(TransferTest, AssignmentOperator) {
cast<RecordStorageLocation>(Env2.getStorageLocation(*BarDecl));
EXPECT_TRUE(recordsEqual(*FooLoc2, *BarLoc2, Env2));
+ EXPECT_EQ(&getLocForDecl(ASTCtx, Env2, "Rval"), FooLoc2);
const auto *FooBazVal2 =
cast<IntegerValue>(getFieldValue(FooLoc2, *BazDecl, Env2));
@@ -2441,7 +2442,75 @@ TEST(TransferTest, AssignmentOperatorReturnsByValue) {
// This is a crash repro.
std::string Code = R"(
struct S {
- S operator=(S&& other);
+ S operator=(const S&);
+ int i;
+ };
+ void target() {
+ S S1 = { 1 };
+ S S2 = { 2 };
+ S S3 = { 3 };
+ // [[before]]
+ // Test that the returned value is modeled by assigning to another value.
+ S1 = (S2 = S3);
+ (void)0;
+ // [[after]]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ const ValueDecl *S1Decl = findValueDecl(ASTCtx, "S1");
+ const ValueDecl *S2Decl = findValueDecl(ASTCtx, "S2");
+ const ValueDecl *S3Decl = findValueDecl(ASTCtx, "S3");
+
+ const Environment &EnvBefore =
+ getEnvironmentAtAnnotation(Results, "before");
+
+ EXPECT_FALSE(recordsEqual(
+ *EnvBefore.get<RecordStorageLocation>(*S1Decl),
+ *EnvBefore.get<RecordStorageLocation>(*S2Decl), EnvBefore));
+ EXPECT_FALSE(recordsEqual(
+ *EnvBefore.get<RecordStorageLocation>(*S2Decl),
+ *EnvBefore.get<RecordStorageLocation>(*S3Decl), EnvBefore));
+
+ const Environment &EnvAfter =
+ getEnvironmentAtAnnotation(Results, "after");
+
+ EXPECT_TRUE(recordsEqual(*EnvAfter.get<RecordStorageLocation>(*S1Decl),
+ *EnvAfter.get<RecordStorageLocation>(*S2Decl),
+ EnvAfter));
+ EXPECT_TRUE(recordsEqual(*EnvAfter.get<RecordStorageLocation>(*S2Decl),
+ *EnvAfter.get<RecordStorageLocation>(*S3Decl),
+ EnvAfter));
+ });
+}
+
+TEST(TransferTest, AssignmentOperatorReturnsDifferentTypeByRef) {
+ // This is a crash repro.
+ std::string Code = R"(
+ struct DifferentType {};
+ struct S {
+ DifferentType& operator=(const S&);
+ };
+ void target() {
+ S s;
+ s = S();
+ // [[p]]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {});
+}
+
+TEST(TransferTest, AssignmentOperatorReturnsDifferentTypeByValue) {
+ // This is a crash repro.
+ std::string Code = R"(
+ struct DifferentType {};
+ struct S {
+ DifferentType operator=(const S&);
};
void target() {
S s;
|
|
@llvm/pr-subscribers-clang-analysis Author: None (martinboehme) ChangesThe existing code was full of comments about how we assume this is always the To make sure that we don't bail out in more cases than intended, I've extended Full diff: https://github.com/llvm/llvm-project/pull/90898.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index fd224aeb79b151..5a57f11b000d8a 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -556,14 +556,23 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
copyRecord(*LocSrc, *LocDst, Env);
- // If the expr is a glvalue, we can reasonably assume the operator is
- // returning T& and thus we can assign it `LocDst`.
- if (S->isGLValue()) {
+ // The assignment operator can have an arbitrary return type. We model the
+ // return value only if the return type is the same as or a base class of
+ // the destination type.
+ if (S->getType().getCanonicalType().getUnqualifiedType() !=
+ LocDst->getType().getCanonicalType().getUnqualifiedType()) {
+ auto ReturnDecl = S->getType()->getAsCXXRecordDecl();
+ auto DstDecl = LocDst->getType()->getAsCXXRecordDecl();
+ if (ReturnDecl == nullptr || DstDecl == nullptr)
+ return;
+ if (!DstDecl->isDerivedFrom(ReturnDecl))
+ return;
+ }
+
+ if (S->isGLValue())
Env.setStorageLocation(*S, *LocDst);
- } else if (S->getType()->isRecordType()) {
- // Assume that the assignment returns the assigned value.
+ else
copyRecord(*LocDst, Env.getResultObjectLocation(*S), Env);
- }
return;
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 301bec32c0cf1d..0b441faa6c76da 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -2228,7 +2228,7 @@ TEST(TransferTest, AssignmentOperator) {
A Foo = { 1 };
A Bar = { 2 };
// [[p1]]
- Foo = Bar;
+ A &Rval = (Foo = Bar);
// [[p2]]
Foo.Baz = 3;
// [[p3]]
@@ -2274,6 +2274,7 @@ TEST(TransferTest, AssignmentOperator) {
cast<RecordStorageLocation>(Env2.getStorageLocation(*BarDecl));
EXPECT_TRUE(recordsEqual(*FooLoc2, *BarLoc2, Env2));
+ EXPECT_EQ(&getLocForDecl(ASTCtx, Env2, "Rval"), FooLoc2);
const auto *FooBazVal2 =
cast<IntegerValue>(getFieldValue(FooLoc2, *BazDecl, Env2));
@@ -2441,7 +2442,75 @@ TEST(TransferTest, AssignmentOperatorReturnsByValue) {
// This is a crash repro.
std::string Code = R"(
struct S {
- S operator=(S&& other);
+ S operator=(const S&);
+ int i;
+ };
+ void target() {
+ S S1 = { 1 };
+ S S2 = { 2 };
+ S S3 = { 3 };
+ // [[before]]
+ // Test that the returned value is modeled by assigning to another value.
+ S1 = (S2 = S3);
+ (void)0;
+ // [[after]]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ const ValueDecl *S1Decl = findValueDecl(ASTCtx, "S1");
+ const ValueDecl *S2Decl = findValueDecl(ASTCtx, "S2");
+ const ValueDecl *S3Decl = findValueDecl(ASTCtx, "S3");
+
+ const Environment &EnvBefore =
+ getEnvironmentAtAnnotation(Results, "before");
+
+ EXPECT_FALSE(recordsEqual(
+ *EnvBefore.get<RecordStorageLocation>(*S1Decl),
+ *EnvBefore.get<RecordStorageLocation>(*S2Decl), EnvBefore));
+ EXPECT_FALSE(recordsEqual(
+ *EnvBefore.get<RecordStorageLocation>(*S2Decl),
+ *EnvBefore.get<RecordStorageLocation>(*S3Decl), EnvBefore));
+
+ const Environment &EnvAfter =
+ getEnvironmentAtAnnotation(Results, "after");
+
+ EXPECT_TRUE(recordsEqual(*EnvAfter.get<RecordStorageLocation>(*S1Decl),
+ *EnvAfter.get<RecordStorageLocation>(*S2Decl),
+ EnvAfter));
+ EXPECT_TRUE(recordsEqual(*EnvAfter.get<RecordStorageLocation>(*S2Decl),
+ *EnvAfter.get<RecordStorageLocation>(*S3Decl),
+ EnvAfter));
+ });
+}
+
+TEST(TransferTest, AssignmentOperatorReturnsDifferentTypeByRef) {
+ // This is a crash repro.
+ std::string Code = R"(
+ struct DifferentType {};
+ struct S {
+ DifferentType& operator=(const S&);
+ };
+ void target() {
+ S s;
+ s = S();
+ // [[p]]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {});
+}
+
+TEST(TransferTest, AssignmentOperatorReturnsDifferentTypeByValue) {
+ // This is a crash repro.
+ std::string Code = R"(
+ struct DifferentType {};
+ struct S {
+ DifferentType operator=(const S&);
};
void target() {
S s;
|
ymand
approved these changes
May 3, 2024
Xazax-hun
reviewed
May 5, 2024
| auto DstDecl = LocDst->getType()->getAsCXXRecordDecl(); | ||
| if (ReturnDecl == nullptr || DstDecl == nullptr) | ||
| return; | ||
| if (!DstDecl->isDerivedFrom(ReturnDecl)) |
There was a problem hiding this comment.
Would we want to create a fresh storage location here?
There was a problem hiding this comment.
I think it's consistent with our more general approach -- we treat "storage location not set" as meaning "we have no information" 1. For example, if above we discover that this isn't a copy or move assignment operator, we bail out without doing anything.
Granted, this case is a bit different in that we have already modeled the assignment itself (by doing the copyRecord()) above, but I think it's still consistent to do this but decide we don't know how to model the return value / location.
Footnotes
-
Of course, a fresh storage location that isn't otherwise constrained expresses essentially the same thing, but it's simpler to instead not assign a storage location at all. ↩
Xazax-hun
approved these changes
May 5, 2024
|
CI failures appear to be unrelated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The existing code was full of comments about how we assume this is always the
case, but it's not mandated by the standard, and there is code out there that
returns a different type. So check that the result type is in fact the same as
the destination type before attempting to copy to the result.
To make sure that we don't bail out in more cases than intended, I've extended
existing tests to verify that in the common case, we do return the destination
object (by reference or value, as the case may be).