feat: UTF8 to string (fromUTF8) - #906
Conversation
Signed-off-by: ldintr <levo.d@swirldslabs.com>
imalygin
left a comment
imalygin
left a comment
There was a problem hiding this comment.
Please, add unit tests.
| * @return the total number of {@code char}s written to {@code dst}, or {@code -1} if the | ||
| * input contains an illegal byte sequence (surrogate range or out-of-range codepoint) | ||
| */ | ||
| public static int fromUTF8(char[] dst, byte[] src, int offset, int pos, int length) { |
There was a problem hiding this comment.
How is this method supposed to be used? This "pos" argument looks like a caller has already partially parsed a string and needs to parse the remainder. That's expected for methods like the above "fromUTF8Tail()", which is private and only called from "fromUTF8()", but this method is different. Some examples (benchmarks, unit tests) would be nice to have
There was a problem hiding this comment.
It's used by PbjReader.readString, they use to be in the same commit but I was asked to split them
| if (i + 1 >= endPos) return -1; | ||
|
|
||
| int b = src[i + 1]; | ||
| if ((a & 0xE0) == 0xC0) { |
There was a problem hiding this comment.
This decoder validates the surrogate range and the > U+10FFFF ceiling, but it never rejects overlong (non-shortest) forms.
All three widths leak:
C0 AF→ decodes toU+002F/. The check(a & 0xE0) == 0xC0` admits lead bytes C0 and C1, which are always overlong.E0 80 AF → U+002F. AnyE0lead with a second byte< A0yields a codepoint <0x800.F0 80 80 AF → U+002F, and worse, it's collapsed into a single BMP char via the codepoint<= 0xFFFFbranch (4 bytes in, 1 char out). AnyF0lead with second byte < 90 is overlong.
I assume this method is replacing readString. JDK decoder rejects all three cases above, and Utf8Tools is documented as "byte for byte identical" to protoc (whose decoder also rejects overlong forms). So this isn't a stylistic strictness choice - accepting overlong forms is a behavioral regression against the exact contract the existing path was written to uphold, and it's the classic overlong-/ filter-bypass hazard.
There was a problem hiding this comment.
Great! Please add the fix to this branch.
|
is there a reason we're not catching 2/3/4-byte overlongs here? the old ProtoParserTools.readString ran a strict CharsetDecoder with onMalformedInput(REPORT) there's a comment on it saying that's deliberate so C0 80, E0 80 80, F0 80 80 80 all threw MalformedProtobufException and now they decode to U+0000. surrogates and >10FFFF are both still caught? It's only the min-value checks that are missing: fromUTF8 at 358 / 365 / 370, and the same three in fromUTF8Tail at 283 / 297 / 306. protobuf-java rejects overlongs too, so this puts us out of step with anything parsing the same bytes downstream, and it means two different byte sequences decode to the same string. |
|
At least one issue has been found in the PR (and fixed, but the commit with the fix is missing in the PR). How were these changes tested? How to make sure there are no more issues? Unit tests would be nice to have |
it looks like this is a fix for this but its only on the ldintr-addPbjReadWriteTest branch |
4 participants
Adds fromUTF8/fromUTF8Tail to ProtoParserTools, decoding raw UTF-8 bytes directly into a caller-supplied char[] (handling 1–4 byte sequences and surrogate pairs), returning -1 on malformed input instead of throwing. Groundwork for PbjReader.readString to avoid allocating an intermediate String/CharBuffer.
Overview: PbjReader/PbjWriter are a leaner, buffer-reusing alternative to the existing ReadableSequentialData/WritableSequentialData read/write path used by Codec, ProtoParserTools/ProtoWriterTools, and the pbj-compiler generators. They reuse an internal ~16KB (L1-cache-sized) buffer across many parse/write calls instead of allocating per call, avoid checked-exception-based control flow by recording a sticky internal error code instead, and split hot-path operations (e.g. zigzag vs. non-zigzag varints, direct UTF-8 decode into a reusable char[]) into dedicated fast methods.