[libc] Fixed StringConverter Error Edge Case #149356
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
…e is no space in dest
|
@llvm/pr-subscribers-libc Author: Uzair Nawaz (uzairnawaz) ChangesFixed StringConverter edge case related to destination limit If we call pop() but there is no space in the dest array, we should always Error(-1) even if the following character is invalid (since we shouldn't really have to look at the next character) Full diff: https://github.com/llvm/llvm-project/pull/149356.diff 2 Files Affected:
diff --git a/libc/src/__support/wchar/string_converter.h b/libc/src/__support/wchar/string_converter.h
index 0635bc57bf3e2..b4e6fde954235 100644
--- a/libc/src/__support/wchar/string_converter.h
+++ b/libc/src/__support/wchar/string_converter.h
@@ -56,11 +56,14 @@ template <typename T> class StringConverter {
// TODO: following functions are almost identical
// look into templating CharacterConverter pop functions
ErrorOr<char32_t> popUTF32() {
+ if (num_to_write == 0)
+ return Error(-1);
+
if (cr.isEmpty() || src_idx == 0) {
auto src_elements_read = pushFullCharacter();
if (!src_elements_read.has_value())
return Error(src_elements_read.error());
-
+
if (cr.sizeAsUTF32() > num_to_write) {
cr.clear();
return Error(-1);
@@ -79,6 +82,9 @@ template <typename T> class StringConverter {
}
ErrorOr<char8_t> popUTF8() {
+ if (num_to_write == 0)
+ return Error(-1);
+
if (cr.isEmpty() || src_idx == 0) {
auto src_elements_read = pushFullCharacter();
if (!src_elements_read.has_value())
diff --git a/libc/test/src/__support/wchar/string_converter_test.cpp b/libc/test/src/__support/wchar/string_converter_test.cpp
index 14d074156d033..d514df9317852 100644
--- a/libc/test/src/__support/wchar/string_converter_test.cpp
+++ b/libc/test/src/__support/wchar/string_converter_test.cpp
@@ -245,6 +245,63 @@ TEST(LlvmLibcStringConverterTest, UTF8To32ErrorHandling) {
ASSERT_EQ(static_cast<int>(sc.getSourceIndex()), 4);
}
+TEST(LlvmLibcStringConverterTest, InvalidCharacterOutsideBounds) {
+ // if an invalid character exists in the source string but we don't have space
+ // to write it, we should return a "stop converting" error rather than an
+ // invalid character error
+
+ // first 4 bytes are clown emoji (🤡)
+ // next 3 form an invalid character
+ const char *src1 = "\xF0\x9F\xA4\xA1\x90\x88\x30";
+ LIBC_NAMESPACE::internal::mbstate ps1;
+ LIBC_NAMESPACE::internal::StringConverter<char8_t> sc1(
+ reinterpret_cast<const char8_t *>(src1), &ps1, 1);
+
+ auto res1 = sc1.popUTF32();
+ ASSERT_TRUE(res1.has_value());
+ ASSERT_EQ(static_cast<int>(res1.value()), 0x1f921);
+ ASSERT_EQ(static_cast<int>(sc1.getSourceIndex()), 4);
+
+ res1 = sc1.popUTF32();
+ ASSERT_FALSE(res1.has_value());
+ // no space to write error NOT invalid character error (EILSEQ)
+ ASSERT_EQ(static_cast<int>(res1.error()), -1);
+ ASSERT_EQ(static_cast<int>(sc1.getSourceIndex()), 4);
+
+ const wchar_t src2[] = {
+ static_cast<wchar_t>(0x1f921), static_cast<wchar_t>(0xffffff),
+ static_cast<wchar_t>(0x0)}; // clown emoji, invalid utf32
+ LIBC_NAMESPACE::internal::mbstate ps2;
+ LIBC_NAMESPACE::internal::StringConverter<char32_t> sc2(
+ reinterpret_cast<const char32_t *>(src2), &ps2, 4);
+
+ auto res2 = sc2.popUTF8();
+ ASSERT_TRUE(res2.has_value());
+ ASSERT_EQ(static_cast<int>(res2.value()), 0xF0);
+ ASSERT_EQ(static_cast<int>(sc2.getSourceIndex()), 1);
+
+ res2 = sc2.popUTF8();
+ ASSERT_TRUE(res2.has_value());
+ ASSERT_EQ(static_cast<int>(res2.value()), 0x9F);
+ ASSERT_EQ(static_cast<int>(sc2.getSourceIndex()), 1);
+
+ res2 = sc2.popUTF8();
+ ASSERT_TRUE(res2.has_value());
+ ASSERT_EQ(static_cast<int>(res2.value()), 0xA4);
+ ASSERT_EQ(static_cast<int>(sc2.getSourceIndex()), 1);
+
+ res2 = sc2.popUTF8();
+ ASSERT_TRUE(res2.has_value());
+ ASSERT_EQ(static_cast<int>(res2.value()), 0xA1);
+ ASSERT_EQ(static_cast<int>(sc2.getSourceIndex()), 1);
+
+ res2 = sc2.popUTF8();
+ ASSERT_FALSE(res2.has_value());
+ // no space to write error NOT invalid character error (EILSEQ)
+ ASSERT_EQ(static_cast<int>(res2.error()), -1);
+ ASSERT_EQ(static_cast<int>(sc2.getSourceIndex()), 1);
+}
+
TEST(LlvmLibcStringConverterTest, MultipleStringConverters32To8) {
/*
We do NOT test partially popping a character and expecting the next
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
Fixed StringConverter edge case related to destination limit
If we call pop() but there is no space in the dest array, we should always return the "no space in destination" error even if the following character is invalid (since we shouldn't really have to look at the next character)