[
https://issues.apache.org/jira/browse/THRIFT-6165?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jens Geyer resolved THRIFT-6165.
--------------------------------
Assignee: Jens Geyer
Resolution: Fixed
> Java: bind the read budget to the frame that carries the message
> ----------------------------------------------------------------
>
> Key: THRIFT-6165
> URL: https://issues.apache.org/jira/browse/THRIFT-6165
> Project: Thrift
> Issue Type: Bug
> Components: Java - Library
> Reporter: Jens Geyer
> Assignee: Jens Geyer
> Priority: Major
> Fix For: 0.25.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> h3. Problem
> {{TFramedTransport}} and {{TFastFramedTransport}} read a frame header that
> tells them the exact
> size of the message they are about to hand to the protocol, and never tell
> the budget.
> {{checkReadBytesAvailable}} is therefore measured against
> {{TConfiguration.maxMessageSize}} for
> the life of the connection, so a 68-byte frame may declare a 64 MB field and
> the array is
> allocated before the shortfall is found.
> h3. The API this needs
> Binding the budget to the frame needs an entry point that does not exist
> today.
> * {{resetConsumedMessageSize(-1)}} is the only genuine full reset, and it is
> {{protected}}, so a
> layered transport cannot call it on its inner transport.
> * {{updateKnownMessageSize()}} refuses to grow a budget an earlier, smaller
> frame narrowed, and
> carries that frame's consumption forward besides.
> So this adds {{resetMessageSizeAndConsumedBytes(long)}} to {{TTransport}},
> mirroring netstd's
> {{ResetMessageSizeAndConsumedBytes}}, implemented on {{TEndpointTransport}},
> delegated by
> {{TLayeredTransport}}, and a no-op on {{TFileTransport}} alongside its
> existing no-op budget
> methods.
> h3. Change
> {{readFrame()}} resets twice, for two different reasons.
> * *Before* the framing reads, because what is left of the previous frame's
> bound describes a frame
> we are done with, and an inner transport that decrements on read --
> {{TMemoryBuffer}}, or the
> {{TMemoryInputTransport}} the nonblocking server uses -- would otherwise
> refuse a frame larger
> than the last one.
> * *After* them, binding to the frame, discarding what the framing itself
> spent rather than
> charging it twice.
> {{TMemoryInputTransport.reset()}} binds to the buffer it is given, as its
> constructor already did.
> Reads there are clamped to the buffer and short-read rather than checked
> against the budget, so
> this only tightens what the protocol may declare.
> h3. Source compatibility
> The new abstract method is a source-compatibility break for anything
> extending {{TTransport}}
> directly rather than through {{TEndpointTransport}} or {{TLayeredTransport}}.
> Four test doubles in
> this repository needed it.
> h3. Tests
> Nine in {{TestFrameBoundReadBudget}}. Three fail against the unmodified
> library; the rest describe
> traffic that is legitimate today and must keep passing, because the natural
> implementation of frame
> binding breaks them. One records why the bound cannot go on the inner
> transport: with the
> nonblocking server's shape, getting the frame off the wire spends the inner
> budget entirely, and
> without the first reset the frame's own bytes are then unreadable. 346 tests
> pass.
> h3. Relationship to THRIFT-5371
> THRIFT-5371 reports the C++ half of this, and asks there whether Java is
> affected too. Partly: Java
> does *not* accumulate across frames the way C++ does, because
> {{TIOStreamTransport}} never
> decrements the budget on read -- its only reset is the {{flush()}} one on the
> write side. What Java
> does share is the other half, the budget never being narrowed to the frame,
> which is what the three
> failing tests describe.
> _Drafted with AI assistance (Claude Opus 5); filed by Jens Geyer._
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
