THRIFT-6300: Read a whole value before decoding it in TBinaryProtocol - #3915

Client: st

TTransport>>readAll: asks read: for the bytes it still needs and
appends whatever comes back, until it has enough:

    [str size < anInteger] whileTrue:
        [str nextPutAll: (self read: anInteger - str size)]

A read: that answers an empty string appends nothing, and asking the
same source again for the same bytes cannot answer differently, so the
loop has no way to end.

Nothing in thrift.st answers an empty string from read: today -
TSocket>>read: signals instead - so this is not reachable as the library
stands. It is a contract any new or third-party transport can break, and
breaking it hangs the caller rather than raising. c_glib had the same
shape in thrift_transport_real_read_all() and was given an explicit
progress check.

readAll: now signals a TTransportError when read: answers nothing,
which is what TSocket>>read: already does for the same condition.

The tests are in a new TTransportReadAllTest suite, driven by two stub
transports: one that answers nothing at all, one that answers at most a
fixed number of bytes per call. They pin down that a stalled transport
raises instead of spinning, that it is asked exactly once rather than
repeatedly, that bytes arriving before the source dries up still end in
an error rather than a short result, and - the case the loop exists for
- that a transport answering in chunks is still assembled correctly.

Each of them runs inside valueWithin:onTimeout:, because the failure
being tested for is a hang, not an exception. Without that guard a
regression would not report a failure at all: it would sit there until
the CI job hit its own timeout.

Against the unmodified library the three stalled cases fail and the
three others pass; with the fix all six pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Client: st

readInt:, readRawInt:, readString and readByte each took whatever one
transport read: answered and decoded that. A read: is allowed to answer
fewer bytes than it was asked for - that is ordinary socket behaviour,
and TTransport has readAll: to loop until the rest arrives - but none of
the four used it.

The result is not an exception but a wrong answer. intFromByteArray:
takes its width from the size of the buffer it was handed rather than
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 is taken from
whatever byte happens to be first. Measured, with two of four bytes
delivered: readI32 answers 32767 where the correct value is 2147483647.
readString checks the declared size against the string limit but not how
many bytes arrived, so it can answer a truncated string as if it were
complete.

The four now read through readAll:.

That needed more than routing, contrary to what the issue suggested.
readAll: accumulated into a String, and a real transport does not answer
one: TSocket connects 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, so nothing had ever exercised
it against a real transport. It now builds its result with the species
of what read: answered, and clamps to the requested size.

The tests drive a transport that holds the whole wire content but
answers at most n bytes per call, which is what a socket does. They
assert the decoded VALUE rather than merely that something was raised,
because the broken behaviour is a plausible wrong number: a test that
only checked for a raise would pass on the unfixed code.

With the four call sites reverted and the reworked readAll: kept, 9 of
the 10 cases fail; with them routed, all 10 pass. The suites together
are 33 tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>