Fix decoder on broken utf8 sequences.

[phprus]

Issue: #3038

utf8_decode did not check if the first byte is correct.
for_each_codepoint did not check for overflow on broken codepoint.

cc @stevenzzzz @dyfrgi

[vitaut]

Thanks for the PR.

[vitaut]

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

[phprus]

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.

[vitaut]

Could you add a test case then?

[vitaut]

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

[phprus]

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

[vitaut]

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

[phprus]

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

[vitaut]

Makes sense, thanks.

[stevenzzzz]

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

[stevenzzzz]

could you help me to understand in the example of "\xF0\x28", is this end still buf_ptr+4 even tho there is an error?

[phprus]

@stevenzzzz
No. On invalid utf-8 sequence, processing is one byte at a time.

Please describe your testing process.

My environment:
macOS 12.5.1
Apple clang version 13.1.6 (clang-1316.0.21.2.5)

Test steps:

CXXFLAGS="-fsanitize=address" cmake -DCMAKE_VERBOSE_MAKEFILE=ON -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_STANDARD=20 -DFMT_PEDANTIC=ON -DFMT_WERROR=ON /path/to/patched/fmt
make
./bin/ranges-test

The test shows no errors.

String "\xF0\x28" present in updated test.

Without this PR:

ERROR: AddressSanitizer: negative-size-param: (size=-2)

[stevenzzzz]

I might have overlooked something, but for "\xF0\x28\0\0anything" the utf8_decode_new/decode would return 4 (as expected), when the error is set to 42. Is that "4" gonna skip the "\x28\0\0"?

same test: https://godbolt.org/z/EbeEex4Gf

[phprus]

@stevenzzzz
If error != 0 then return value of the utf8_decode function is not used and one byte is skipped (pushed to output in "\xVV" format).

I added a test for the string "\xf0\x28\0\0anything" and it finished without ASAN error and with expected results: \"\\xf0(\\x00\\x00anything\".

Please provide full test source for ASAN errors.

[stevenzzzz]

thanks for working on this!

We got the ASAN error by printing a protobuf debugstring() which is arbitrary binary bytes, unfortunately I can't show the protobuf to you.

I am a little bit lost on how the printing works, could you help me to understand where is the "skip one byte on error" pls? per this piece of code:

    for (auto end = p + s.size() - block_size + 1; p < end;) {
      p = decode(p, p);
      if (!p) return;
    }

it seems p would advance 4 bytes when encode(p, p) is called on "\xof\x28".

[phprus]

@stevenzzzz
Please replace

  auto decode = [f](const char* buf_ptr, const char* ptr) {
    auto cp = uint32_t();
    auto error = 0;
    auto end = utf8_decode(buf_ptr, &cp, &error);
    bool result = f(error ? invalid_code_point : cp,
                    string_view(ptr, error ? 1 : to_unsigned(end - buf_ptr)));
    return result ? end : nullptr;
  };

with:

  auto decode = [f](const char* buf_ptr, const char* ptr) {
    auto cp = uint32_t();
    auto error = 0;
    auto end = utf8_decode(buf_ptr, &cp, &error);
    bool result = f(error ? invalid_code_point : cp,
                    string_view(ptr, error ? 1 : to_unsigned(end - buf_ptr)));
    return result ? (error ? buf_ptr + 1 : end) : nullptr;
  };

and run test.

[stevenzzzz]

I was wondering why you didn't replace that in this PR.

[phprus]

@stevenzzzz
Did this replacement fix the bug? If yes, I will update the PR.

I didn't see this bug before.

[vitaut]

Merged, thanks! @phprus, could you report this to https://github.com/skeeto/branchless-utf8 where this implementation originates from?

[phprus]

@vitaut : skeeto/branchless-utf8#8