Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Record that the buffer is mapped when its size is zero. #2877

Merged
merged 3 commits into from
Jul 13, 2022

Conversation

nical
Copy link
Contributor

@nical nical commented Jul 13, 2022

Checklist

  • Run cargo clippy.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Description

This makes buffers behave properly when the user maps and unmaps an empty buffer range. Without this PR, 'unmap' will return an error saying that the buffer is not mapped even though the mapping closure reproted success.

It's a bit of a ridiculous test cases but fuzzers love this type of input.

The NonNull::dangling pointer might be intimidating, thankfully it's used in very few places so it's easy to check that we don't do dangerous things things with it. It can be summed up to the following snippet which I ran under miri without complaints of undefined behavior:

    #[test]
    fn it_works() {
        unsafe {
            let nonnul_ptr: std::ptr::NonNull<u8> = std::ptr::NonNull::dangling();
            let actual_ptr = nonnul_ptr.as_ptr();
            let offset = actual_ptr.offset(0);
            let slice = std::slice::from_raw_parts(actual_ptr, 0);
            println!("ok!");
        }
    }

Testing

No automated tests.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh edge cases. Thank you!

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) July 13, 2022 16:36
@cwfitzgerald cwfitzgerald disabled auto-merge July 13, 2022 17:20
@cwfitzgerald
Copy link
Member

Format is mad

@nical nical force-pushed the map-empty-range branch from 808be10 to 82734cd Compare July 13, 2022 17:43
@nical
Copy link
Contributor Author

nical commented Jul 13, 2022

Format is mad

Oops! Fixed.

@nical nical force-pushed the map-empty-range branch from 82734cd to 4b3f5a1 Compare July 13, 2022 17:46
@cwfitzgerald cwfitzgerald enabled auto-merge (squash) July 13, 2022 17:52
@cwfitzgerald cwfitzgerald merged commit 6058676 into gfx-rs:master Jul 13, 2022
cwfitzgerald pushed a commit to cwfitzgerald/wgpu that referenced this pull request Jul 14, 2022
* Record that the buffer is mapped when its size is zero.

* Avoid internally trying to map a zero-sized buffer if mapped_at_creation is true.

* Add an entry in the changelog.
cwfitzgerald pushed a commit that referenced this pull request Jul 14, 2022
* Record that the buffer is mapped when its size is zero.

* Avoid internally trying to map a zero-sized buffer if mapped_at_creation is true.

* Add an entry in the changelog.
@nical nical deleted the map-empty-range branch July 14, 2022 11:45
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.

2 participants