From e80cd91c1972843274ab183f11abb050677d4bf6 Mon Sep 17 00:00:00 2001 From: Darrin D'Mello Date: Fri, 23 Oct 2026 09:11:35 -0700 Subject: [PATCH] Call out freeing an acceptor before its forceStop() runs Summary: `forceStop()` only queues the drop of an acceptor's connections on its EventBase, even when called on that thread, so freeing the acceptor right after it is a use-after-free. [`ServiceWorker::drainServiceAcceptor`](https://www.internalfb.com/code/fbsource/[master]/fbcode/proxygen/lib/services/ServiceWorker.h) does that when it evicts a previously-draining acceptor, though no caller reaches that branch today: the hot-reload rebind from D116582982 releases every draining acceptor before draining its address again. Flag the branch with a FIXME, spell out the free-a-loop-later duty `releaseDrainingAcceptor()` hands its caller, and note why `clearAcceptors()` does not force-stop and what the uncalled `clearDrainingAcceptors()` is for. Also correct the [RevProxy worker reference](https://www.internalfb.com/code/fbsource/[master]/fbcode/proxygen/facebook/revproxy/.llms/skills/revproxy/references/core/HTTPRevProxyWorker.md), which still says the VIP-swap helpers have no callers, and the `prepareVipSocketSwap` comment, which still relies on that eviction. There is no behavior change. Reviewed By: DavidLu1997 Differential Revision: D120071479 fbshipit-source-id: fc208f77ecaf7385da2baee3c6628251c99cf9ac --- proxygen/lib/services/ServiceWorker.h | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/proxygen/lib/services/ServiceWorker.h b/proxygen/lib/services/ServiceWorker.h index 86c3cd4f7..c7432b456 100644 --- a/proxygen/lib/services/ServiceWorker.h +++ b/proxygen/lib/services/ServiceWorker.h @@ -83,6 +83,13 @@ class ServiceWorker { << " (drainingAcceptors_.size=" << drainingAcceptors_.size() << ")"; // Evict any previous draining acceptor still occupying this address. + // + // FIXME: use-after-free if this branch is ever taken. forceStop() only + // queues the drop of the acceptor's connections on its EventBase, even + // when called on that thread, and erase() frees the acceptor before the + // drop runs. Current callers release every draining acceptor before + // draining its address again, so this is not reached. Free the evicted + // acceptor on a later loop iteration before relying on it. auto drainingIt = drainingAcceptors_.find(accAddress); if (drainingIt != drainingAcceptors_.end()) { LOG(INFO) @@ -98,7 +105,11 @@ class ServiceWorker { namedAddress_.erase(name); } - // Release a draining acceptor, the caller now owns its lifetime. + // Release a draining acceptor, the caller now owns its lifetime. If it may + // still hold connections, forceStop() it and free it on a later loop + // iteration of its EventBase: forceStop() only queues the drop, so freeing + // the acceptor right after it leaves the queued drop running on freed + // memory. [[nodiscard]] wangle::Acceptor* releaseDrainingAcceptor( const AcceptorAddress& accAddress) { auto it = drainingAcceptors_.find(accAddress); @@ -140,13 +151,22 @@ class ServiceWorker { virtual void flushStats() { } - // Destruct all the acceptors + // Destruct all the acceptors. + // + // Deliberately does not forceStop() first: this runs from worker destruction, + // after the worker's EventBase is destroyed, and forceStop() posts to it. + // Anything an acceptor still owns must have been reaped before then. virtual void clearAcceptors() { acceptors_.clear(); namedAddress_.clear(); drainingAcceptors_.clear(); } + // Frees every draining acceptor at once, without forceStop(). It has no + // callers; it is for discarding drains that are over: call it on the worker's + // EventBase once no draining acceptor holds connections or has work queued + // there. To retire a live one, take it with releaseDrainingAcceptor() + // instead. virtual void clearDrainingAcceptors() { drainingAcceptors_.clear(); }