Jens-G opened a new pull request, #3915: URL: https://github.com/apache/thrift/pull/3915
> **Stacked on [#3914](https://github.com/apache/thrift/pull/3914) (THRIFT-6258) → [#3913](https://github.com/apache/thrift/pull/3913) (THRIFT-6063).** Review those first; this branch contains them. `readInt:`, `readRawInt:`, `readString` and `readByte` each took whatever one `transport read:` answered and decoded *that*. A `read:` is allowed to answer fewer bytes than asked for — ordinary socket behaviour — and `TTransport` has `readAll:` to loop until the rest arrives. None of the four used it. ## The harm is a wrong answer, not an exception `intFromByteArray:` takes its width from the size of the buffer it was handed, not from the number of bytes asked for. So a short read decodes to a well-formed integer that happens to be wrong, and the sign comes from whatever byte is first. Measured on this branch, with two of four bytes delivered: ``` readI32 with 2 of 4 bytes delivered -> 32767 (correct: 2147483647) ``` No error. A plausible number. `readString` is the same shape: it checks the *declared* size against the string limit but never how many bytes arrived, so it can answer a truncated string as if it were complete. That is why the tests assert the decoded **value** rather than merely that something was raised — a raise-only test passes on the unfixed code. ## The issue's suggested fix was not sufficient The ticket says *"Use `readAll:` in all three places. It already exists and already has the loop; nothing new has to be written."* That turns out not to hold: `readAll:` accumulated into a **String**, and a real transport does not answer one. `TSocket>>connect` sets its stream `binary`, so `read:` answers a **ByteArray**, and the protocol decodes it by shifting the elements as integers. Handing a ByteArray to a String stream raises: ``` Improper store into indexable object ``` `readAll:` had **no callers at all** — `grep` finds only its own definition — so nothing had ever run it against a real transport. Routing the decoders into it as-is would have swapped a wrong answer for a crash. So `readAll:` now builds its result with the species of whatever `read:` answered, and clamps to the requested size. A test pins that directly (`testReadAllKeepsTheSpeciesTheTransportAnswered`). ## Coverage New `TProtocolShortReadTest`, 10 cases, driven by a transport that holds the whole wire content but answers at most *n* bytes per call: - i32/i16 assembled across short reads, including the misleading case where the sign byte arrives but the magnitude does not (`-1` delivered 3 bytes at a time) - a peer that stops mid-value raises instead of answering a 3-byte integer - `readString` assembled across chunks; a string truncated by the peer raises instead of returning short - `readDouble`, which goes through `readRawInt:` twice - `readByte` on an exhausted transport raises `TTransportError` rather than a `doesNotUnderstand: #first` - `readAll:` preserves the transport's species ## Two-state verification With the four call sites reverted and the reworked `readAll:` kept — isolating exactly the routing — **9 of 10 fail**. With them routed, all 10 pass. (The tenth is the species test, which does not depend on routing.) Whole binding on this branch: **33 tests, all green.** ``` TProtocolStringSizeLimitTest 11 of 11 TTransportReadAllTest 6 of 6 TProtocolShortReadTest 10 of 10 TProtocolRecursionDepthTest 6 of 6 ``` ## Note on scope `readByte` is not named in the issue. It reads exactly one byte, so it cannot receive a short-but-non-empty result — but on an exhausted transport `(read: 1) first` is a `doesNotUnderstand`, not a transport error. It is one line in the same family and routing it makes the failure mode consistent; happy to drop it if you would rather keep strictly to the three. JIRA: [THRIFT-6300](https://issues.apache.org/jira/browse/THRIFT-6300) 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
