From 741844cc2b3a2944a31455974bcd1df76a115380 Mon Sep 17 00:00:00 2001 From: yanchith Date: Fri, 17 Jan 2020 14:20:20 +0100 Subject: [PATCH 1/8] Fix clippy errors In wgpu-remote, some functions are now marked `unsafe` on the outside, because they dereferenced a raw pointer they couldn't have checked themselves. --- wgpu-remote/src/lib.rs | 12 ++++++------ wgpu-remote/src/server.rs | 20 ++++++++------------ 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/wgpu-remote/src/lib.rs b/wgpu-remote/src/lib.rs index 310c09f99d..a684a66a6f 100644 --- a/wgpu-remote/src/lib.rs +++ b/wgpu-remote/src/lib.rs @@ -71,20 +71,20 @@ pub extern "C" fn wgpu_client_new() -> Infrastructure { } #[no_mangle] -pub extern "C" fn wgpu_client_delete(client: *mut Client) { +pub unsafe extern "C" fn wgpu_client_delete(client: *mut Client) { log::info!("Terminating WGPU client"); - let _client = unsafe { Box::from_raw(client) }; + let _client = Box::from_raw(client); } #[no_mangle] -pub extern "C" fn wgpu_client_make_adapter_ids( +pub unsafe extern "C" fn wgpu_client_make_adapter_ids( client: &Client, ids: *mut id::AdapterId, id_length: usize, ) -> usize { let mut identities = client.identities.lock(); assert_ne!(id_length, 0); - let mut ids = unsafe { slice::from_raw_parts_mut(ids, id_length) }.iter_mut(); + let mut ids = slice::from_raw_parts_mut(ids, id_length).iter_mut(); *ids.next().unwrap() = identities.vulkan.adapters.alloc(Backend::Vulkan); @@ -101,13 +101,13 @@ pub extern "C" fn wgpu_client_make_adapter_ids( } #[no_mangle] -pub extern "C" fn wgpu_client_kill_adapter_ids( +pub unsafe extern "C" fn wgpu_client_kill_adapter_ids( client: &Client, ids: *const id::AdapterId, id_length: usize, ) { let mut identity = client.identities.lock(); - let ids = unsafe { slice::from_raw_parts(ids, id_length) }; + let ids = slice::from_raw_parts(ids, id_length); for &id in ids { identity.select(id.backend()).adapters.free(id) } diff --git a/wgpu-remote/src/server.rs b/wgpu-remote/src/server.rs index abe7a6947a..7e7327f87c 100644 --- a/wgpu-remote/src/server.rs +++ b/wgpu-remote/src/server.rs @@ -15,9 +15,9 @@ pub extern "C" fn wgpu_server_new() -> *mut Global { } #[no_mangle] -pub extern "C" fn wgpu_server_delete(global: *mut Global) { +pub unsafe extern "C" fn wgpu_server_delete(global: *mut Global) { log::info!("Terminating WGPU server"); - unsafe { Box::from_raw(global) }.delete(); + Box::from_raw(global).delete(); log::info!("\t...done"); } @@ -26,13 +26,13 @@ pub extern "C" fn wgpu_server_delete(global: *mut Global) { /// /// Returns the index in this list, or -1 if unable to pick. #[no_mangle] -pub extern "C" fn wgpu_server_instance_request_adapter( +pub unsafe extern "C" fn wgpu_server_instance_request_adapter( global: &Global, desc: &core::instance::RequestAdapterOptions, ids: *const id::AdapterId, id_length: usize, ) -> i8 { - let ids = unsafe { slice::from_raw_parts(ids, id_length) }; + let ids = slice::from_raw_parts(ids, id_length); match global.pick_adapter( desc, core::instance::AdapterInputs::IdSet(ids, |i| i.backend()), @@ -68,7 +68,7 @@ pub extern "C" fn wgpu_server_device_create_buffer( } #[no_mangle] -pub extern "C" fn wgpu_server_device_set_buffer_sub_data( +pub unsafe extern "C" fn wgpu_server_device_set_buffer_sub_data( global: &Global, self_id: id::DeviceId, buffer_id: id::BufferId, @@ -76,14 +76,12 @@ pub extern "C" fn wgpu_server_device_set_buffer_sub_data( data: *const u8, size: core::BufferAddress, ) { - let slice = unsafe { - slice::from_raw_parts(data, size as usize) - }; + let slice = slice::from_raw_parts(data, size as usize); gfx_select!(self_id => global.device_set_buffer_sub_data(self_id, buffer_id, offset, slice)); } #[no_mangle] -pub extern "C" fn wgpu_server_device_get_buffer_sub_data( +pub unsafe extern "C" fn wgpu_server_device_get_buffer_sub_data( global: &Global, self_id: id::DeviceId, buffer_id: id::BufferId, @@ -91,9 +89,7 @@ pub extern "C" fn wgpu_server_device_get_buffer_sub_data( data: *mut u8, size: core::BufferAddress, ) { - let slice = unsafe { - slice::from_raw_parts_mut(data, size as usize) - }; + let slice = slice::from_raw_parts_mut(data, size as usize); gfx_select!(self_id => global.device_get_buffer_sub_data(self_id, buffer_id, offset, slice)); } From 53b9a01b2f6fdc4984a51b0c374736665887f64c Mon Sep 17 00:00:00 2001 From: yanchith Date: Fri, 17 Jan 2020 14:44:02 +0100 Subject: [PATCH 2/8] Resolve trivial clippy warning --- wgpu-core/src/device/life.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/wgpu-core/src/device/life.rs b/wgpu-core/src/device/life.rs index 4468803857..b7c4934b94 100644 --- a/wgpu-core/src/device/life.rs +++ b/wgpu-core/src/device/life.rs @@ -195,7 +195,7 @@ impl LifetimeTracker { } /// Find the pending entry with the lowest active index. If none can be found that means - /// everything in the allocator can be cleaned up, so std::usize::MAX is correct. + /// everything in the allocator can be cleaned up, so std::usize::MAX is correct. pub fn lowest_active_submission(&self) -> SubmissionIndex { self.active .iter() @@ -225,7 +225,7 @@ impl LifetimeTracker { .active .iter() .position(|a| unsafe { !device.get_fence_status(&a.fence).unwrap() }) - .unwrap_or(self.active.len()); + .unwrap_or_else(|| self.active.len()); let last_done = if done_count != 0 { self.active[done_count - 1].index } else { From 0d9ad70468c1807322203fb9e73935e290ef7654 Mon Sep 17 00:00:00 2001 From: yanchith Date: Fri, 17 Jan 2020 14:44:17 +0100 Subject: [PATCH 3/8] Add TODOs for range_plus_one clippy warnings --- wgpu-core/src/command/transfer.rs | 28 ++++++++++++++++++++-------- wgpu-core/src/track/texture.rs | 10 +++++++++- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/wgpu-core/src/command/transfer.rs b/wgpu-core/src/command/transfer.rs index 0b5a4b4be6..7fe2e8dd7c 100644 --- a/wgpu-core/src/command/transfer.rs +++ b/wgpu-core/src/command/transfer.rs @@ -43,19 +43,31 @@ impl TextureCopyView { fn to_selector(&self, aspects: hal::format::Aspects) -> hal::image::SubresourceRange { let level = self.mip_level as hal::image::Level; let layer = self.array_layer as hal::image::Layer; - hal::image::SubresourceRange { - aspects, - levels: level .. level + 1, - layers: layer .. layer + 1, + + // TODO: Can't satisfy clippy here unless we modify + // `hal::image::SubresourceRange` in gfx to use `std::ops::RangeBounds`. + #[allow(clippy::range_plus_one)] + { + hal::image::SubresourceRange { + aspects, + levels: level .. level + 1, + layers: layer .. layer + 1, + } } } fn to_sub_layers(&self, aspects: hal::format::Aspects) -> hal::image::SubresourceLayers { let layer = self.array_layer as hal::image::Layer; - hal::image::SubresourceLayers { - aspects, - level: self.mip_level as hal::image::Level, - layers: layer .. layer + 1, + // TODO: Can't satisfy clippy here unless we modify + // `hal::image::SubresourceLayers` in gfx to use + // `std::ops::RangeBounds`. + #[allow(clippy::range_plus_one)] + { + hal::image::SubresourceLayers { + aspects, + level: self.mip_level as hal::image::Level, + layers: layer .. layer + 1, + } } } } diff --git a/wgpu-core/src/track/texture.rs b/wgpu-core/src/track/texture.rs index d93997c733..7452f4de71 100644 --- a/wgpu-core/src/track/texture.rs +++ b/wgpu-core/src/track/texture.rs @@ -106,6 +106,10 @@ impl ResourceState for TextureState { if unit.last == usage && TextureUsage::ORDERED.contains(usage) { continue; } + // TODO: Can't satisfy clippy here unless we modify + // `hal::image::SubresourceRange` in gfx to use + // `std::ops::RangeBounds`. + #[allow(clippy::range_plus_one)] let pending = PendingTransition { id, selector: hal::image::SubresourceRange { @@ -178,11 +182,15 @@ impl ResourceState for TextureState { last: end.last, } } else { + // TODO: Can't satisfy clippy here unless we modify + // `hal::image::SubresourceRange` in gfx to use + // `std::ops::RangeBounds`. + #[allow(clippy::range_plus_one)] let pending = PendingTransition { id, selector: hal::image::SubresourceRange { aspects: hal::format::Aspects::empty(), - levels: level .. level+1, + levels: level .. level + 1, layers: layers.clone(), }, usage: start.last .. to_usage, From 8414b0140387fad002d7200a15ebf8432bb22172 Mon Sep 17 00:00:00 2001 From: yanchith Date: Fri, 17 Jan 2020 15:04:57 +0100 Subject: [PATCH 4/8] Add # Safety docs to wgpu-remote Only two unsafe functions were used internally: - `slice::from_raw_parts` - `Box::from_raw` The safety messages are adapted from the safety messages of these functions. --- wgpu-remote/src/lib.rs | 13 +++++++++++++ wgpu-remote/src/server.rs | 31 +++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/wgpu-remote/src/lib.rs b/wgpu-remote/src/lib.rs index a684a66a6f..693c504d4e 100644 --- a/wgpu-remote/src/lib.rs +++ b/wgpu-remote/src/lib.rs @@ -70,12 +70,21 @@ pub extern "C" fn wgpu_client_new() -> Infrastructure { } } +/// # Safety +/// +/// This function is unsafe because improper use may lead to memory +/// problems. For example, a double-free may occur if the function is called +/// twice on the same raw pointer. #[no_mangle] pub unsafe extern "C" fn wgpu_client_delete(client: *mut Client) { log::info!("Terminating WGPU client"); let _client = Box::from_raw(client); } +/// # Safety +/// +/// This function is unsafe as there is no guarantee that the given pointer is +/// valid for `id_length` elements. #[no_mangle] pub unsafe extern "C" fn wgpu_client_make_adapter_ids( client: &Client, @@ -100,6 +109,10 @@ pub unsafe extern "C" fn wgpu_client_make_adapter_ids( id_length - ids.len() } +/// # Safety +/// +/// This function is unsafe as there is no guarantee that the given pointer is +/// valid for `id_length` elements. #[no_mangle] pub unsafe extern "C" fn wgpu_client_kill_adapter_ids( client: &Client, diff --git a/wgpu-remote/src/server.rs b/wgpu-remote/src/server.rs index 7e7327f87c..ccabb93b87 100644 --- a/wgpu-remote/src/server.rs +++ b/wgpu-remote/src/server.rs @@ -14,6 +14,11 @@ pub extern "C" fn wgpu_server_new() -> *mut Global { Box::into_raw(Box::new(Global::new("wgpu"))) } +/// # Safety +/// +/// This function is unsafe because improper use may lead to memory +/// problems. For example, a double-free may occur if the function is called +/// twice on the same raw pointer. #[no_mangle] pub unsafe extern "C" fn wgpu_server_delete(global: *mut Global) { log::info!("Terminating WGPU server"); @@ -25,6 +30,11 @@ pub unsafe extern "C" fn wgpu_server_delete(global: *mut Global) { /// Provide the list of IDs to pick from. /// /// Returns the index in this list, or -1 if unable to pick. +/// +/// # Safety +/// +/// This function is unsafe as there is no guarantee that the given pointer is +/// valid for `id_length` elements. #[no_mangle] pub unsafe extern "C" fn wgpu_server_instance_request_adapter( global: &Global, @@ -67,6 +77,10 @@ pub extern "C" fn wgpu_server_device_create_buffer( gfx_select!(self_id => global.device_create_buffer(self_id, desc, new_id)); } +/// # Safety +/// +/// This function is unsafe as there is no guarantee that the given pointer is +/// valid for `size` elements. #[no_mangle] pub unsafe extern "C" fn wgpu_server_device_set_buffer_sub_data( global: &Global, @@ -80,6 +94,10 @@ pub unsafe extern "C" fn wgpu_server_device_set_buffer_sub_data( gfx_select!(self_id => global.device_set_buffer_sub_data(self_id, buffer_id, offset, slice)); } +/// # Safety +/// +/// This function is unsafe as there is no guarantee that the given pointer is +/// valid for `size` elements. #[no_mangle] pub unsafe extern "C" fn wgpu_server_device_get_buffer_sub_data( global: &Global, @@ -118,6 +136,10 @@ pub extern "C" fn wgpu_server_encoder_destroy( gfx_select!(self_id => global.command_encoder_destroy(self_id)); } +/// # Safety +/// +/// This function is unsafe as there is no guarantee that the given pointer is +/// valid for `byte_length` elements. #[no_mangle] pub unsafe extern "C" fn wgpu_server_encode_compute_pass( global: &Global, @@ -129,6 +151,11 @@ pub unsafe extern "C" fn wgpu_server_encode_compute_pass( gfx_select!(self_id => global.command_encoder_run_compute_pass(self_id, raw_data)); } +/// # Safety +/// +/// This function is unsafe as there is no guarantee that the given pointers are +/// valid for `color_attachments_length` and `command_length` elements, +/// respectively. #[no_mangle] pub unsafe extern "C" fn wgpu_server_encode_render_pass( global: &Global, @@ -144,6 +171,10 @@ pub unsafe extern "C" fn wgpu_server_encode_render_pass( gfx_select!(self_id => global.command_encoder_run_render_pass(self_id, color_attachments, depth_stencil_attachment, raw_pass)); } +/// # Safety +/// +/// This function is unsafe as there is no guarantee that the given pointer is +/// valid for `command_buffer_id_length` elements. #[no_mangle] pub unsafe extern "C" fn wgpu_server_queue_submit( global: &Global, From d24d7157cf7689b59a4647dc1b99063385f20f57 Mon Sep 17 00:00:00 2001 From: yanchith Date: Fri, 17 Jan 2020 15:18:35 +0100 Subject: [PATCH 5/8] Add # Safety docs to wgpu-native --- wgpu-native/src/command.rs | 10 ++++++++++ wgpu-native/src/device.rs | 11 +++++++++++ 2 files changed, 21 insertions(+) diff --git a/wgpu-native/src/command.rs b/wgpu-native/src/command.rs index ddbb67a6d6..bccdffc5d1 100644 --- a/wgpu-native/src/command.rs +++ b/wgpu-native/src/command.rs @@ -83,6 +83,11 @@ pub extern "C" fn wgpu_command_encoder_copy_texture_to_texture( } +/// # Safety +/// +/// This function is unsafe because improper use may lead to memory +/// problems. For example, a double-free may occur if the function is called +/// twice on the same raw pointer. #[no_mangle] pub unsafe extern "C" fn wgpu_render_pass_end_pass(pass_id: id::RenderPassId) { let (pass_data, encoder_id, targets) = Box::from_raw(pass_id).finish_render(); @@ -114,6 +119,11 @@ pub unsafe extern "C" fn wgpu_render_pass_end_pass(pass_id: id::RenderPassId) { gfx_select!(encoder_id => GLOBAL.command_encoder_run_render_pass(encoder_id, &color_attachments, depth_stencil_attachment, &pass_data)) } +/// # Safety +/// +/// This function is unsafe because improper use may lead to memory +/// problems. For example, a double-free may occur if the function is called +/// twice on the same raw pointer. #[no_mangle] pub unsafe extern "C" fn wgpu_compute_pass_end_pass(pass_id: id::ComputePassId) { let (pass_data, encoder_id) = Box::from_raw(pass_id).finish_compute(); diff --git a/wgpu-native/src/device.rs b/wgpu-native/src/device.rs index f178b014ab..5107869cac 100644 --- a/wgpu-native/src/device.rs +++ b/wgpu-native/src/device.rs @@ -135,6 +135,9 @@ pub extern "C" fn wgpu_create_surface_from_windows_hwnd( )) } +/// # Safety +/// +/// This function is unsafe as it calls an unsafe extern callback. #[no_mangle] pub unsafe extern "C" fn wgpu_request_adapter_async( desc: Option<&core::instance::RequestAdapterOptions>, @@ -181,6 +184,10 @@ pub extern "C" fn wgpu_device_create_buffer( gfx_select!(device_id => GLOBAL.device_create_buffer(device_id, desc, PhantomData)) } +/// # Safety +/// +/// This function is unsafe as there is no guarantee that the given pointer +/// dereferenced in this function is valid. #[no_mangle] pub unsafe extern "C" fn wgpu_device_create_buffer_mapped( device_id: id::DeviceId, @@ -297,6 +304,10 @@ pub extern "C" fn wgpu_device_get_queue(device_id: id::DeviceId) -> id::QueueId device_id } +/// # Safety +/// +/// This function is unsafe as there is no guarantee that the given pointer is +/// valid for `command_buffers_length` elements. #[no_mangle] pub unsafe extern "C" fn wgpu_queue_submit( queue_id: id::QueueId, From ab205b042c2e3e5229ef0be152b3a8e93ec45597 Mon Sep 17 00:00:00 2001 From: yanchith Date: Fri, 17 Jan 2020 16:15:05 +0100 Subject: [PATCH 6/8] Add # Safery docs to wgpu-core --- wgpu-core/src/command/compute.rs | 6 ++++++ wgpu-core/src/command/mod.rs | 7 ++++++- wgpu-core/src/command/render.rs | 12 ++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/wgpu-core/src/command/compute.rs b/wgpu-core/src/command/compute.rs index 631488dfec..80bec01e35 100644 --- a/wgpu-core/src/command/compute.rs +++ b/wgpu-core/src/command/compute.rs @@ -225,6 +225,12 @@ pub mod compute_ffi { }; use std::{convert::TryInto, slice}; + /// # Safety + /// + /// This function is unsafe as there is no guarantee that the given pointer is + /// valid for `offset_length` elements. + // TODO: There might be other safety issues, such as using the unsafe + // `RawPass::encode` and `RawPass::encode_slice`. #[no_mangle] pub unsafe extern "C" fn wgpu_compute_pass_set_bind_group( pass: &mut RawPass, diff --git a/wgpu-core/src/command/mod.rs b/wgpu-core/src/command/mod.rs index d623e646df..4f373c22b8 100644 --- a/wgpu-core/src/command/mod.rs +++ b/wgpu-core/src/command/mod.rs @@ -197,7 +197,7 @@ pub struct CommandBufferDescriptor { } #[no_mangle] -pub unsafe extern "C" fn wgpu_command_encoder_begin_compute_pass( +pub extern "C" fn wgpu_command_encoder_begin_compute_pass( encoder_id: id::CommandEncoderId, _desc: Option<&ComputePassDescriptor>, ) -> *mut RawPass { @@ -220,6 +220,11 @@ pub struct RawRenderPass { targets: RawRenderTargets, } +/// # Safety +/// +/// This function is unsafe as there is no guarantee that the given pointer +/// (`RenderPassDescriptor::color_attachments`) is valid for +/// `RenderPassDescriptor::color_attachments_length` elements. #[no_mangle] pub unsafe extern "C" fn wgpu_command_encoder_begin_render_pass( encoder_id: id::CommandEncoderId, diff --git a/wgpu-core/src/command/render.rs b/wgpu-core/src/command/render.rs index 959cf7f04e..29a3609a09 100644 --- a/wgpu-core/src/command/render.rs +++ b/wgpu-core/src/command/render.rs @@ -1127,6 +1127,12 @@ pub mod render_ffi { }; use std::{convert::TryInto, slice}; + /// # Safety + /// + /// This function is unsafe as there is no guarantee that the given pointer is + /// valid for `offset_length` elements. + // TODO: There might be other safety issues, such as using the unsafe + // `RawPass::encode` and `RawPass::encode_slice`. #[no_mangle] pub unsafe extern "C" fn wgpu_render_pass_set_bind_group( pass: &mut RawRenderPass, @@ -1166,6 +1172,12 @@ pub mod render_ffi { }); } + /// # Safety + /// + /// This function is unsafe as there is no guarantee that the given pointers + /// (`buffer_ids` and `offsets`) are valid for `length` elements. + // TODO: There might be other safety issues, such as using the unsafe + // `RawPass::encode` and `RawPass::encode_slice`. #[no_mangle] pub unsafe extern "C" fn wgpu_render_pass_set_vertex_buffers( pass: &mut RawRenderPass, From b6215db57de7df5ee76ebbe6734eb0d04f76c246 Mon Sep 17 00:00:00 2001 From: yanchith Date: Fri, 17 Jan 2020 16:30:12 +0100 Subject: [PATCH 7/8] Modify CI script to run clippy instead of check --- .travis.yml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 8b80d51bf5..020ae86395 100644 --- a/.travis.yml +++ b/.travis.yml @@ -60,9 +60,11 @@ before_install: script: - cargo test # TODO: enable GL backend - - (cd wgpu-core && cargo check --all-features) - - if [[ $TRAVIS_OS_NAME == "osx" ]]; then (cd wgpu-native && cargo check --features vulkan-portability); fi - - if [[ $TRAVIS_OS_NAME == "linux" ]]; then cargo check --release; fi + - (cd wgpu-core && cargo clippy --all-features) + - (cd wgpu-native && cargo clippy) + - (cd wgpu-remote && cargo clippy --all-features) + - if [[ $TRAVIS_OS_NAME == "osx" ]]; then (cd wgpu-native && cargo clippy --features vulkan-portability); fi + - if [[ $TRAVIS_OS_NAME == "linux" ]]; then cargo clippy --release; fi - if [[ $TRAVIS_RUST_VERSION == "nightly" ]]; then cargo +nightly install cbindgen; fi - if [[ $TRAVIS_RUST_VERSION == "nightly" ]] && [[ $TRAVIS_OS_NAME == "windows" ]]; then wget -nc -O glfw.zip https://github.com/glfw/glfw/archive/3.3.zip && From cd114e780a0d1ea22f7c945b87df2c0aa7f0152d Mon Sep 17 00:00:00 2001 From: yanchith Date: Fri, 17 Jan 2020 17:58:42 +0100 Subject: [PATCH 8/8] Don't forget to install clippy --- .travis.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.travis.yml b/.travis.yml index 020ae86395..1aa12fbe5d 100644 --- a/.travis.yml +++ b/.travis.yml @@ -57,6 +57,9 @@ before_install: - if [[ $TRAVIS_RUST_VERSION != "nightly" ]] && [[ $TRAVIS_OS_NAME == "windows" ]]; then rustup default stable-msvc; fi - if [[ $TRAVIS_RUST_VERSION == "nightly" ]] && [[ $TRAVIS_OS_NAME == "windows" ]]; then rustup default nightly-msvc; fi +before_script: + - rustup component add clippy + script: - cargo test # TODO: enable GL backend