Jens Geyer created THRIFT-6166:
----------------------------------
Summary: C (GLib): bind the read budget to the frame that carries
the message
Key: THRIFT-6166
URL: https://issues.apache.org/jira/browse/THRIFT-6166
Project: Thrift
Issue Type: Bug
Components: C glib - Library
Reporter: Jens Geyer
Fix For: 0.25.0
h3. Problem
{{thrift_framed_transport_read_frame()}} has the exact size of the frame in
hand and never tells
the budget about it, so a protocol reading an eight-byte frame is still
measured against
{{maxMessageSize}} and may be talked into sizing a string body or a container
element count from a
number the frame comes nowhere near carrying.
Neither {{updateKnownMessageSize}} nor {{resetConsumedMessageSize}} is called
anywhere in the
c_glib library outside {{thrift_transport.c}} itself: the mechanism is there
and entirely unused.
The check the protocol makes before allocating -- nine call sites across
{{thrift_binary_protocol.c}} and {{thrift_compact_protocol.c}} -- therefore
passes.
h3. Change
Two parts.
* {{read_frame()}} resets the budget and then binds it to what the frame
delivered. The full reset
first is not optional: {{resetConsumedMessageSize()}} refuses to grow a budget,
so the frame after
a smaller one would be rejected outright. The bind uses
{{resetConsumedMessageSize()}} rather than
{{updateKnownMessageSize()}} for two reasons -- nothing has been consumed
against the budget that
was just reset, so the two are equivalent here, and
{{updateKnownMessageSize()}} would reject a
zero-length frame, which {{flush()}} with an empty write buffer produces and
which
{{testframedtransport}} already sends.
* {{thrift_framed_transport_read()}} checks the budget against the bytes the
buffer can deliver
rather than the bytes asked for. One frame is as far as a single read goes and
a short read is the
documented answer, so once the budget is one frame an ordinary "give me up to N
bytes" would
otherwise be refused from the second frame onwards. This does not loosen the
bound the protocol is
held to: every allocation-gating check calls {{checkReadBytesAvailable()}}
directly with the size
the wire declared and does not come through {{read()}}.
h3. Why this shape and not the C++ one
The endpoint decides. {{thrift_socket_read()}} checks the budget but never
decrements it, so a
framed transport that answered short from its own buffer instead of binding
would hand the
shortfall to an endpoint still holding the whole of {{maxMessageSize}}. Only
{{thrift_zlib_transport}} charges reads, and it is not on this path.
Nothing else in the binding needs the same treatment: c_glib ships only
{{thrift_simple_server}},
so unlike C++ there is no server that takes the frame apart itself and hands a
memory buffer
straight to the protocol.
h3. Tests
Five in a new {{testframedreadbudget}}, driving the framed transport over a
memory buffer holding
pre-built frames -- no sockets, no fork, no timing. Three fail against the
unmodified library
({{BudgetIsBoundToTheFrame}}, {{ALargerFrameMayFollowASmallerOne}},
{{ManyFramesDoNotExhaustTheBudget}}); two pass as regression guards
({{AReadLargerThanTheFrameIsShortNotRefused}},
{{AnOversizedFrameIsStillRefused}}). 22 of 22 ctest
cases pass, and the c_glib cross-language client and server complete five loops
with zero failures
over framed transport on both binary and compact.
Same defect as THRIFT-5371 in C++.
_Drafted with AI assistance (Claude Opus 5); filed by Jens Geyer._
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
