[
https://issues.apache.org/jira/browse/THRIFT-6166?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jens Geyer resolved THRIFT-6166.
--------------------------------
Assignee: Jens Geyer
Resolution: Fixed
> 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
> Assignee: Jens Geyer
> Priority: Major
> Fix For: 0.25.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> 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)
