ReviewOS

also looking at this

craft-native/craft

fix(android): text going to the JVM is modified UTF-8, and was not being

#168
Merged chris wants to merge fix/android-modified-utf8 into main
12 files +326 -136
9 days ago

NewStringUTF reads Java's modified UTF-8. Every migrated action has been handing it standard UTF-8, which is the same encoding only for ASCII and the BMP.

Two places it is not:

  • U+0000. Java writes it as C0 80 precisely so a string can be NUL-terminated. Passed through as a single 0x00 byte it is the terminator — the string ends there and nothing says so. A database column holding a NUL is the everyday way to hit this, and it truncates the reply script, so evaluateJavascript runs the fragment before it.
  • Anything outside the BMP. Java holds an emoji as a UTF-16 surrogate pair — two three-byte sequences. Standard UTF-8 has one four-byte sequence. NewStringUTF given the four-byte form does not produce the character that was meant.

The file already knew

jni_runtime.zig has had decodeModifiedUtf8 since the JNI layer landed, and newStringUtf carried this comment:

Callers pass NUL-terminated literals and ASCII package names today, which are identical in both encodings; encodeModifiedUtf8 is the function to add when that stops being true, and this is where it goes.

That stopped being true several migrations ago. Nobody noticed because the affected text is never a literal in this repository — it is a contact's name, a widget's title, a calendar event, a SQL parameter, a database value.

What changed

encodeModifiedUtf8 and newStringUtf8, and every site carrying text that did not come from a literal now goes through them:

  • the whole reply channel — android_events.evaluate takes a slice, so every resolve and reject on Android is re-encoded;
  • the clipboard, the log, share, openURL, the secure store;
  • the four modules added this week (calendar, database, shared items, contacts, widgets).

newStringUtf stays, for literals, and its doc now says that is what it is for.

Two things found on the way

share read out of bounds on an empty title. It tested title[0] != 0 — correct for a [*:0]const u8, an out-of-bounds read for a slice. The existing empty-title test caught it the moment the signature changed, which is the argument for that test existing.

The intents fake stored the pointer NewStringUTF was given. A real NewStringUTF copies into the JVM's heap and licenses the caller to free immediately — which is exactly what the re-encoder does. So the fake was reading freed memory as soon as its input stopped being a literal. It copies now.

What the tests can and cannot show

The round trip through encodeModifiedUtf8 / decodeModifiedUtf8 is the property that matters, but it cannot catch a shared misunderstanding of the format: our decoder accepts the four-byte form our encoder should never emit, so a round trip would stay green with the surrogate branch removed.

So the surrogate and NUL cases are asserted as bytes as well, and android_events asserts them end to end through settle — a reply containing 🙂 must arrive as ED A0 BD ED B9 82 with no four-byte form left in it. Deleting the surrogate branch fails all three tests; that mutation was run before this was committed.

Testing

zig build test:android passes. Both libcraft.so targets still build with no NDK.

12 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.