Skip to content

Commit 9df8399

Browse files
yuxuanchen1997meta-codesync[bot]
authored andcommitted
Match async client channel declaration to definition
Summary: Every thrift-python async client crashes with a heap-use-after-free under libc++ on its first RPC. This is a latent bug for any service that does a Thrift RPC over ServiceRouter / get_client faults inside the native client dispatch. Root cause: `thrift/lib/python/client/async_client.pxd` declared `cdef bind_client(self, cRequestChannel_ptr&& channel)`, but `async_client.pyx` defined it by value as `cdef bind_client(self, cRequestChannel_ptr channel)`. Cross-module callers such as `requestchannel_callback` in `async_client_factory.pyx` compile against the `.pxd`, so they emit the vtable slot as Ptr&& and pass a reference to the channel `unique_ptr`, which lives inside the resolved `folly::Future` Core. The by-value function body then reads that reference as if it were the `unique_ptr` object, so `OmniClient::channel_` ends up pointing at the Core storage instead of the channel. The real channel stays owned by the Core and is freed by `~Core`, so the first RPC reads freed memory. Fix: drop the `&&` from the `.pxd` declaration so it matches the by-value `.pyx` definition, which is also consistent with the neighboring `bind_client_shared` signature style. The channel is still moved end-to-end: callers pass `cmove(channel._cpp_obj)`, `bind_client` receives the move-only `cRequestChannel_ptr` by value, and then moves it into `OmniClient`. Also adds a named regression test exercising async `get_client` plus first RPC, which heap-use-after-frees under libc++ without this fix. The BUCK change is autodeps making the pre-existing `thrift.python.common` import explicit on the client test targets. Reviewed By: iahs Differential Revision: D109862203 fbshipit-source-id: 83ef740c3c41f8b9fd04e6376c265147b3d56d6f
1 parent 517f1e2 commit 9df8399

2 files changed

Lines changed: 26 additions & 1 deletion

File tree

‎thrift/lib/python/client/async_client.pxd‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,6 @@ cdef class AsyncClient:
2929
cdef unique_ptr[cOmniClient] _omni_client
3030
cdef unordered_map[string, string] _persistent_headers
3131
cdef vector[shared_ptr[cTProcessorEventHandler]] _deferred_event_handlers
32-
cdef bind_client(self, cRequestChannel_ptr&& channel)
32+
cdef bind_client(self, cRequestChannel_ptr channel)
3333
cdef bind_client_shared(self, shared_ptr[cRequestChannel] channel)
3434
cdef add_event_handler(self, const shared_ptr[cTProcessorEventHandler]& handler)

‎thrift/lib/python/client/test/async_client_test.py‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -439,3 +439,28 @@ async def test_protocol_id_retrieval_with_compact_protocol(self) -> None:
439439
# an exception before the request is sent
440440
result = await client.add(1, 2)
441441
self.assertEqual(3, result)
442+
443+
async def test_async_channel_bind_first_rpc_no_use_after_free(self) -> None:
444+
"""Regression: async get_client must bind the channel by move, not alias
445+
the folly Future Core's storage.
446+
447+
async_client.pxd must declare `bind_client` by value, matching the
448+
async_client.pyx definition. When the declaration used `&&`, the
449+
cross-module requestchannel_callback (async_client_factory.pyx) passed a
450+
reference to the channel unique_ptr -- which lives inside the folly
451+
Future Core -- through an rvalue-ref vtable slot into a by-value body.
452+
OmniClient::channel_ ended up pointing at the Core's storage instead of
453+
the channel. The real channel was left in the Core and freed by ~Core, so
454+
the first RPC's getChannelProtocolId() read freed memory: a
455+
heap-use-after-free under libc++. Exercising the async connect + first
456+
RPC over the asyncio bridge catches any regression of the signature
457+
mismatch.
458+
"""
459+
# pyre-fixme[16]: `AsyncContextManager` has no attribute `__aenter__`.
460+
async with server_in_event_loop() as addr:
461+
async with get_client(TestService, host=addr.ip, port=addr.port) as client:
462+
# First RPC reads the freshly bound channel; a channel_ that
463+
# dangled into a freed Core would fault here under libc++.
464+
self.assertEqual(3, await client.add(1, 2))
465+
# A second RPC confirms the channel stayed valid after binding.
466+
self.assertEqual(5, await client.add(2, 3))

0 commit comments

Comments
 (0)