[ConstEval] Fix crash when comparing strings past the end #137078
Merged
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
When `ArePotentiallyOverlappingStringLiterals`, added in llvm#109208, compares string literals it drops the front of the string with the greatest offset from its base pointer. The number of characters dropped is equal to the difference between the two strings' offsets from their base pointers. This would trigger an assert when the resulting offset is past the end of the object. Not only are one-past-the-end pointers legal constructs, the compiler should not crash even when faced with illegal constructs. rdar://149865910
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Henrik G. Olsson (hnrklssn) ChangesWhen rdar://149865910 Full diff: https://github.com/llvm/llvm-project/pull/137078.diff 3 Files Affected:
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 80ece3c4ed7e1..262c78458b78f 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -2234,10 +2234,15 @@ static bool ArePotentiallyOverlappingStringLiterals(const EvalInfo &Info,
// within RHS. We don't need to look at the characters of one string that
// would appear before the start of the other string if they were merged.
CharUnits Offset = RHS.Offset - LHS.Offset;
- if (Offset.isNegative())
+ if (Offset.isNegative()) {
+ if (LHSString.Bytes.size() < (size_t)-Offset.getQuantity())
+ return false;
LHSString.Bytes = LHSString.Bytes.drop_front(-Offset.getQuantity());
- else
+ } else {
+ if (RHSString.Bytes.size() < (size_t)Offset.getQuantity())
+ return false;
RHSString.Bytes = RHSString.Bytes.drop_front(Offset.getQuantity());
+ }
bool LHSIsLonger = LHSString.Bytes.size() > RHSString.Bytes.size();
StringRef Longer = LHSIsLonger ? LHSString.Bytes : RHSString.Bytes;
diff --git a/clang/test/AST/ByteCode/cxx20.cpp b/clang/test/AST/ByteCode/cxx20.cpp
index 42e6ae33e92e4..4c1b1592896c9 100644
--- a/clang/test/AST/ByteCode/cxx20.cpp
+++ b/clang/test/AST/ByteCode/cxx20.cpp
@@ -119,6 +119,15 @@ constexpr auto b3 = name1() == name1(); // ref-error {{must be initialized by a
constexpr auto b4 = name1() == name2();
static_assert(!b4);
+constexpr auto bar(const char *p) { return p + __builtin_strlen(p); }
+constexpr auto b5 = bar(p1) == p1;
+static_assert(!b5);
+constexpr auto b6 = bar(p1) == ""; // ref-error {{must be initialized by a constant expression}} \
+ // ref-note {{comparison of addresses of potentially overlapping literals}}
+constexpr auto b7 = bar(p1) + 1 == ""; // both-error {{must be initialized by a constant expression}} \
+ // ref-note {{comparison against pointer '&"test1"[6]' that points past the end of a complete object has unspecified value}} \
+ // expected-note {{comparison against pointer '&"test1"[6] + 1' that points past the end of a complete object has unspecified value}}
+
namespace UninitializedFields {
class A {
public:
diff --git a/clang/test/SemaCXX/constant-expression-cxx11.cpp b/clang/test/SemaCXX/constant-expression-cxx11.cpp
index c35f3a5632a05..ec154517949cc 100644
--- a/clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ b/clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2199,6 +2199,8 @@ namespace BuiltinStrlen {
static_assert(__builtin_strlen("foo") == 3, "");
static_assert(__builtin_strlen("foo\0quux") == 3, "");
static_assert(__builtin_strlen("foo\0quux" + 4) == 4, "");
+ static_assert(__builtin_strlen("foo") + 1 + "foo" == "foo", ""); // expected-error {{static assertion expression is not an integral constant expression}}
+ // expected-note@-1 {{comparison against pointer '&"foo"[4]' that points past the end of a complete object has unspecified value}}
constexpr bool check(const char *p) {
return __builtin_strlen(p) == 3 &&
|
zygoloid
approved these changes
Apr 23, 2025
There was a problem hiding this comment.
Thanks for the fix!
I was wondering if the new code paths should actually return true not false, but I think it makes sense that for a comparison like "hello" + 6 == "world", we say that the strings are not potentially-overlapping but we instead detect the case of comparing a past-the-end pointer to one complete object against the start of another complete object.
That's an interesting thought. I tried it out real quick to see what the difference would be. Instead of |
|
Yeah. I agree that using the overlap diagnostic only in the case where there's at least one byte of potential overlap (as this PR does) is best. |
hnrklssn
commented
Apr 23, 2025
| // ref-note {{comparison of addresses of potentially overlapping literals}} | ||
| constexpr auto b7 = bar(p1) + 1 == ""; // both-error {{must be initialized by a constant expression}} \ | ||
| // ref-note {{comparison against pointer '&"test1"[6]' that points past the end of a complete object has unspecified value}} \ | ||
| // expected-note {{comparison against pointer '&"test1"[6] + 1' that points past the end of a complete object has unspecified value}} |
There was a problem hiding this comment.
@tbaederr Not sure what causes the '&"test1"[6]' vs '&"test1"[6] + 1' discrepancy, but I thought I'd highlight it in case it isn't a known issue
There was a problem hiding this comment.
Hmm I may have been to quick with this, CI reports differently now. Reverting to check.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/23465 Here is the relevant piece of the build log for the reference |
hnrklssn
added a commit
that referenced
this pull request
Apr 23, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/21837 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/16533 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/15716 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/4024 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/133/builds/14998 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/15667 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/18865 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/13648 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/16675 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/29721 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/10325 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/24173 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/25516 Here is the relevant piece of the build log for the reference |
hnrklssn
added a commit
that referenced
this pull request
Apr 24, 2025
Relands #137078 after updating clang/test/AST/ByteCode/cxx20.cpp to account for diagnostic outputs that differ between Linux and macOS.
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
May 6, 2025
…the end" (#137088) Reverts llvm/llvm-project#137078
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
When `ArePotentiallyOverlappingStringLiterals`, added in llvm#109208, compares string literals it drops the front of the string with the greatest offset from its base pointer. The number of characters dropped is equal to the difference between the two strings' offsets from their base pointers. This would trigger an assert when the resulting offset is past the end of the object. Not only are one-past-the-end pointers legal constructs, the compiler should not crash even when faced with illegal constructs. rdar://149865910
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
Relands llvm#137078 after updating clang/test/AST/ByteCode/cxx20.cpp to account for diagnostic outputs that differ between Linux and macOS.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
When `ArePotentiallyOverlappingStringLiterals`, added in llvm#109208, compares string literals it drops the front of the string with the greatest offset from its base pointer. The number of characters dropped is equal to the difference between the two strings' offsets from their base pointers. This would trigger an assert when the resulting offset is past the end of the object. Not only are one-past-the-end pointers legal constructs, the compiler should not crash even when faced with illegal constructs. rdar://149865910
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
Relands llvm#137078 after updating clang/test/AST/ByteCode/cxx20.cpp to account for diagnostic outputs that differ between Linux and macOS.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
When `ArePotentiallyOverlappingStringLiterals`, added in llvm#109208, compares string literals it drops the front of the string with the greatest offset from its base pointer. The number of characters dropped is equal to the difference between the two strings' offsets from their base pointers. This would trigger an assert when the resulting offset is past the end of the object. Not only are one-past-the-end pointers legal constructs, the compiler should not crash even when faced with illegal constructs. rdar://149865910
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
Relands llvm#137078 after updating clang/test/AST/ByteCode/cxx20.cpp to account for diagnostic outputs that differ between Linux and macOS.
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
May 9, 2025
When `ArePotentiallyOverlappingStringLiterals`, added in llvm#109208, compares string literals it drops the front of the string with the greatest offset from its base pointer. The number of characters dropped is equal to the difference between the two strings' offsets from their base pointers. This would trigger an assert when the resulting offset is past the end of the object. Not only are one-past-the-end pointers legal constructs, the compiler should not crash even when faced with illegal constructs. rdar://149865910
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
May 9, 2025
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
May 9, 2025
Relands llvm#137078 after updating clang/test/AST/ByteCode/cxx20.cpp to account for diagnostic outputs that differ between Linux and macOS.
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.
When
ArePotentiallyOverlappingStringLiterals, added in #109208, compares string literals it drops the front of the string with the greatest offset from its base pointer. The number of characters dropped is equal to the difference between the two strings' offsets from their base pointers. This would trigger an assert when the resulting offset is past the end of the object. Not only are one-past-the-end pointers legal constructs, the compiler should not crash even when faced with illegal constructs.rdar://149865910