Allow unspecified bits when deserializing the API's bitflags (#3229)

CHANGELOG.md

@@ -87,6 +87,7 @@ Bottom level categories:
 - Remove `wgpu_types::Features::DEPTH24PLUS_STENCIL8`, making `wgpu::TextureFormat::Depth24PlusStencil8` available on all backends. By @Healthire in (#3151)[https://github.com/gfx-rs/wgpu/pull/3151]
 - Fix an integer overflow in `queue_write_texture` by @nical in (#3146)[https://github.com/gfx-rs/wgpu/pull/3146]
 - Make `RenderPassCompatibilityError` and `CreateShaderModuleError` not so huge. By @jimblandy in (#3226)[https://github.com/gfx-rs/wgpu/pull/3226]
+- Check for invalid bitflag bits in wgpu-core and allow them to be captured/replayed by @nical in (#3229)[https://github.com/gfx-rs/wgpu/pull/3229]
 
 #### WebGPU
 

Cargo.lock

@@ -173,16 +173,6 @@ version = "1.3.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a"
 
-[[package]]
-name = "bitflags_serde_shim"
-version = "0.2.2"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "25c3d626f0280ec39b33a6fc5c6c1067432b4c41e94aee40ded197a6649bf025"
-dependencies = [
- "bitflags",
- "serde",
-]
-
 [[package]]
 name = "block"
 version = "0.1.6"
@@ -2898,7 +2888,6 @@ name = "wgpu-types"
 version = "0.14.0"
 dependencies = [
  "bitflags",
- "bitflags_serde_shim",
  "serde",
  "serde_json",
 ]

Cargo.toml

@@ -46,7 +46,6 @@ version = "0.10"
 arrayvec = "0.7"
 async-executor = "1.0"
 bitflags = "1"
-bitflags_serde_shim = "0.2"
 bit-vec = "0.6"
 bytemuck = "1.4"
 cargo-run-wasm = "0.2.0"

wgpu-core/src/binding_model.rs

@@ -50,6 +50,8 @@ pub enum CreateBindGroupLayoutError {
     TooManyBindings(BindingTypeMaxCountError),
     #[error("Binding index {binding} is greater than the maximum index {maximum}")]
     InvalidBindingIndex { binding: u32, maximum: u32 },
+    #[error("Invalid visibility {0:?}")]
+    InvalidVisibility(wgt::ShaderStages),
 }
 
 //TODO: refactor this to move out `enum BindingError`.

wgpu-core/src/device/mod.rs

@@ -600,8 +600,8 @@ impl<A: HalApi> Device<A> {
 
         let mut usage = conv::map_buffer_usage(desc.usage);
 
-        if desc.usage.is_empty() {
-            return Err(resource::CreateBufferError::EmptyUsage);
+        if desc.usage.is_empty() || desc.usage.contains_invalid_bits() {
+            return Err(resource::CreateBufferError::InvalidUsage(desc.usage));
         }
 
         if !self
@@ -717,8 +717,8 @@ impl<A: HalApi> Device<A> {
     ) -> Result<resource::Texture<A>, resource::CreateTextureError> {
         use resource::{CreateTextureError, TextureDimensionError};
 
-        if desc.usage.is_empty() {
-            return Err(CreateTextureError::EmptyUsage);
+        if desc.usage.is_empty() || desc.usage.contains_invalid_bits() {
+            return Err(CreateTextureError::InvalidUsage(desc.usage));
         }
 
         conv::check_texture_dimension_size(
@@ -1560,6 +1560,13 @@ impl<A: HalApi> Device<A> {
                         error,
                     })?;
             }
+
+            if entry.visibility.contains_invalid_bits() {
+                return Err(
+                    binding_model::CreateBindGroupLayoutError::InvalidVisibility(entry.visibility),
+                );
+            }
+
             if entry.visibility.contains(wgt::ShaderStages::VERTEX) {
                 if writable_storage == WritableStorage::Yes {
                     required_features |= wgt::Features::VERTEX_WRITABLE_STORAGE;
@@ -2650,6 +2657,10 @@ impl<A: HalApi> Device<A> {
         for (i, cs) in color_targets.iter().enumerate() {
             if let Some(cs) = cs.as_ref() {
                 let error = loop {
+                    if cs.write_mask.contains_invalid_bits() {
+                        break Some(pipeline::ColorStateError::InvalidWriteMask(cs.write_mask));
+                    }
+
                     let format_features = self.describe_format_features(adapter, cs.format)?;
                     if !format_features
                         .allowed_usages

wgpu-core/src/pipeline.rs

@@ -303,6 +303,8 @@ pub enum ColorStateError {
     },
     #[error("blend factors for {0:?} must be `One`")]
     InvalidMinMaxBlendFactors(wgt::BlendComponent),
+    #[error("invalid write mask {0:?}")]
+    InvalidWriteMask(wgt::ColorWrites),
 }
 
 #[derive(Clone, Debug, Error)]

wgpu-core/src/resource.rs

@@ -242,8 +242,8 @@ pub enum CreateBufferError {
     AccessError(#[from] BufferAccessError),
     #[error("buffers that are mapped at creation have to be aligned to `COPY_BUFFER_ALIGNMENT`")]
     UnalignedSize,
-    #[error("Buffers cannot have empty usage flags")]
-    EmptyUsage,
+    #[error("Invalid usage flags {0:?}")]
+    InvalidUsage(wgt::BufferUsages),
     #[error("`MAP` usage can only be combined with the opposite `COPY`, requested {0:?}")]
     UsageMismatch(wgt::BufferUsages),
     #[error("Buffer size {requested} is greater than the maximum buffer size ({maximum})")]
@@ -490,8 +490,8 @@ pub enum TextureDimensionError {
 pub enum CreateTextureError {
     #[error(transparent)]
     Device(#[from] DeviceError),
-    #[error("Textures cannot have empty usage flags")]
-    EmptyUsage,
+    #[error("Invalid usage flags {0:?}")]
+    InvalidUsage(wgt::TextureUsages),
     #[error(transparent)]
     InvalidDimension(#[from] TextureDimensionError),
     #[error("Depth texture ({1:?}) can't be created as {0:?}")]

wgpu-types/Cargo.toml

@@ -16,13 +16,12 @@ rustdoc-args = ["--cfg", "docsrs"]
 [lib]
 
 [features]
-trace = ["serde", "bitflags_serde_shim"]
-replay = ["serde", "bitflags_serde_shim"]
+trace = ["serde"]
+replay = ["serde"]
 
 [dependencies]
 bitflags.workspace = true
 serde = { workspace = true, features = ["serde_derive"], optional = true }
-bitflags_serde_shim = { workspace = true, optional = true }
 
 [dev-dependencies]
 serde_json.workspace = true

wgpu-types/src/lib.rs

@@ -14,6 +14,48 @@ use serde::{Deserialize, Serialize};
 use std::hash::{Hash, Hasher};
 use std::{num::NonZeroU32, ops::Range};
 
+// Use this macro instead of the one provided by the bitflags_serde_shim crate
+// because the latter produces an error when deserializing bits that are not
+// specified in the bitflags, while we want deserialization to succeed and
+// and unspecified bits to lead to errors handled in wgpu-core.
+// Note that plainly deriving Serialize and Deserialized would have a similar
+// behavior to this macro (unspecified bit do not produce an error).
+macro_rules! impl_bitflags {
+    ($name:ident) => {
+        #[cfg(feature = "trace")]
+        impl serde::Serialize for $name {
+            fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+            where
+                S: serde::Serializer,
+            {
+                self.bits().serialize(serializer)
+            }
+        }
+
+        #[cfg(feature = "replay")]
+        impl<'de> serde::Deserialize<'de> for $name {
+            fn deserialize<D>(deserializer: D) -> Result<$name, D::Error>
+            where
+                D: serde::Deserializer<'de>,
+            {
+                let value = <_ as serde::Deserialize<'de>>::deserialize(deserializer)?;
+                // Note: newer version of bitflags replaced from_bits_unchecked with
+                // from_bits_retain which is not marked as unsafe (same implementation).
+                Ok(unsafe { $name::from_bits_unchecked(value) })
+            }
+        }
+
+        impl $name {
+            /// Returns true if the bitflags contains bits that are not part of
+            /// the bitflags definition.
+            pub fn contains_invalid_bits(&self) -> bool {
+                let all = Self::all().bits();
+                (self.bits() | all) != all
+            }
+        }
+    };
+}
+
 /// Integral type used for buffer offsets.
 pub type BufferAddress = u64;
 /// Integral type used for buffer slice sizes.
@@ -115,8 +157,7 @@ bitflags::bitflags! {
     }
 }
 
-#[cfg(feature = "bitflags_serde_shim")]
-bitflags_serde_shim::impl_serde_for_bitflags!(Backends);
+impl_bitflags!(Backends);
 
 impl From<Backend> for Backends {
     fn from(backend: Backend) -> Self {
@@ -633,8 +674,7 @@ bitflags::bitflags! {
     }
 }
 
-#[cfg(feature = "bitflags_serde_shim")]
-bitflags_serde_shim::impl_serde_for_bitflags!(Features);
+impl_bitflags!(Features);
 
 impl Features {
     /// Mask of all features which are part of the upstream WebGPU standard.
@@ -1090,8 +1130,7 @@ bitflags::bitflags! {
     }
 }
 
-#[cfg(feature = "bitflags_serde_shim")]
-bitflags_serde_shim::impl_serde_for_bitflags!(DownlevelFlags);
+impl_bitflags!(DownlevelFlags);
 
 impl DownlevelFlags {
     /// All flags that indicate if the backend is WebGPU compliant
@@ -1213,8 +1252,7 @@ bitflags::bitflags! {
     }
 }
 
-#[cfg(feature = "bitflags_serde_shim")]
-bitflags_serde_shim::impl_serde_for_bitflags!(ShaderStages);
+impl_bitflags!(ShaderStages);
 
 /// Dimensions of a particular texture view.
 ///
@@ -1666,8 +1704,7 @@ impl TextureFormatFeatureFlags {
     }
 }
 
-#[cfg(feature = "bitflags_serde_shim")]
-bitflags_serde_shim::impl_serde_for_bitflags!(TextureFormatFeatureFlags);
+impl_bitflags!(TextureFormatFeatureFlags);
 
 /// Features supported by a given texture format
 ///
@@ -3085,8 +3122,7 @@ bitflags::bitflags! {
     }
 }
 
-#[cfg(feature = "bitflags_serde_shim")]
-bitflags_serde_shim::impl_serde_for_bitflags!(ColorWrites);
+impl_bitflags!(ColorWrites);
 
 impl Default for ColorWrites {
     fn default() -> Self {
@@ -3644,8 +3680,7 @@ bitflags::bitflags! {
     }
 }
 
-#[cfg(feature = "bitflags_serde_shim")]
-bitflags_serde_shim::impl_serde_for_bitflags!(BufferUsages);
+impl_bitflags!(BufferUsages);
 
 /// Describes a [`Buffer`](../wgpu/struct.Buffer.html).
 ///
@@ -3846,8 +3881,7 @@ bitflags::bitflags! {
     }
 }
 
-#[cfg(feature = "bitflags_serde_shim")]
-bitflags_serde_shim::impl_serde_for_bitflags!(TextureUsages);
+impl_bitflags!(TextureUsages);
 
 /// Configures a [`Surface`] for presentation.
 ///
@@ -5046,8 +5080,7 @@ bitflags::bitflags! {
     }
 }
 
-#[cfg(feature = "bitflags_serde_shim")]
-bitflags_serde_shim::impl_serde_for_bitflags!(PipelineStatisticsTypes);
+impl_bitflags!(PipelineStatisticsTypes);
 
 /// Argument buffer layout for draw_indirect commands.
 #[repr(C)]

wgpu/tests/buffer_usages.rs

@@ -50,7 +50,8 @@ fn buffer_usage() {
         Bu::MAP_WRITE | Bu::COPY_SRC | Bu::STORAGE,
         Bu::all(),
     ];
-    let always_fail = &[Bu::empty()];
+    let invalid_bits = unsafe { Bu::from_bits_unchecked(0b1111111111111) };
+    let always_fail = &[Bu::empty(), invalid_bits];
 
     try_create(
         false,