Skip to content

Fix buffer map/destroy race#9610

Draft
danyalahmed1995 wants to merge 1 commit into
gfx-rs:trunkfrom
danyalahmed1995:fix-buffer-map-destroy-race
Draft

Fix buffer map/destroy race#9610
danyalahmed1995 wants to merge 1 commit into
gfx-rs:trunkfrom
danyalahmed1995:fix-buffer-map-destroy-race

Conversation

@danyalahmed1995
Copy link
Copy Markdown

Fixes #9552.

Buffer::map temporarily exposed BufferMapState::Idle while completing a pending map. During that window, buffer_destroy could call unmap, observe Idle, and continue destroying the buffer while the map path was still completing.

keeps map_state locked across the temporary Waiting -> Idle -> Active transition, so unmap / buffer_destroy cannot observe the transient Idle state during map completion.

Also added a focused state-machine regression test covering the core property: another thread cannot acquire map_state and observe the temporary Idle value while the map path still holds the lock.

Testing:

  • cargo fmt --check
  • cargo check -p wgpu-core
  • cargo test -p wgpu-core temporary_idle_map_state_is_not_observable_without_lock
  • cargo test -p wgpu-core

@danyalahmed1995 danyalahmed1995 force-pushed the fix-buffer-map-destroy-race branch from f1acfc0 to 9a1e7b7 Compare May 30, 2026 21:22
@andyleiserson
Copy link
Copy Markdown
Contributor

Thanks for this. We may not take the change right away, because there is some other locking cleanup (#9524 and #9479) that will put us on firmer ground for assessing locking correctness. But we definitely want to fix it at some point.

Buffer::map is not the only place that the map_state lock is acquired transiently to update the state as part of a larger operation. Did you assess whether any of the others are problematic?

I mentioned in the issue that I have a stress test that is able to reproduce the problem. If I get a chance I will clean that up and see if we can include it in CI. The proposed unit test is just illustrating code behavior without exercising the implementation, so doesn't have a lot of value. Even if it did exercise the implementation, it can be hard to get good coverage of problems like this one with unit tests.

@andyleiserson andyleiserson marked this pull request as draft June 4, 2026 17:21
@danyalahmed1995
Copy link
Copy Markdown
Author

Hiii , So I originally only focused on the specific Buffer::map interleaving from #9552, where Waiting is temporarily replaced with Idle while map completion is still in progress and buffer_destroy can observe that through unmap.

But however after your comment I did a pass over the other map_state transitions. The exact #9552 pattern still looks specific to Buffer::map, but there are other places, especially around unmap_inner, where Idle is installed before the larger operation has fully completed. Right now the PR scope is narrow and if we want to make it in broader direction i am going to need your input or like direction to make sure we are on the same page.

I can remove the test if you want, or leave the PR until #9524 / #9479 land and the lock-validation picture is clearer.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race between buffer mapping and buffer destruction

2 participants