[
https://issues.apache.org/jira/browse/THRIFT-6271?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jens Geyer resolved THRIFT-6271.
--------------------------------
Fix Version/s: 0.25.0
Assignee: Jens Geyer
Resolution: Fixed
> C++: readAll on a concrete TWebSocketServer bypasses WebSocket framing
> ----------------------------------------------------------------------
>
> Key: THRIFT-6271
> URL: https://issues.apache.org/jira/browse/THRIFT-6271
> Project: Thrift
> Issue Type: Bug
> Components: C++ - Library
> Reporter: Jens Geyer
> Assignee: Jens Geyer
> Priority: Minor
> Fix For: 0.25.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> The inheritance chain is {{TWebSocketServer}} -> {{THttpServer}} ->
> {{THttpTransport}} -> {{TVirtualTransport<THttpTransport>}}.
> {{TWebSocketServer}} ({{lib/cpp/src/thrift/transport/TWebSocketServer.h:54}})
> overrides {{readAll_virt()}} ({{:63}}), which is what does the handshake and
> the frame decoding. It does not define a non-virtual {{readAll()}}, so the
> one it inherits is {{TVirtualTransport<THttpTransport>::readAll()}} -- and
> that one is templated on {{THttpTransport}}, so it reads through
> {{THttpTransport::read}} and never sees a WebSocket frame.
> The result is that the same object behaves differently depending on the
> static type of the pointer:
> * through a {{TTransport}} / {{shared_ptr<TTransport>}} -- {{readAll_virt()}}
> dispatches, WebSocket semantics
> * through a {{TWebSocketServer*}} -- the inherited non-virtual {{readAll()}}
> is found, HTTP semantics
> In the shipped code path this does not bite, because the transport factory
> hands out {{shared_ptr<TTransport>}}. {{TVirtualTransport.h}} already names
> the shape in a comment on that very method:
> {quote}subclasses that use TVirtualTransport to derive from another transport
> implementation (i.e., not TTransportDefaults) should beware that this may
> override any non-default readAll() implementation provided by the parent
> transport class. They may need to redefine readAll() to call the correct
> parent implementation, if desired.{quote}
> which is exactly this case. It is recorded because it is easy to hit when
> writing a test against the concrete type.
> h2. Suggested
> Give {{TWebSocketServer}} a non-virtual {{readAll()}} that forwards to its
> own {{readAll_virt()}}, as the comment above suggests, so the concrete type
> behaves like the abstract one.
> _Drafted with AI assistance (Claude Opus 5); reviewed and filed by Jens
> Geyer._
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
