dsmiley commented on PR #4653:
URL: https://github.com/apache/solr/pull/4653#issuecomment-5093753371
I reviewed the code. I had suspicions the historic developments that lead
to it's current state had insights that the change here reverts. I had Claude
review and it came up with the following report:
----
Reviewed the write-side changes in `JavaBinCodec.writeStr` — found what
looks like a correctness regression in the large-string (>64KB) path.
**Potential infinite loop on malformed UTF-16 input**
The double-pass encode at `JavaBinCodec.writeStr` (the streaming branch for
strings whose worst-case UTF-8 size exceeds
`MAX_UTF8_SIZE_FOR_ARRAY_GROW_STRATEGY`) calls the low-level
`CharsetEncoder.encode(CharBuffer, ByteBuffer, boolean)` but never inspects the
returned `CoderResult`:
```java
while (!endOfInput) {
endOfInput = !cb.hasRemaining();
scratch.clear();
encoder.encode(cb, scratch, endOfInput); // result discarded
sz += scratch.position();
}
```
The default `CodingErrorAction` for a `CharsetEncoder` is `REPORT`. On
malformed input (e.g. an unpaired surrogate), `REPORT` returns a `MALFORMED`
result *without consuming the offending char* — so `cb`'s position never
advances, `cb.hasRemaining()` stays true forever, and this spins indefinitely
(verified locally: a 30k-char string with one unpaired surrogate at index 15000
loops forever, stuck at the same position every iteration).
The deleted `ByteUtils.writeUTF16toUTF8`/`UTF16toUTF8` explicitly detected
unpaired/out-of-order surrogates and substituted U+FFFD, which guaranteed
forward progress. That guard is gone here, so any large field value containing
an unpaired surrogate (corrupted imports, non-validating JSON/XML producing a
lone `\uD800`-range char, etc.) will hang the thread serializing it via javabin
— this reads as a DoS risk, not just a perf nit.
`testLongString` (added in this PR) uses `TestUtil.randomUnicodeString`,
which never produces unpaired surrogates, so it wouldn't catch this. Might be
worth adding a case with a deliberately malformed surrogate to cover it.
Suggested fix: check the `CoderResult` and handle
`isMalformed()`/`isUnmappable()` explicitly (e.g. write a replacement char and
advance past the bad input), mirroring what the old `ByteUtils` code did.
**Two smaller, lower-confidence points:**
- The small-string branch now allocates a fresh `byte[]` on every `writeStr`
call (`s.toString().getBytes(StandardCharsets.UTF_8)`) instead of reusing the
pooled `bytes` field the surrounding code maintains via `getBufferSize`. That
buffer is still reused on the read path and in the >64KB write path, just not
here. Might be a net win from the JIT intrinsic, but given this is the hot path
for the overwhelming majority of field writes, a quick JMH comparison against
the old buffer-reuse approach would be good to confirm it's actually a net
improvement rather than trading CPU for extra allocation/GC pressure.
- Also noting `String.getBytes(UTF_8)`'s default replacement for an unpaired
surrogate is a single `0x3F` ('?'), vs. the old code's 3-byte U+FFFD (`EF BF
BD`). Minor, silent wire-format behavior change for malformed data, probably
fine to ignore unless someone depends on it.
----
IMO the allocation is probably a non-issue as I assume it'd be
stack-allocated -- the JVM is very smart
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
