[zmq] Fix use-after-free in deferred burst notify; bound notification deferral - #1234
Open
venkit-nexthop wants to merge 3 commits into
Open
[zmq] Fix use-after-free in deferred burst notify; bound notification deferral#1234venkit-nexthop wants to merge 3 commits into
venkit-nexthop wants to merge 3 commits into
Conversation
ZmqRouteServer::mqPollThread accumulates raw ZmqMessageHandler pointers across poll iterations and notifies them once the burst quiesces. A consumer destroyed between being marked dirty and that deferred flush has already unregistered itself, but its pointer stayed in the set and the flush called notifyPending() through it. Handlers are long-lived in practice, so the window is small, but it is real. Add ZmqHandlerRegistry::flushDirtyHandlers() and route the flush through it: notification happens under the same mutex removeHandler() takes, and the registry is iterated so membership doubles as the liveness check. A handler removed since it went dirty has already been erased from the registry and is skipped rather than notified through a dangling pointer; a concurrent removeHandler() blocks until the flush returns. Add a registry-level regression test that unregisters one of two dirty handlers before the flush and asserts the survivor is notified exactly once and the removed one not at all. Reported in post-merge review of sonic-net#1187. Signed-off-by: Venkit Kasiviswanathan <venkit@nexthop.ai>
The quiesce notify fires only when the stream pauses for BURST_QUIESCE_MS, and the consumer-side threshold only when enough distinct keys stage up. A stream that never pauses and stays under the threshold deferred notification indefinitely. Coalescing keeps the staged state current, so the exposure is delayed convergence rather than wrong state -- but a sustained same-key flap is exactly when prompt programming of the latest state matters. Track the time each handler first went dirty (dirtyHandlers becomes a map keyed by handler) and flush overdue handlers once per drain pass, bounding notification latency at BURST_MAX_HOLDOFF_MS (50 ms) even when the burst never quiesces. flushDirtyHandlers() now takes a cutoff: steady_clock::now() flushes everything (the quiesce path), now() minus the holdoff flushes only overdue entries (the mid-burst path). The flush walks the registry to notify (membership is the liveness check, as before) and then erases every overdue entry live or dead, so a dangling pointer cannot pin the poll loop in its short-timeout mode. emplace() keeps the first dirty time on repeat touches; measuring the holdoff from the last touch would let a busy stream reset its own clock indefinitely. Tests: FlushHonorsCutoff pins the subset semantics at registry level; ContinuousStreamWakesWithinHoldoff drives a 2 ms same-key stream and asserts the Select loop wakes well inside a 500 ms window that unbounded deferral would have exhausted. Raised in review of sonic-swss#4564. Signed-off-by: Venkit Kasiviswanathan <venkit@nexthop.ai>
Collaborator
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
Author
|
@qiluo-msft I have fixed your comment from #1187 in this PR |
deepak-singhal0408
left a comment
deepak-singhal0408
left a comment
Contributor
There was a problem hiding this comment.
One thing the registry fix doesn't reach, on a file outside this diff:
ZmqConsumerStateTable::~ZmqConsumerStateTable() calls removeHandler(), which is correct for its own members — but ZmqRouteConsumerStateTable declares no destructor, so its m_ingressCallback is destroyed before that base body runs, while the object still has the derived vptr and is still registered. An in-flight dispatch() in that window calls the overridden handleReceivedData() and reads a destroyed std::function. The dtor comment states the invariant ("detach from the registry before any of our members are destroyed") that the subclass then breaks.
Not reachable in orchagent, which exits via _exit(0), but reachable from tests that destroy a consumer while the poll thread runs. Detaching in the most-derived destructor — or a detach() helper each most-derived class calls — would close it and make the requirement explicit for future subclasses.
…, pin holdoff test to wake latency - Remove aminclude_static.am and common/cfg_schema.h (local build artifacts) and gitignore them. - Read steady_clock once per drain pass instead of per message; emplace discards the value on repeat touches anyway. Pass-start granularity only makes flushes earlier, never later. - Reword the post-drain flush comment to state the actual guarantee: once per drain-to-empty pass, not an unconditional 50ms bound. - Make ContinuousStreamWakesWithinHoldoff attribute the wake: pace at 300us so a quiesce gap needs a >16x scheduler stall, and assert the measured wake latency lands in the 40-150ms holdoff window. Verified the test fails with BURST_MAX_HOLDOFF_MS reverted. Signed-off-by: Venkit Kasiviswanathan <venkit@nexthop.ai>
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why I did it
Two follow-ups to #1187's burst-coalescing ZmqRouteServer:
Use-after-free in the deferred notify (raised post-merge in Add ZmqRouteServer and ZmqRouteConsumerStateTable for burst-coalesced route ingress #1187, here):
mqPollThreadcollects raw handler pointers across poll iterations and notifies them after the burst quiesces. A consumer destroyed in that window had already unregistered, but the flush still callednotifyPending()through the stale pointer.Unbounded notification deferral (raised in review of [orchagent]: Add ZmqRouteServer for concurrent route updates sonic-swss#4564, here, item 1): the quiesce notify needs a BURST_QUIESCE_MS pause and the consumer-side threshold needs enough distinct keys. A stream that never pauses and stays under the threshold deferred notification indefinitely. Coalescing keeps the staged state current, so the exposure is delayed convergence -- but a sustained same-key flap is exactly when prompt programming of the latest state matters.
How I did it
Commit 1: add
ZmqHandlerRegistry::flushDirtyHandlers()and flush through it. Notification happens under the same mutexremoveHandler()takes, iterating the registry so membership doubles as the liveness check; removed handlers are skipped instead of dereferenced.Commit 2:
dirtyHandlersbecomes a map of handler to first-dirty time, and the flush takes a cutoff. The quiesce path passes now() (flush everything); a new per-drain-pass call passes now() minusBURST_MAX_HOLDOFF_MS(50 ms), bounding notification latency even when a burst never quiesces. Overdue entries are erased live or dead, so a dangling pointer cannot pin the poll loop in its short-timeout mode.How to verify it
New tests:
ZmqHandlerRegistry.FlushSkipsUnregisteredHandlers(unregister between dirty and flush -- the survivor is notified once, the removed handler never),ZmqHandlerRegistry.FlushHonorsCutoff(only overdue entries flushed),ZmqRouteConsumerStateTable.ContinuousStreamWakesWithinHoldoff(2 ms same-key stream with no quiesce gap wakes the Select loop well inside a 500 ms window).Ran the zmq route + registry suites locally (sonic-slave-bookworm, redis): 10/10, and the existing burst/wakeup tests are unchanged and green.