Skip to content

[Nexthop][bugfix] Capture SwSwitch by value in neighbor-cache state-update lambdas - #1903

Open
deepakgudiseva-nexthop wants to merge 1 commit into
meta:mainfrom
nexthop-ai:capture-swswitch-by-value-in-neighbor
Open

deepakgudiseva-nexthop wants to merge 1 commit into
meta:mainfrom
nexthop-ai:capture-swswitch-by-value-in-neighbor

Conversation

@deepakgudiseva-nexthop

Copy link
Copy Markdown

Pre-submission checklist

  • I've ran the linters locally and fixed lint errors related to the files I modified in this PR. You can install the linters by running pip install -r requirements-dev.txt && pre-commit install
  • pre-commit run

Summary

Use-after-free in SwSwitch::stop(): the neighbor-cache state-update lambdas capture this (NeighborCacheImpl), but stop() destroys the caches (nUpdater_.reset()) before stopThreads() drains pendingUpdates_. A queued neighbor update then reads sw_ through freed memory and faults in SwitchIdScopeResolver::scope(), so the agent dies during a graceful exit and loses its warm-boot state. Reordering stop() is not an option: ~NeighborUpdater waits on the neighbor-cache evb that stopThreads() has already joined.

Fix: the lambdas capture sw, intfID and needL2EntryForNeighbor by value, as getUpdateFnToProgramPendingEntryForVlan already does. flushEntryFromSwitchState and RemoteNeighborUpdater::processChangedInterface, which had the same this capture, become static.

Test Plan

New SwSwitchTestNbrs.neighborUpdatesOutliveTheirCache queues updates from a standalone ArpCache, destroys the cache, then lets them run and checks they landed. The UAF itself is only reported under ASAN.

Hardware, OSS build, warm restarts of fboss_sw_agent with many resolved neighbors and MALLOC_CONF=junk:free so the dangling read always faults:

  • before: SIGSEGV in NeighborCacheImpl<T>::getUpdateFnToProgramEntry lambda <- SwSwitch::handlePendingUpdates <- stopThreads on 7/7 stops
  • after: 8/8 stops clean, no core

Signed-off-by: deepakgudiseva-nexthop <deepakgudiseva@nexthop.ai>
@deepakgudiseva-nexthop
deepakgudiseva-nexthop requested a review from a team as a code owner November 3, 2026 20:08
@meta-cla meta-cla Bot added the CLA Signed label Nov 3, 2026
@github-actions github-actions Bot added nexthop project:bugfix FBOSS project: bugfix labels Nov 3, 2026
meta-codesync Bot pushed a commit that referenced this pull request Nov 4, 2026
Summary:
`NeighborCacheImpl::getUpdateFnToProgramEntryForVlan()` and `getUpdateFnToProgramPendingEntryForVlan()` have had no callers since 8b49d2f ("Remove FLAGS_intf_nbr_tables from NeighborCacheImpl-defs.h"), which removed the VLAN-table branch of `programEntry()` / `programPendingEntry()`. Every neighbor update now goes through `getUpdateFnToProgramEntry()` / `getUpdateFnToProgramPendingEntry()`. This removes both dead functions.

It also removes the includes that clang-tidy's `misc-include-cleaner` reports as unused in `NeighborCacheImpl.h`, `NeighborCacheImpl-defs.h` and `SwSwitchTest.cpp`. `NeighborCacheImpl-defs.h` throws `FbossError` itself, so it now includes `FbossError.h` directly instead of getting it through `NeighborCacheImpl.h`.

We checked with Meta that neither function has callers in the internal code base either.

This touches the same function as #1903, which changes its lambda captures. Whichever of the two lands second needs a trivial rebase (keep the deletion).

Pull Request resolved: #1905

Test Plan:
The equivalent change in our internal tree passed our CI's Bazel build and its cmake build + unit test run (clang). No reference to either removed function remains in the tree.

## Review Findings

No issues found above confidence threshold (fboss-review, 7 applicable reviewers).

Reviewed By: AarjunC

Differential Revision: D129483370

Pulled By: KevinYakar

fbshipit-source-id: 70f2113080dafca3497fda25f23cd9322a9cad57
@deepakgudiseva-nexthop

Copy link
Copy Markdown
Author

/autorebase

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant