[
https://issues.apache.org/jira/browse/THRIFT-6182?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jens Geyer updated THRIFT-6182:
-------------------------------
Description:
Wrapping a transport in TFramedTransport or TFastFramedTransport overwrites the
maximum frame size the transport was configured with, even when no maximum was
asked for.
lib/java/src/main/java/org/apache/thrift/transport/layered/TFramedTransport.java:
{code}
public TFramedTransport(TTransport transport, int maxLength) throws
TTransportException {
super(transport);
TConfiguration _configuration =
Objects.isNull(transport.getConfiguration())
? new TConfiguration()
: transport.getConfiguration();
_configuration.setMaxFrameSize(maxLength);
...
}
public TFramedTransport(TTransport transport) throws TTransportException {
this(transport, TConfiguration.DEFAULT_MAX_FRAME_SIZE);
}
{code}
So
{code}
socket.getConfiguration().setMaxFrameSize(1024);
TTransport framed = new TFramedTransport(socket);
{code}
leaves the socket configured for 16384000. Two things go wrong at once. The
maximum an operator set is silently replaced by the library default, and the
object it is written into belongs to the transport underneath, so anything else
sharing that TConfiguration is changed too --
TLayeredTransport.getConfiguration() delegates to the inner transport by
design, so a layered transport has no configuration of its own to write into.
The same holds for TFastFramedTransport(underlying) and
TFastFramedTransport(underlying, initialBufferCapacity), and for
TFramedTransport.Factory() and TFastFramedTransport.Factory() /
Factory(initialCapacity), which all pass TConfiguration.DEFAULT_MAX_FRAME_SIZE
to the constructor that applies it. A server configured with new
TFramedTransport.Factory() therefore raises the limit on every connection it
accepts.
The fix is that a constructor or factory which was not given a maximum passes
back the one the transport already carries, so wrapping changes nothing. Where
a maximum is given explicitly the behaviour is unchanged: it is applied, as the
argument's javadoc promises.
Go solved this for its own deprecated constructors and the reason is written
into TConfiguration: the noPropagation flag exists "to avoid overriding
underlying TTransport/TProtocol's cfg by accidental propagations"
(lib/go/thrift/configuration.go). C++, netstd, Delphi and Haxe all read the
configured maximum and never write it. Java is the only binding where wrapping
writes it.
Tests are added to TestTFramedTransport and inherited by
TestTFastFramedTransport, so both classes are covered: wrapping keeps a lowered
maximum, the factory keeps it, an explicit maximum is still applied, and -- the
point of the other three -- a frame declaring 2048 bytes is still refused by a
transport configured for 1024 after being wrapped. Three of the four fail on
each class before the change; the explicit-maximum one is a regression guard
and passes either way.
was:
Wrapping a transport in TFramedTransport or TFastFramedTransport overwrites the
maximum frame size the transport was configured with, even when no maximum was
asked for.
lib/java/src/main/java/org/apache/thrift/transport/layered/TFramedTransport.java:
public TFramedTransport(TTransport transport, int maxLength) throws
TTransportException {
super(transport);
TConfiguration _configuration =
Objects.isNull(transport.getConfiguration())
? new TConfiguration()
: transport.getConfiguration();
_configuration.setMaxFrameSize(maxLength);
...
}
public TFramedTransport(TTransport transport) throws TTransportException {
this(transport, TConfiguration.DEFAULT_MAX_FRAME_SIZE);
}
So
socket.getConfiguration().setMaxFrameSize(1024);
TTransport framed = new TFramedTransport(socket);
leaves the socket configured for 16384000. Two things go wrong at once. The
maximum an operator set is silently replaced by the library default, and the
object it is written into belongs to the transport underneath, so anything else
sharing that TConfiguration is changed too --
TLayeredTransport.getConfiguration() delegates to the inner transport by
design, so a layered transport has no configuration of its own to write into.
The same holds for TFastFramedTransport(underlying) and
TFastFramedTransport(underlying, initialBufferCapacity), and for
TFramedTransport.Factory() and TFastFramedTransport.Factory() /
Factory(initialCapacity), which all pass TConfiguration.DEFAULT_MAX_FRAME_SIZE
to the constructor that applies it. A server configured with new
TFramedTransport.Factory() therefore raises the limit on every connection it
accepts.
The fix is that a constructor or factory which was not given a maximum passes
back the one the transport already carries, so wrapping changes nothing. Where
a maximum is given explicitly the behaviour is unchanged: it is applied, as the
argument's javadoc promises.
Go solved this for its own deprecated constructors and the reason is written
into TConfiguration: the noPropagation flag exists "to avoid overriding
underlying TTransport/TProtocol's cfg by accidental propagations"
(lib/go/thrift/configuration.go). C++, netstd, Delphi and Haxe all read the
configured maximum and never write it. Java is the only binding where wrapping
writes it.
Tests are added to TestTFramedTransport and inherited by
TestTFastFramedTransport, so both classes are covered: wrapping keeps a lowered
maximum, the factory keeps it, an explicit maximum is still applied, and -- the
point of the other three -- a frame declaring 2048 bytes is still refused by a
transport configured for 1024 after being wrapped. Three of the four fail on
each class before the change; the explicit-maximum one is a regression guard
and passes either way.
> Wrapping a transport must not raise the configured maximum frame size (Java)
> ----------------------------------------------------------------------------
>
> Key: THRIFT-6182
> URL: https://issues.apache.org/jira/browse/THRIFT-6182
> Project: Thrift
> Issue Type: Bug
> Components: Java - Library
> Reporter: Jens Geyer
> Priority: Major
> Fix For: 0.25.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Wrapping a transport in TFramedTransport or TFastFramedTransport overwrites
> the maximum frame size the transport was configured with, even when no
> maximum was asked for.
> lib/java/src/main/java/org/apache/thrift/transport/layered/TFramedTransport.java:
> {code}
> public TFramedTransport(TTransport transport, int maxLength) throws
> TTransportException {
> super(transport);
> TConfiguration _configuration =
> Objects.isNull(transport.getConfiguration())
> ? new TConfiguration()
> : transport.getConfiguration();
> _configuration.setMaxFrameSize(maxLength);
> ...
> }
> public TFramedTransport(TTransport transport) throws TTransportException {
> this(transport, TConfiguration.DEFAULT_MAX_FRAME_SIZE);
> }
> {code}
> So
> {code}
> socket.getConfiguration().setMaxFrameSize(1024);
> TTransport framed = new TFramedTransport(socket);
> {code}
> leaves the socket configured for 16384000. Two things go wrong at once. The
> maximum an operator set is silently replaced by the library default, and the
> object it is written into belongs to the transport underneath, so anything
> else sharing that TConfiguration is changed too --
> TLayeredTransport.getConfiguration() delegates to the inner transport by
> design, so a layered transport has no configuration of its own to write into.
> The same holds for TFastFramedTransport(underlying) and
> TFastFramedTransport(underlying, initialBufferCapacity), and for
> TFramedTransport.Factory() and TFastFramedTransport.Factory() /
> Factory(initialCapacity), which all pass
> TConfiguration.DEFAULT_MAX_FRAME_SIZE to the constructor that applies it. A
> server configured with new TFramedTransport.Factory() therefore raises the
> limit on every connection it accepts.
> The fix is that a constructor or factory which was not given a maximum passes
> back the one the transport already carries, so wrapping changes nothing.
> Where a maximum is given explicitly the behaviour is unchanged: it is
> applied, as the argument's javadoc promises.
> Go solved this for its own deprecated constructors and the reason is written
> into TConfiguration: the noPropagation flag exists "to avoid overriding
> underlying TTransport/TProtocol's cfg by accidental propagations"
> (lib/go/thrift/configuration.go). C++, netstd, Delphi and Haxe all read the
> configured maximum and never write it. Java is the only binding where
> wrapping writes it.
> Tests are added to TestTFramedTransport and inherited by
> TestTFastFramedTransport, so both classes are covered: wrapping keeps a
> lowered maximum, the factory keeps it, an explicit maximum is still applied,
> and -- the point of the other three -- a frame declaring 2048 bytes is still
> refused by a transport configured for 1024 after being wrapped. Three of the
> four fail on each class before the change; the explicit-maximum one is a
> regression guard and passes either way.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
