lib/proxygen
1
0
Fork 0
mirror of https://github.com/facebook/proxygen.git synced 2026-10-24 01:33:54 +03:00
Code Activity

Call out freeing an acceptor before its forceStop() runs

Browse Source 
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
This commit is contained in:
Darrin D'Mello
2026-10-23 09:11:35 -07:00
committed by meta-codesync[bot]
parent ff985ae54a
commit e80cd91c19
1 changed files with 22 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
proxygen/lib/services/ServiceWorker.h
View File

@@ -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();
}