fs/ext: Fix encoding for surrogate pairs #535

2026-06-28T14:47:51Z

Matou1306 commented

2026-06-28 14:47:51 +00:00

Replaced utf8to16 calls with unicode.utfX.encode/decode.
Created umka test 87 similar to xfs' t011

Replaced `utf8to16` calls with `unicode.utfX.encode/decode`. Created umka test 87 similar to xfs' t011

Matou1306 requested review from Burer 2026-06-28 15:03:05 +00:00

Matou1306 requested review from Doczom 2026-06-28 15:03:05 +00:00

Matou1306 requested review from dunkaist 2026-06-28 15:03:06 +00:00

Burer requested changes 2026-06-28 16:32:11 +00:00

Dismissed

Burer left a comment

Logic looks correct, but I suspect one real problem - infinite loop on non-UTF-8 / malformed names.

unicode.utf8.decode's .error path doesn't advance esi or decrement ecx, and the loop only checks ecx:

@@: call    unicode.utf8.decode   ; error -> esi/ecx unchanged
    ...
    test    ecx, ecx
    jnz     @b                    ; never reaches 0 -> hang

ext filenames are arbitrary bytes (not guaranteed UTF-8), so a non-UTF-8 name hangs the kernel. The old cmp esi, ecx loop was bounded. Same pattern exists in xfs.asm, so the real fix is in unicode.utf8.decode (guarantee forward progress on error).

Minor - inconsistent label names: .utf16_loop_next vs .utf16_no_high for the same construct.

Logic looks correct, but I suspect one real problem - infinite loop on non-UTF-8 / malformed names. `unicode.utf8.decode`'s `.error` path doesn't advance `esi` or decrement `ecx`, and the loop only checks `ecx`: ```asm @@: call unicode.utf8.decode ; error -> esi/ecx unchanged ... test ecx, ecx jnz @b ; never reaches 0 -> hang ``` ext filenames are arbitrary bytes (not guaranteed UTF-8), so a non-UTF-8 name hangs the kernel. The old `cmp esi, ecx` loop was bounded. Same pattern exists in `xfs.asm`, so the real fix is in `unicode.utf8.decode` (guarantee forward progress on error). Minor - inconsistent label names: `.utf16_loop_next` vs `.utf16_no_high` for the same construct.

❤️ 1

Matou1306 force-pushed unicode from f20d813657 to 1c0525b8f7

2026-06-28 22:41:56 +00:00

Compare

Matou1306 force-pushed unicode from 1c0525b8f7 to f20d813657

2026-06-28 22:43:41 +00:00

Compare

Matou1306 force-pushed unicode from f20d813657 to 6868dd2019

2026-06-28 23:02:52 +00:00

Compare

Matou1306 commented

2026-06-28 23:05:58 +00:00

@Burer Please let me know if that's what you meant, and if there is anything else.

❤️ 1

Burer commented

2026-06-29 13:50:39 +00:00

@Matou1306

The hang fix and label unification look good. But making .error advance without sanitizing eax turns the old hang into an out-of-bounds write in the UTF-16 path.

OOB write**. On error, unicode.utf8.decode leaves the partial codepoint in eax, which can be >= 0x10000 (an error after the shl eax, 6 stages in .read4) while consuming only 1 byte. unicode.utf16.encode then emits a 2-word surrogate pair for that garbage. ext names are arbitrary bytes (not guaranteed UTF-8), so a crafted name like F4 90 repeated yields ~1.5 UTF-16 units/byte - a 254-byte name writes ~380 units (~760 bytes) into the 520-byte (260-unit) name field, overflowing it in both ext_ReadFolder and ext_GetFileInfo. Valid UTF-8 is unaffected (<= 1 unit/byte). The same decode is shared with xfs.asm, so xfs has it too.

Fix at the root so each consumed byte yields exactly one unit:

.error:
        mov     eax, 0xFFFD    ; replacement char
        dec     ecx
        inc     esi
.done:
        ret

This also makes the cp866 path emit a proper replacement instead of a garbage byte.

Also, a few minor problems remains, just for your info:

Lone surrogate. decode accepts 3-byte sequences encoding the surrogate range U+D800-U+DFFF (e.g. ED A0 80); unicode.utf16.encode returns those unchanged, emitting a lone surrogate (invalid UTF-16). One word, no overflow. The fix above doesn't cover it (decode treats it as success) - either reject surrogates in decode or emit a replacement in utf16.encode's .error.

Out of scope. fat, ntfs, iso9660, exfat, fs_lfn, gui/font, taskman still use the old utf8to16, so they keep the supplementary-char limitation this PR fixes for ext. Would be nice to fix them as well in future.

@Matou1306 The hang fix and label unification look good. But making `.error` advance without sanitizing `eax` turns the old hang into an **out-of-bounds write** in the UTF-16 path. OOB write**. On error, `unicode.utf8.decode` leaves the partial codepoint in `eax`, which can be `>= 0x10000` (an error after the `shl eax, 6` stages in `.read4`) while consuming only 1 byte. `unicode.utf16.encode` then emits a **2-word surrogate pair** for that garbage. ext names are arbitrary bytes (not guaranteed UTF-8), so a crafted name like `F4 90` repeated yields ~1.5 UTF-16 units/byte - a 254-byte name writes ~380 units (~760 bytes) into the 520-byte (260-unit) name field, overflowing it in both `ext_ReadFolder` and `ext_GetFileInfo`. Valid UTF-8 is unaffected (`<= 1` unit/byte). The same `decode` is shared with `xfs.asm`, so xfs has it too. Fix at the root so each consumed byte yields exactly one unit: ```asm .error: mov eax, 0xFFFD ; replacement char dec ecx inc esi .done: ret ``` This also makes the cp866 path emit a proper replacement instead of a garbage byte. Also, a few minor problems remains, just for your info: **Lone surrogate**. `decode` accepts 3-byte sequences encoding the surrogate range U+D800-U+DFFF (e.g. `ED A0 80`); `unicode.utf16.encode` returns those unchanged, emitting a lone surrogate (invalid UTF-16). One word, no overflow. The fix above doesn't cover it (decode treats it as success) - either reject surrogates in `decode` or emit a replacement in `utf16.encode`'s `.error`. **Out of scope**. `fat`, `ntfs`, `iso9660`, `exfat`, `fs_lfn`, `gui/font`, `taskman` still use the old `utf8to16`, so they keep the supplementary-char limitation this PR fixes for ext. Would be nice to fix them as well in future.

Matou1306 commented

2026-06-30 10:58:05 +00:00

@Burer thanks, please take a look at this.
Btw the new build system looks awesome! I noticed it built significantly faster now.

@Burer thanks, please take a look at this. Btw the new build system looks awesome! I noticed it built significantly faster now.

Burer commented

2026-06-30 18:54:12 +00:00

@Matou1306

The error hardening looks correct now, thank you.
One real bug remains, and it's the case this PR targets:

unicode.utf16.encode.write2 swaps the high/low 10 bits of the surrogate pair.

.write2:
        sub     eax, 0x10000
        shl     eax, 6
        shr     ax, 6
        or      eax, 0xdc00d800   ; gives low=0xD800+L, high=0xDC00+H (reversed)

After shl eax,6 / shr ax,6 the low word holds L (bottom 10 bits) and the high word holds H (top 10). The or then pairs the lead base 0xD800 with L and the trail base 0xDC00 with H. It must be lead = 0xD800+H, trail = 0xDC00+L. The code emits the low word first (stosw):

codepoint	correct	this code
U+10001	`D800 DC01`	`D801 DC00` (= U+10400)
U+1F600	`D83D DE00`	`DA00 DC3D` (= U+9003D)

(Only H==L codepoints like U+10000 / U+10FFFF come out right by coincidence.) Every supplementary-plane name on the UTF-16 path - ext_ReadFolder, ext_GetFileInfo ([edx+4]==2), and xfs.asm via the shared encoder - decodes to the wrong character. The size is correct (2 units), so no overflow - value bug only. The math is pre-existing, but this is the first PR to actually write both words.

Fix should be simple, but please, double-check this before applying:

        shr     ax, 6
        ror     eax, 16          ; low=H, high=L
        or      eax, 0xdc00d800

Verified by replicating the exact bit-ops: the current code is wrong for U+10001 / U+10400 / U+1F600; with ror all of U+10000 / 10001 / 10400 / 1F600 / 10FFFF match the reference values.

Minor: the empty-input path now falls through to .done with eax unset - correct to avoid the dec ecx underflow, but a zero-length name (corrupt image) then writes one garbage unit. Setting eax, 0xFFFD there too, or guarding ecx > 0 in the caller, would make it fully defensive.

@Matou1306 The error hardening looks correct now, thank you. One real bug remains, and it's the case this PR targets: **`unicode.utf16.encode.write2` swaps the high/low 10 bits of the surrogate pair.** ```asm .write2: sub eax, 0x10000 shl eax, 6 shr ax, 6 or eax, 0xdc00d800 ; gives low=0xD800+L, high=0xDC00+H (reversed) ``` After `shl eax,6` / `shr ax,6` the low word holds L (bottom 10 bits) and the high word holds H (top 10). The `or` then pairs the lead base `0xD800` with L and the trail base `0xDC00` with H. It must be lead = `0xD800+H`, trail = `0xDC00+L`. The code emits the low word first (`stosw`): | codepoint | correct | this code | |------------|---------------|------------------------------| | U+10001 | `D800 DC01` | `D801 DC00` (= U+10400) | | U+1F600 | `D83D DE00` | `DA00 DC3D` (= U+9003D) | (Only H==L codepoints like U+10000 / U+10FFFF come out right by coincidence.) Every supplementary-plane name on the UTF-16 path - `ext_ReadFolder`, `ext_GetFileInfo` (`[edx+4]==2`), and `xfs.asm` via the shared encoder - decodes to the wrong character. The size is correct (2 units), so no overflow - value bug only. The math is pre-existing, but this is the first PR to actually write both words. Fix should be simple, but please, double-check this before applying: ```asm shr ax, 6 ror eax, 16 ; low=H, high=L or eax, 0xdc00d800 ``` Verified by replicating the exact bit-ops: the current code is wrong for U+10001 / U+10400 / U+1F600; with `ror` all of U+10000 / 10001 / 10400 / 1F600 / 10FFFF match the reference values. **Minor:** the empty-input path now falls through to `.done` with `eax` unset - correct to avoid the `dec ecx` underflow, but a zero-length name (corrupt image) then writes one garbage unit. Setting `eax, 0xFFFD` there too, or guarding `ecx > 0` in the caller, would make it fully defensive.

Matou1306 commented

2026-07-04 18:32:25 +00:00

@Burer I just applied the change, thanks for spotting it! I couldn't notice it because umka tests were already popping unreadable characters because every line showed different encodings, so I didn't check whether the "unreadable" character was correct or not.

As for the suggestion in the end, extfs already guards ecx. Given it is already something that shouldn't happen and the result won't cause panic, I would lean more towards leaving the responsibility for the caller. Unless you think otherwise for sure.

ps: I realized building is failing for some reason, tup works fine locally idk why is that happening?

@Burer I just applied the change, thanks for spotting it! I couldn't notice it because umka tests were already popping unreadable characters because every line showed different encodings, so I didn't check whether the "unreadable" character was correct or not. As for the suggestion in the end, extfs already guards ecx. Given it is already something that shouldn't happen and the result won't cause panic, I would lean more towards leaving the responsibility for the caller. Unless you think otherwise for sure. ps: I realized building is failing for some reason, tup works fine locally idk why is that happening?

Burer force-pushed unicode from 75dca8e75e to 096227e3cc

2026-07-05 12:41:33 +00:00

Compare

Burer commented

2026-07-05 13:03:34 +00:00

@Matou1306

Looks okay after rebase on fresh main!

@Matou1306 Looks okay after rebase on fresh main!

❤️ 1

Burer approved these changes 2026-07-05 13:03:43 +00:00

dunkaist approved these changes 2026-07-05 15:54:45 +00:00

Doczom approved these changes 2026-07-05 22:57:13 +00:00

Burer added 4 commits 2026-07-06 04:43:24 +00:00

fix encoding for surrogate pairs 4b507ca355

prevent infinite loop + name consistency 2027e0a5af

fix oob write by setting unicode replacement character when invalid 6862164e6b

fix R/L mismatch

Check kernel codestyle / Check kernel codestyle (pull_request) Successful in 3m32s

Details

Test PR / Build (es_ES) (pull_request) Successful in 5m37s

Details

Test PR / Build (ru_RU) (pull_request) Successful in 5m41s

Details

Test PR / Build (en_US) (pull_request) Successful in 5m45s

Details

68d5ac736f

Burer force-pushed unicode from 096227e3cc to 68d5ac736f

2026-07-06 04:43:24 +00:00

Compare

Burer merged commit 91f5df9c53 into main

2026-07-06 05:39:59 +00:00

Burer deleted branch unicode

2026-07-06 05:39:59 +00:00

Sign in to join this conversation.

No Reviewers

Burer

dunkaist

Doczom

4 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KolibriOS/kolibrios#535

Allow edits from maintainers