consomme: fix deadlock and UAF in Windows DNS resolver

[Steven Malis (smalis-msft)]

We had a DNS vmm test flake by timing out. The lock on pending_requests is being held inside cancel_all while a
completion called the callback, which tries to take the same mutex, resulting in a deadlock. Fix this by not holding the lock over the calls to DnsCancelQueryRaw, but also narrow the lock windows everywhere else too. Then wrap the handle in an Arc to ensure that it stays alive across all possible asynchronous flows, fixing a potential UAF.

[github-actions]

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

[Copilot]

Pull request overview

This PR updates the Windows DnsQueryRaw backend in net_consomme to reduce mutex hold times around pending_requests, specifically avoiding holding the lock while calling DnsCancelQueryRaw during shutdown to prevent potential callback/lock re-entrancy deadlocks.

Changes:

• Narrow pending_requests lock scope when pre-inserting the cancel-handle placeholder and reading pending_count.
• Avoid holding the pending_requests mutex across DnsCancelQueryRaw calls by mem::take-ing the slab in cancel_all.
• Minor lock-scope tightening in the callback removal path.

[github-actions]

2 Petri tests failed (1 unstable)

[John Starks (jstarks)]

This fix does fix a real deadlock, but it exacerbates a use after free of the cancel context. We could fix this with ref counts, but I want to explore fixing this with a task-oriented approach. Let me see if I can coax a better fix out of copilot.

[John Starks (jstarks)]

No, that's not practical due to other structural limitations (no real way for an endpoint to spawn its own tasks right now), plus it may interfere with some multiqueue refactoring going on.

Can you add a ref count and have cancel_io take refs on everything under the lock and drop, rather than drain? And then the callback should assert that it removed its own thing from the slab, rather than just try_remove. That should fix the use after free and deadlock, I believe.

[Steven Malis (smalis-msft)]

I'm not 100% sure I understood, is this what you meant?

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

vm/devices/net/net_consomme/consomme/src/dns_resolver/windows/mod.rs:268

• Slab::remove panics if the key is vacant. Since this code explicitly allows the entry to already be gone (e.g. callback raced before handle update / cancel_all clears), this should use try_remove like before to avoid a potential panic in the Windows callback thread.

    let _ = context.pending_requests.lock().remove(context.slab_key);

vm/devices/net/net_consomme/consomme/src/dns_resolver/windows/mod.rs:199

• cancel_all no longer clears the slab; it clones handles but leaves all entries tracked until callbacks run. This keeps memory/live entries longer than necessary and makes repeated cancellation attempts possible if cancel_all is ever called more than once. You can keep the narrower lock window while preserving the old “drain then cancel” semantics by draining into a Vec under the lock and cancelling after releasing it.

This issue also appears on line 268 of the same file.

    fn cancel_all(&mut self) {
        let pending: Vec<_> = self
            .pending_requests
            .lock()
            .iter()
            .map(|(_, cancel_handle)| cancel_handle.clone())
            .collect();

[github-actions]

4 Petri tests failed (2 unstable)

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

vm/devices/net/net_consomme/consomme/src/dns_resolver/windows/mod.rs:263

• Slab::remove will panic if the entry was already removed (e.g., cancel_all() now swaps/drains the slab, and callbacks can still fire afterward). The callback should tolerate missing entries and avoid panicking in this race.

    let context = unsafe { Box::from_raw(query_context.cast::<RawCallbackContext>().cast_mut()) };

    let _ = context.pending_requests.lock().remove(context.slab_key);

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

vm/devices/net/net_consomme/consomme/src/dns_resolver/windows/mod.rs:183

• Slab::remove panics if the key is already vacant. Using try_remove here makes the error path resilient to unexpected callback timing (or future changes that may remove the entry earlier) and avoids a potential panic.

            self.pending_requests.lock().remove(slab_key);

vm/devices/net/net_consomme/consomme/src/dns_resolver/windows/mod.rs:282

• Slab::remove will panic if the entry is already missing. The previous code used try_remove, which keeps the callback panic-free if the slab entry was removed elsewhere (e.g., due to an unexpected Windows API behavior or a future refactor).

    let _cancel_handle = context.pending_requests.lock().remove(context.slab_key);

[github-actions]

2 Petri tests failed (1 unstable)

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

vm/devices/net/net_consomme/consomme/src/dns_resolver/windows/mod.rs:282

• Slab::remove will panic if the entry was already removed (e.g., by the error path in query() or an unexpected callback ordering). Using try_remove keeps the cancel-handle-lifetime intent without risking a crash.

    let _cancel_handle = context.pending_requests.lock().remove(context.slab_key);

[github-actions]

3 Petri tests failed (2 unstable)

[Daman Mulye (damanm24)]

LGTM - thanks for fixing this!