Validate binding ranges against buffer size

[andyleiserson]

Fixes the issues noted in #1039 (comment). Together with #7874, this addresses #1039.

This tightens up the validation of binding sizes. I have added a helper on the wgpu_core Buffer type to resolve implicit remainder-of-buffer sizes and validate the binding offset and size against the size of the buffer. This helper is now the preferred way of constructing a hal::BufferBinding. Direct construction is no longer allowed in wgpu_core. There is an unsafe new_unchecked helper for cases where direct construction is necessary.

The more controversial and potentially risky part of this change is that the validation of binding size rejects zero-size bindings. Previously, zero-size bindings were allowed in create_buffer_binding, even though hal::BufferBinding was documented as not allowing zero-size bindings. So I think this version is safer, but it's possible that zero-size bindings were tolerated in some cases before that will no longer work.

I have left support for zero-size bindings in approximately the same state as it was, since attempting to disallow them entirely caused test failures. #3170

Testing
Tested locally using the following CTS tests:

webgpu:api,validation,encoding,cmds,render,setIndexBuffer:offset_and_size_oob:*
webgpu:api,validation,encoding,cmds,render,setVertexBuffer:offset_and_size_oob:*

The tests are still failing, due to #3170, but the situation is improved.

Squash or Rebase? Rebase (seems useful to keep the "validate binding ranges" and "restore zero-sized buffer" support commits separate, since it may be useful to revert the latter).

Checklist

• Run cargo fmt.
• Run taplo format.
• Run cargo clippy --tests. If applicable, add:
  • --target wasm32-unknown-unknown
• Run cargo xtask test to run tests.
• If this contains user-facing changes, add a CHANGELOG.md entry.

[andyleiserson]

Hmm. If disallowing zero-size bindings breaks the CTS in CI, then maybe it is a bad idea.

[fail] webgpu:api,validation,encoding,cmds,index_access:out_of_bounds_zero_sized_index_buffer:indexCount=0;firstIndex=0;instanceCount=1 (18ms). Log:
  - EXCEPTION: Error: Unexpected validation error occurred: In a set_index_buffer command: Buffer Buffer with '' label: Binding offset 0 is greater than or equal to buffer size 0

[jimblandy]

Thank you very much for the safety condition comments and documentation!

We should take this patch as it stands, but I think the way it's written may suggest a misunderstanding of an important aspect of wgpu's architecture, and we should address that misunderstanding in a follow-up PR. See the comments here:

wgpu/wgpu-hal/src/lib.rs

Lines 181 to 195 in 3d0fe3a

	//! A desire for "defense in depth" may suggest performing additional validation
	//! in `wgpu-hal` when the opportunity arises, but this must be done with
	//! caution. Even experienced contributors infer the expectations their changes
	//! must meet by considering not just requirements made explicit in types, tests,
	//! assertions, and comments, but also those implicit in the surrounding code.
	//! When one sees validation or state-tracking code in `wgpu-hal`, it is tempting
	//! to conclude, "Oh, `wgpu-hal` checks for this, so `wgpu-core` needn't worry
	//! about it - that would be redundant!" The responsibility for exhaustive
	//! validation always rests with `wgpu-core`, regardless of what may or may not
	//! be checked in `wgpu-hal`.
	//!
	//! To this end, any "defense in depth" validation that does appear in `wgpu-hal`
	//! for requirements that `wgpu-core` should have enforced should report failure
	//! via the `unreachable!` macro, because problems detected at this stage always
	//! indicate a bug in `wgpu-core`.

Specifically, it doesn't seem valuable to make BufferBinding::new_unchecked unsafe when the only thing you can do with it is pass it to an unsafe Device or CommandEncoder method anyway.

I admit this concern is arguable: certainly the code does continue to do necessary validation in wgpu-core, with the wgpu-hal changes merely forcing people to think about safety conditions in a more focused place (at BufferBinding creation time) than they would otherwise (at set_index_buffer, create_bind_group, etc. time).

[jimblandy]

This is fine for now, but for the future: documentation should not generally explain its own history; we have git and commit log messages for that. The documentation's audience is someone trying to understand what rules they need to follow today. Providing rationale and context can serve that goal, but the above seems distracting.