Skip to content

Commit fb14e6c

Browse files
committed
[SPARK-57520][SQL] Fix UTF8String.codePointFrom and copyUTF8String reading past the end of a truncated trailing UTF-8 sequence
### What changes were proposed in this pull request? `UTF8String.codePointFrom` decodes a code point by reading `numBytesForFirstByte(leader)` continuation bytes, and `copyUTF8String` copies `end - start + 1` bytes. Neither bounds the read by the bytes that actually remain, so when a string ends in a truncated multi-byte sequence (a leader byte whose declared width exceeds the remaining bytes), both read past the end of the backing memory. `trimLeft`/`trimRight` build their search character through `copyUTF8String`, so they over-read too. This PR: - `codePointFrom` reads continuation bytes through a small `continuationByte` helper that returns 0 once the index passes the end of the string. - `copyUTF8String` clamps the copy length to `numBytes - start`. - Once `copyUTF8String` stops over-reading, `trimRight` needs a matching accounting fix: it advanced `trimEnd` by the leader's declared width, which overshoots a truncated trailing character, so it now uses the actual (clamped) byte count, as `trimLeft` already does. ### Why are the changes needed? `UTF8String` can hold malformed UTF-8 (for example, bytes from binary coercion or truncated input). For a string ending in an incomplete multi-byte sequence, these methods read out of bounds and produced wrong results: `codePointFrom` assembled a code point from adjacent memory, and `trimRight` could drop valid leading characters. Well-formed UTF-8 is unaffected, since a complete sequence never exceeds the remaining bytes. This is a follow-up to SPARK-57507, which fixed the same kind of over-read in `reverse()`. ### Does this PR introduce _any_ user-facing change? Yes, it fixes incorrect results on malformed input. String operations that reach these methods (such as trimming or code-point access) no longer read past the end of a value that ends in a truncated multi-byte sequence; only previously-incorrect results change. Well-formed strings behave exactly as before. ### How was this patch tested? Added cases to `UTF8StringSuite`: - `testCodePointFrom`: truncated trailing 2-, 3-, and 4-byte leaders, including a 4-byte leader with only the last continuation byte missing. - `copyUTF8StringClampsToRemainingBytes`: an `end` one past the last byte, with a non-zero start so the clamp must use `numBytes - start`. - `trimTruncatedTrailingSequence`: trimming a truncated trailing leader keeps the valid preceding character. Each uses a sliced backing array with a trailing sentinel byte, so the previous over-read produces a deterministically wrong value; the cases fail on the old code and pass with the fix. `build/sbt 'unsafe/testOnly *UTF8StringSuite'` passes (51 tests). ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 4.8) Closes #56585 from LuciferYang/SPARK-57520-utf8-overread. Authored-by: YangJie <yangjie01@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com> (cherry picked from commit 33ae7f6) Signed-off-by: yangjie01 <yangjie01@baidu.com>
1 parent f5981e8 commit fb14e6c

2 files changed

Lines changed: 79 additions & 8 deletions

File tree

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -724,7 +724,8 @@ public int getChar(int charIndex) {
724724

725725
/**
726726
* Returns the code point starting from the byte at position `byteIndex`.
727-
* If byte index is invalid, throws exception.
727+
* If byte index is invalid, throws exception. If the sequence is truncated (the leader byte
728+
* declares more bytes than remain), the missing continuation bytes are treated as 0.
728729
*/
729730
public int codePointFrom(int byteIndex) {
730731
if (byteIndex < 0 || byteIndex >= numBytes) {
@@ -736,18 +737,28 @@ public int codePointFrom(int byteIndex) {
736737
case 1 ->
737738
b & 0x7F;
738739
case 2 ->
739-
((b & 0x1F) << 6) | (getByte(byteIndex + 1) & 0x3F);
740+
((b & 0x1F) << 6) | continuationByte(byteIndex + 1);
740741
case 3 ->
741-
((b & 0x0F) << 12) | ((getByte(byteIndex + 1) & 0x3F) << 6) |
742-
(getByte(byteIndex + 2) & 0x3F);
742+
((b & 0x0F) << 12) | (continuationByte(byteIndex + 1) << 6) |
743+
continuationByte(byteIndex + 2);
743744
case 4 ->
744-
((b & 0x07) << 18) | ((getByte(byteIndex + 1) & 0x3F) << 12) |
745-
((getByte(byteIndex + 2) & 0x3F) << 6) | (getByte(byteIndex + 3) & 0x3F);
745+
((b & 0x07) << 18) | (continuationByte(byteIndex + 1) << 12) |
746+
(continuationByte(byteIndex + 2) << 6) | continuationByte(byteIndex + 3);
746747
default ->
747748
throw new IllegalStateException("Error in UTF-8 code point");
748749
};
749750
}
750751

752+
/**
753+
* Returns the low 6 bits of the UTF-8 continuation byte at `byteIndex`, or 0 when `byteIndex`
754+
* is past the end of the string. The bounds check stops a truncated trailing multi-byte
755+
* sequence (a leader byte whose declared width exceeds the bytes that remain) from reading
756+
* past the end of the backing memory.
757+
*/
758+
private int continuationByte(int byteIndex) {
759+
return byteIndex < numBytes ? getByte(byteIndex) & 0x3F : 0;
760+
}
761+
751762
public boolean matchAt(final UTF8String s, int pos) {
752763
if (s.numBytes + pos > numBytes || pos < 0) {
753764
return false;
@@ -944,7 +955,10 @@ public int findInSet(UTF8String match) {
944955
* @return a new UTF8String in the position of [start, end] of current UTF8String bytes.
945956
*/
946957
public UTF8String copyUTF8String(int start, int end) {
947-
int len = end - start + 1;
958+
// Clamp to the bytes that actually remain so an out-of-range `end` (for example, derived
959+
// from a truncated trailing multi-byte sequence) can't copy past the end of the backing
960+
// memory.
961+
int len = Math.min(end - start + 1, numBytes - start);
948962
byte[] newBytes = new byte[len];
949963
copyMemory(base, offset + start, newBytes, BYTE_ARRAY_OFFSET, len);
950964
return UTF8String.fromBytes(newBytes);
@@ -1137,7 +1151,9 @@ public UTF8String trimRight(UTF8String trimString) {
11371151
stringCharPos[numChars - 1],
11381152
stringCharPos[numChars - 1] + stringCharLen[numChars - 1] - 1);
11391153
if (trimString.find(searchChar, 0) >= 0) {
1140-
trimEnd -= stringCharLen[numChars - 1];
1154+
// Advance by the bytes the character actually occupies. A truncated trailing leader is
1155+
// shorter than the width its leader byte declares, so use the (clamped) search char.
1156+
trimEnd -= searchChar.numBytes;
11411157
} else {
11421158
break;
11431159
}

common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1228,6 +1228,61 @@ public void testCodePointFrom() {
12281228
assertThrows(IndexOutOfBoundsException.class, () -> s.codePointFrom(-1));
12291229
assertThrows(IndexOutOfBoundsException.class, () -> s.codePointFrom(str.length()));
12301230
assertThrows(IndexOutOfBoundsException.class, () -> s.codePointFrom(str.length() + 1));
1231+
1232+
// Truncated trailing multi-byte sequence: the leader declares more bytes than remain.
1233+
// codePointFrom should decode only the bytes present (missing continuation bytes count as
1234+
// 0) and not read past the end. Each backing array has extra trailing bytes, so an
1235+
// over-read regression would show up in the result.
1236+
// 2-byte leader 0xCE with no continuation byte present.
1237+
assertEquals((0xCE & 0x1F) << 6,
1238+
fromBytes(new byte[] {(byte) 0xCE, 0x42}, 0, 1).codePointFrom(0));
1239+
// 3-byte leader 0xE4 with no continuation bytes present.
1240+
assertEquals((0xE4 & 0x0F) << 12,
1241+
fromBytes(new byte[] {(byte) 0xE4, 0x42, 0x43}, 0, 1).codePointFrom(0));
1242+
// 3-byte leader 0xE4 0xB8 with the final continuation byte missing.
1243+
assertEquals(((0xE4 & 0x0F) << 12) | ((0xB8 & 0x3F) << 6),
1244+
fromBytes(new byte[] {(byte) 0xE4, (byte) 0xB8, 0x42}, 0, 2).codePointFrom(0));
1245+
// 4-byte leader 0xF1 with no continuation bytes present.
1246+
assertEquals((0xF1 & 0x07) << 18,
1247+
fromBytes(new byte[] {(byte) 0xF1, 0x42, 0x43, 0x44}, 0, 1).codePointFrom(0));
1248+
// 4-byte leader 0xF1 with two continuation bytes present and only the last one missing,
1249+
// so just the final read crosses the end.
1250+
assertEquals(((0xF1 & 0x07) << 18) | ((0x9F & 0x3F) << 12) | ((0x8F & 0x3F) << 6),
1251+
fromBytes(new byte[] {(byte) 0xF1, (byte) 0x9F, (byte) 0x8F, 0x42}, 0, 3).codePointFrom(0));
1252+
}
1253+
1254+
@Test
1255+
public void copyUTF8StringClampsToRemainingBytes() {
1256+
// Here `end` runs one byte past the string, as it would for a truncated trailing sequence.
1257+
// copyUTF8String should clamp to the available bytes; the extra backing byte would show up
1258+
// in the result if it over-read.
1259+
byte[] backing = new byte[] {0x41, 0x42, 0x43};
1260+
UTF8String s = fromBytes(backing, 0, 2); // views "AB"
1261+
// `end` (2) is one past the last valid byte index (1); only the two real bytes are copied.
1262+
assertEquals(fromString("AB"), s.copyUTF8String(0, 2));
1263+
// Same with a non-zero start, so the clamp uses `numBytes - start`, not `numBytes`.
1264+
assertEquals(fromString("B"), s.copyUTF8String(1, 2));
1265+
// In-bounds copies are unaffected.
1266+
assertEquals(fromString("AB"), s.copyUTF8String(0, 1));
1267+
assertEquals(fromString("B"), s.copyUTF8String(1, 1));
1268+
}
1269+
1270+
@Test
1271+
public void trimTruncatedTrailingSequence() {
1272+
// trimLeft/trimRight build the search character through copyUTF8String, so an over-read would
1273+
// make it longer than the bytes that remain. The backing arrays carry an extra trailing byte
1274+
// to make any over-read deterministic.
1275+
// A lone truncated 2-byte leader (0xC2): the clamped search char is just the leader, which
1276+
// matches the 1-byte trim set and is trimmed away.
1277+
UTF8String lone = fromBytes(new byte[] {(byte) 0xC2, 0x42}, 0, 1);
1278+
UTF8String trim2 = fromBytes(new byte[] {(byte) 0xC2});
1279+
assertEquals(EMPTY_UTF8, lone.trimLeft(trim2));
1280+
assertEquals(EMPTY_UTF8, lone.trimRight(trim2));
1281+
// 'A' followed by a truncated 3-byte leader (0xE4). Trimming the leader from the right must
1282+
// keep 'A': the trailing character occupies only one real byte, so only that byte is removed.
1283+
UTF8String prefixed = fromBytes(new byte[] {0x41, (byte) 0xE4, 0x42}, 0, 2);
1284+
UTF8String trim3 = fromBytes(new byte[] {(byte) 0xE4});
1285+
assertEquals(fromString("A"), prefixed.trimRight(trim3));
12311286
}
12321287

12331288
@Test

0 commit comments

Comments
 (0)