Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix decoder on broken utf8 sequences. #3044

Merged
merged 1 commit into from
Aug 24, 2022
Merged

Fix decoder on broken utf8 sequences. #3044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions include/fmt/format.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,7 @@ FMT_CONSTEXPR FMT_NOINLINE auto copy_str_noinline(InputIt begin, InputIt end,
*/
FMT_CONSTEXPR inline auto utf8_decode(const char* s, uint32_t* c, int* e)
-> const char* {
constexpr const int prefix_masks[] = {0x00, 0x80, 0xe0, 0xf0, 0xf8};
constexpr const int masks[] = {0x00, 0x7f, 0x1f, 0x0f, 0x07};
constexpr const uint32_t mins[] = {4194304, 0, 128, 2048, 65536};
constexpr const int shiftc[] = {0, 18, 12, 6, 0};
Expand All @@ -628,6 +629,8 @@ FMT_CONSTEXPR inline auto utf8_decode(const char* s, uint32_t* c, int* e)
*e |= uchar(s[3]) >> 6;
*e ^= 0x2a; // top two bits of each tail byte correct?
*e >>= shifte[len];
*e |= ((uchar(s[0]) & prefix_masks[len]) !=
uchar((prefix_masks[len] << 1) & 0xFF)); // first byte correct?
Comment on lines +632 to +633
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed because utf8_decode already gives an error for "\xf0\x28...".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this check failed at sequence \xf4\x8f\xbf\xc0.

/Users/phprus/Devel/fmt/test/ranges-test.cc:393: Failure
Expected equality of these values:
  fmt::format("{}", vec{"\xf4\x8f\xbf\xc0"})
    Which is: "[\"\\xf4\\x0f\xBF\\xc0\"]"
  "[\"\\xf4\\x8f\\xbf\\xc0\"]"

utf8_decode return zero in e.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test case then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I missed the code snippet which shows it's already there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test already present:

fmt/test/ranges-test.cc

Lines 383 to 384 in 541cd21

EXPECT_EQ(fmt::format("{}", vec{"\xf4\x8f\xbf\xc0"}),
"[\"\\xf4\\x8f\\xbf\\xc0\"]");

This patch fix two error.
First error fixed by add condition error ? 1 : to_unsigned(end - buf_ptr)
Second - by add first byte check.

The first error masked the second on the sequence \xf4\x8f\xbf\xc0

Copy link
Contributor

@vitaut vitaut Aug 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICS the current version of utf8_decode already returns an error for "\xf4\x8f\xbf\xc0" too: https://godbolt.org/z/E194qvshK

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Step 1:
"\xf4\x8f\xbf\xc0" - is not valid utf-8, print \xf4, remove \xf4 from sequence.
Step 2:
"\x8f\xbf\xc0\0" - is not valid utf-8, but utf8_decode return no error and invalid codepoint (15, not 143).

See: https://godbolt.org/z/EbeEex4Gf

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks.


return next;
}
Expand All @@ -643,8 +646,8 @@ FMT_CONSTEXPR void for_each_codepoint(string_view s, F f) {
auto error = 0;
auto end = utf8_decode(buf_ptr, &cp, &error);
bool result = f(error ? invalid_code_point : cp,
string_view(ptr, to_unsigned(end - buf_ptr)));
return result ? end : nullptr;
string_view(ptr, error ? 1 : to_unsigned(end - buf_ptr)));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we saw error here "end - buf_ptr" is < 0, but I dont follow how that could happen with invalid code point.

return result ? (error ? buf_ptr + 1 : end) : nullptr;
};
auto p = s.data();
const size_t block_size = 4; // utf8_decode always reads blocks of 4 chars.
Expand Down
7 changes: 7 additions & 0 deletions test/ranges-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,8 +380,15 @@ TEST(ranges_test, escape_string) {
EXPECT_EQ(fmt::format("{}", vec{"\xcd\xb8"}), "[\"\\u0378\"]");
// Unassigned Unicode code points.
EXPECT_EQ(fmt::format("{}", vec{"\xf0\xaa\x9b\x9e"}), "[\"\\U0002a6de\"]");
// Broken utf-8.
EXPECT_EQ(fmt::format("{}", vec{"\xf4\x8f\xbf\xc0"}),
"[\"\\xf4\\x8f\\xbf\\xc0\"]");
EXPECT_EQ(fmt::format("{}", vec{"\xf0\x28"}), "[\"\\xf0(\"]");
EXPECT_EQ(fmt::format("{}", vec{"\xe1\x28"}), "[\"\\xe1(\"]");
EXPECT_EQ(fmt::format("{}", vec{std::string("\xf0\x28\0\0anything", 12)}),
"[\"\\xf0(\\x00\\x00anything\"]");

// Correct utf-8.
EXPECT_EQ(fmt::format("{}", vec{"понедельник"}), "[\"понедельник\"]");
}
}
Expand Down