file_server: fix use-after-free in FileStreamer async-file callbacks after request cancel

[omkhar]

Commit Message:
file_server: fix UAF on async-file callback after request cancel

FileStreamer::start() captured [this] into the async-file callbacks
returned by AsyncFileManager::stat() and AsyncFileManager::read().
The async-file manager guarantees the callback fires (even if cancel
was called), so when the FileStreamer is destroyed before the
callback runs the bare this capture becomes dangling and the
callback dereferences freed memory.

This is reachable on any deployment using the file_server filter when
downstream clients can cancel or reset requests while an async-file
operation is in flight.

Specifically, the race is if the callback is placed in a dispatcher queue
by AsyncFileManager before cancelation, then deletion before
the callback reaches the head of the queue.

Solution is to hold a CancelCallback sentinel in FileStreamer and
wrap the async-file callbacks in cancelWrapper.

Additional Description:

Reported privately via GHSA-c4w8-wqj5-4xph; envoy security team
(@wbpcode) asked for this to be fixed in the open given the
file_server filter's WIP status.

Fixes #45152.

Risk Level: Low.
Defensive lifetime guard around two existing async-file callback sites.
No behavior change for the in-flight-completes-before-cancel path. The
cancel-then-callback path previously dereferenced freed memory and now
no-ops.

Testing: bazel test --config=clang --config=clang-asan //test/extensions/filters/http/file_server:file_server_test passes
locally on the patched tree at upstream main. Happy to add a focused
regression test that drives the cancel-during-stat path if maintainers
would like.

Docs Changes: None required.

Release Notes: Added to changelogs/current.yaml under bug_fixes
(area: file_server).

Platform Specific Features: None.

[ravenblackx]

Could you add an integration test case that reproduces the crash, before the fix?

(cancel_callback_ should already be called in the abort/destruction path, which should already be making AsyncFileManager not call the callback, so there's a broader problem if this change addresses a real problem. We shouldn't have two sentinels doing the same job.)

Edit: Ah, I see the issue, the callback can be dispatched before it is canceled, here, but then canceled before the dispatched event reaches head of queue, so it also needs the ability to abort if it's canceled while the callback is in the dispatcher's queue.

And it makes sense here to have two sentinels, because one is to abort the file operation if it isn't complete, and the other is to abort the callback if it got queued first.

So since it's racy it's probably quite difficult to repro reliably in an integration test.

Consider using cancelWrapped rather than another custom sentinel, since it was created for exactly this kind of purpose. (Annoying though it is to have to call two cancel-callback functions!)

[phlax]

needs to use new changelog layout - see #45095

/wait

[omkhar]

Thanks @ravenblackx — that read of the dispatcher-queue race is right, and using cancelWrapped is cleaner than rolling my own.

Pushed eccd1db:

• Drops std::shared_ptr<bool> alive_ and the [this, alive=...] capture pattern.
• Adds an Envoy::CancelWrapper::CancelFunction cancel_dispatcher_callbacks_ member; each async-file completion lambda is now wrapped via Envoy::CancelWrapper::cancelWrapped(lambda, &cancel_dispatcher_callbacks_).
• abort() and ~FileStreamer() invoke cancel_dispatcher_callbacks_() after cancel_callback_(). The existing cancel_callback_ continues to abort the in-flight file op; cancel_dispatcher_callbacks_ short-circuits any completion the dispatcher has already enqueued (the path you cited at async_file_manager_thread_pool.cc:112-120).
• Adds //source/common/common:cancel_wrapper_lib to the file_server BUILD deps.

Verified on Linux x86_64: bazel test --config=clang //test/extensions/filters/http/file_server:file_server_test passes on a clean envoyproxy/envoy@main with this patch applied.

On the integration test: I agree the race is "quite difficult to repro reliably" — wiring the dispatch-then-cancel sequence deterministically from an integration test would require driving AsyncFileManager state from the test harness. Happy to draft one if you want it before merge, but per your earlier note I think the cancel_wrapper change makes the safety property local enough that a unit test would be sufficient if anything.

[ravenblackx]

Looks good to me, and I'm okay with not trying to force a regression test around it.

Still needs the changelog updated to the new layout per phlax's comment.

[omkhar]

Addressed @phlax / @ravenblackx — migrated the bug_fix entry to changelogs/current/bug_fixes/file_server__fixed-use-after-free-in-filestreamer-callbacks.rst per #45095 / #45093, and merged upstream/main to clear the conflict. No code changes since eccd1db; the cancelWrapped refactor stands. Ready for re-review when CI clears.

[ravenblackx]

/wait

Format pre-check is angry because it wants more hideous indents. You could either run clang-format over the touched files (fastest) or run ./ci/do_ci.sh format fix in the branch, or copy-paste and apply the diff from the CI report.

[ravenblackx]

Thanks. I've modified the description to match the cancelWrapped implementation rather than alive_, and am queuing it to merge assuming CI will pass this time.