-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[core] fix invalid read in Queue::write_texture
#7893
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
[core] fix invalid read in Queue::write_texture
#7893
Conversation
9d63eba
to
0459867
Compare
Queue::write_texture
Queue::write_texture
Thanks for finding this! I'm curious, what were you doing when it showed up? My inclination would be to get rid of this unsafe code entirely (I would hope that the compiler can optimize away some redundant bounds checks when we're copying row-by-row), but it would also be good to get an opinion from @teoxoy on this. |
Writing bitmaps with paddings and an offset often enough that it eventually SEGV when the last read bytes where out of memory pages, most of the time it was just reading uninitialized bytes without crashing. I'm wondering if it should be an Also, please double review the use of |
0459867
to
a53d338
Compare
Removing the unsafe code would be nice, last time I touched this code I broke it and we went in UB land. IIRC the staging buffer copying code has been using unsafe since its inception but it would be great if it didn't and the compiler can elide unnecessary bounds checks (we could also encourage it to do so with safe code). |
commit 0a1ce66 Author: Mads Marquart <[email protected]> Date: Fri Apr 25 22:01:53 2025 +0200 Use normal objc2 naming scheme commit 35a9525 Author: Mads Marquart <[email protected]> Date: Tue Apr 29 14:24:41 2025 +0200 Use `objc2-metal` with `metal` naming scheme To keep the diff smaller and easier to review, this uses a temporary fork of `objc2-metal` and `objc2-quartz-core` whose methods use the naming scheme of the `metal` crate. One particular difficult part with this is that the `metal` crate has several methods where the order of the arguments are swapped relative to the corresponding Objective-C methods. This includes most perilously (since these have both an offset and an index argument, both of which are integers): - `set_bytes` - `set_vertex_bytes` - `set_fragment_bytes` - `set_buffer` - `set_vertex_buffer` - `set_fragment_buffer` - `set_threadgroup_memory_length` But also: - `set_vertex_texture` - `set_fragment_texture` - `set_sampler_state` - `set_vertex_sampler_state` - `set_fragment_sampler_state` Another noteworthy thing to mention is that `objc2-metal` does not (yet) provide a fallback for MTLCopyAllDevices, so we have to do that ourselves: madsmtm/objc2@3543940 commit ab07462 Author: Mads Marquart <[email protected]> Date: Fri Apr 25 22:01:34 2025 +0200 [examples] Do not request redraw on resize This is no longer necessary, since Winit can now properly issue redraw events on resize. commit b99da84 Author: Mads Marquart <[email protected]> Date: Fri Apr 25 22:00:41 2025 +0200 [metal] Use raw-window-metal to do layer creation This removes the manual implementation introduced in 7b00140. commit d2f8c44 Author: Teodor Tanasoaia <[email protected]> Date: Fri Jul 18 19:04:46 2025 +0200 [wgpu-core] remove implicit PL & BGL IDs from pipeline creation (gfx-rs#7967) commit 4ef4f5d Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Thu Jul 10 22:24:39 2025 +0000 chore(deps): update dependency ubuntu to v24 commit cf4a74c Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Thu Jul 10 22:24:26 2025 +0000 chore(deps): update deps and deny file commit 232d465 Author: Connor Fitzgerald <[email protected]> Date: Thu Jul 17 21:12:30 2025 -0400 ci: only allow one set of jobs per PR (gfx-rs#7958) commit 5d14f33 Author: Andy Leiserson <[email protected]> Date: Thu Jul 17 15:52:51 2025 -0700 Test for use of staging buffer in writeTexture (gfx-rs#7963) commit 9a596fa Author: Andy Leiserson <[email protected]> Date: Thu Jul 17 13:08:06 2025 -0700 Fix warnings from the nightly Rust compiler (gfx-rs#7964) commit ff0de91 Author: Connor Fitzgerald <[email protected]> Date: Thu Jul 17 13:00:40 2025 -0400 Bump REPO_MSRV to 1.88 (gfx-rs#7960) commit 33b9f86 Author: Teodor Tanasoaia <[email protected]> Date: Thu Jul 17 18:47:59 2025 +0200 Fix detection of `local_invocation_id` for zero initialization of workgroup memory (gfx-rs#7962) commit 76dba55 Author: Erich Gubler <[email protected]> Date: Wed Jul 16 17:16:29 2025 -0400 style(vulkan): no trailing space after raytrace cap. negotiation (gfx-rs#7954) commit 49b773f Author: Connor Fitzgerald <[email protected]> Date: Fri Jul 11 00:48:44 2025 -0400 Add some documentation to wgpu-hal types commit 1ed6187 Author: Connor Fitzgerald <[email protected]> Date: Fri Jul 11 00:26:02 2025 -0400 Remove wgpu_core::hal_api::HalApi commit 0dbe5fb Author: Connor Fitzgerald <[email protected]> Date: Thu Jul 10 23:56:07 2025 -0400 Add annotations for the types hal types return commit 81b3e7c Author: Connor Fitzgerald <[email protected]> Date: Thu Jul 10 22:33:46 2025 -0400 Rearrange methods on Instance commit e5b10e0 Author: Connor Fitzgerald <[email protected]> Date: Thu Jul 10 22:14:36 2025 -0400 Adjust InstanceDescriptor docs commit f7b672f Author: Connor Fitzgerald <[email protected]> Date: Thu Jul 10 22:14:36 2025 -0400 Update release checklist commit f83af61 Author: Connor Fitzgerald <[email protected]> Date: Thu Jul 10 21:59:35 2025 -0400 Clarify `naga` psuedo-feature in wgpu commit 15f7c7e Author: Connor Fitzgerald <[email protected]> Date: Thu Jul 10 21:48:50 2025 -0400 Remove unneeded comment commit 13d0cd4 Author: Lucas Abel <[email protected]> Date: Wed Jul 16 18:49:03 2025 +0200 [core] fix invalid read in `Queue::write_texture` (gfx-rs#7893) commit 1da2177 Author: Andy Leiserson <[email protected]> Date: Tue Jul 15 10:06:11 2025 -0700 Add skip conditions to CTS runner (gfx-rs#7949) commit dd50e56 Author: SupaMaggie70Incorporated <[email protected]> Date: Mon Jul 14 21:51:31 2025 -0700 Update rspirv version (gfx-rs#7945) commit 4572e05 Author: Connor Fitzgerald <[email protected]> Date: Mon Jul 14 12:48:49 2025 -0400 ci: add coverage for cts (gfx-rs#7942) commit 9872df5 Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Mon Jul 14 02:50:23 2025 -0400 Update cts digest to 6e9d87b (gfx-rs#7941) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit c51fe57 Author: Jim Blandy <[email protected]> Date: Sat Jul 12 22:04:19 2025 -0700 Improve docs for `wgpu_types::InstanceFlags::VALIDATION`. (gfx-rs#7939) commit 1b4eca9 Author: Andy Leiserson <[email protected]> Date: Tue Mar 25 19:09:43 2025 -0700 [naga hlsl-out] Factor out some repetitive code commit bfa7ee8 Author: Andy Leiserson <[email protected]> Date: Sat Mar 22 13:01:11 2025 -0700 [naga hlsl-out] Handle additional cases of Cx2 matrices Fixes gfx-rs#4423 commit c868142 Author: Andy Leiserson <[email protected]> Date: Fri Jul 11 14:43:03 2025 -0700 Validate vertex and index buffer alignment (gfx-rs#7929) commit ae946db Author: Andy Leiserson <[email protected]> Date: Fri Jul 11 13:20:13 2025 -0700 Use backwards-compatible options to `git config` (gfx-rs#7934) commit 5b2c840 Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Thu Jul 10 18:57:33 2025 -0400 chore(deps): update cts digest to 18fcd81 (gfx-rs#7923) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit 085eaa6 Author: Connor Fitzgerald <[email protected]> Date: Thu Jul 10 18:42:49 2025 -0400 Forward port v26.0.1 changelog (gfx-rs#7921) commit 4844fa6 Author: Vecvec <[email protected]> Date: Fri Jul 11 07:53:32 2025 +1200 Merge acceleration structure feature and ray query feature. (gfx-rs#7913) Co-authored-by: Connor Fitzgerald <[email protected]> commit 166c2ea Author: Erich Gubler <[email protected]> Date: Thu Jul 10 15:04:37 2025 -0400 fix(core): check query set index before other validation (gfx-rs#7908) commit b83c9cf Author: Kevin Reid <[email protected]> Date: Thu Jul 10 10:06:36 2025 -0700 Fix `initialize_adapter_from_env` under no_std. Without this change, `desired_adapter_name` is an undeclared variable. commit c0a8ba6 Author: Kevin Reid <[email protected]> Date: Thu Jul 10 09:46:37 2025 -0700 [ci] Check with profiling enabled. Minimum version of `profiling` increases from ^1.0.0 to ^1.0.1 because the `type-check` feature was added in 1.0.1. commit da6ccd5 Author: Kevin Reid <[email protected]> Date: Thu Jul 10 09:39:06 2025 -0700 Fix undeclared `base` in profiling scopes. This would cause compilation to fail if profiling was enabled. commit dc924bc Author: Andy Leiserson <[email protected]> Date: Wed Jul 9 16:56:49 2025 -0700 Remove `unsafe` from `hal::BufferBinding::new_unchecked` commit 9b966bf Author: Andy Leiserson <[email protected]> Date: Wed Jul 9 09:56:57 2025 -0700 Fix for destroyed resource errors commit 468632f Author: Andy Leiserson <[email protected]> Date: Wed Jul 9 11:31:06 2025 -0700 Restore plumbing of implicit remainder-of-buffer size to backends commit 00406a7 Author: Andy Leiserson <[email protected]> Date: Wed Jul 9 13:55:37 2025 -0700 Fix calculation error commit 2db1d71 Author: Andy Leiserson <[email protected]> Date: Wed Jul 9 16:28:51 2025 -0700 [deno] Implement nullable vertex buffer layouts commit 27e7408 Author: Andy Leiserson <[email protected]> Date: Wed Jul 9 13:17:07 2025 -0700 Restore the buffer size changes
Description
Queue::write_texture
might read to uninitialized memory and SEGV.This commit both add a
debug_assert
to prevent it happen again, and fix the issue by copying bybytes_in_last_row
instead ofstage_bytes_per_row.min(bytes_per_row)
.Testing
Manual testing.
Checklist
cargo fmt
.RunNo change to toml filestaplo format
.cargo clippy --tests
. If applicable, add:--target wasm32-unknown-unknown
cargo xtask test
to run tests.If this contains user-facing changes, add aCHANGELOG.md
entry.