Repository navigation
Fix scaler memory safety, mouse clamp panic and Key transmute - #431
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The scaler test guard is undersized for its cases, and one corrected overflow path remains untested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes memory-safety and correctness issues in shared input and POSIX scaling code.
Changes:
- Hardens C scalers against overflow, invalid dimensions, and out-of-bounds writes.
- Safely handles degenerate mouse dimensions and key conversions.
- Adds rebuild directives and regression tests.
File summaries
| File | Description |
|---|---|
build.rs |
Rebuilds changed native sources. |
src/key.rs |
Fixes Key representation. |
src/key_handler.rs |
Bounds key storage and conversions. |
src/lib.rs |
Prevents mouse-clamp panic. |
src/native/posix/scalar.c |
Fixes scaler arithmetic and bounds. |
src/os/posix/common.rs |
Adds scaler regression tests. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+205
to
+207
| #[test] | ||
| fn resize_linear_handles_large_source_dimensions() { | ||
| let width = 2_200_000usize; |
|
|
||
| let src: Vec<u32> = (0..src_width * src_height).map(|i| i as u32 | 1).collect(); | ||
| let visible = dst_width * dst_height; | ||
| let mut dst = vec![GUARD_VALUE; visible + GUARD]; |
image_upper_left looped over src_height - (src_height - dst_height) / 2 rows into a dst_height buffer, writing past its end. Reachable from safe code via ScaleMode::UpperLeft. Rewritten as a min(src, dst) row/column copy, which is what upper-left anchoring means anyway. Fixed-point coordinates in image_resize_linear and its stride variant accumulated in a 32-bit int, so a source dimension above 2^21 wrapped negative and read out of bounds. A 2.2M x 1 buffer is 8.8 MB and passes check_buffer_size. Now int64_t, with each coordinate clamped to the source extent. The aspect-fill offsets were computed as (a - b) / -2 in unsigned arithmetic, where -2 converts to 0xfffffffe, so the offset was always 0 or 1 and the image rendered top-left aligned instead of centered. f32::clamp asserts min <= max, so get_mouse_pos in Clamp mode aborted when the scale-divided window size fell below one pixel. Key had no repr, leaving the discriminant layout unspecified while key_handler transmuted u8 into it over a 512-entry range with a wrapping cast. Key is now repr(u8), the transmutes go through a bounds-checked key_from_index, and the state arrays are sized to the enum. cc emits no cargo:rerun-if-changed for the files it compiles, so cargo kept stale objects when only the native sources changed. Without this the scaler tests silently ran against the previous build. Closes #406 Closes #407 Closes #422 Closes #424
emoon
force-pushed
the
fix/audit-memory-safety
branch
from
August 29, 2026 10:35
033e6ef to
bd0a4f1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four audit findings, each reproduced locally before the fix and covered by a regression test.
Closes #406
Closes #407
Closes #422
Closes #424
Changes
image_upper_leftlooped oversrc_height - (src_height - dst_height) / 2rows into adst_heightbuffer. Heap out-of-bounds write, reachable from safe code viaScaleMode::UpperLeft. Confirmed under ASan and as a glibc abort in a live Wayland window.intwrapped negative for source dimensions above 2^21 (a 2.2M x 1 buffer passescheck_buffer_size), and the aspect-fill offsets were computed as(a - b) / -2in unsigned arithmetic, so the image rendered top-left aligned instead of centered.f32::clampassertsmin <= max, so a scale-divided window size below one pixel aborted.Keyhad norepr, so thetransmute::<u8, Key>inkey_handlerrelied on unspecified layout.Also fixes a build-script bug found while verifying:
ccemits nocargo:rerun-if-changed, so cargo kept stale objects when only the C sources chan 8000 ged. Without it the scaler tests silently ran against the previous build.Verification
cargo teston Linux/Wayland, plus an x11-only build. The four scaler and key tests fail against the pre-fixscalar.cand pass after.Note: #355 (the reported
AspectRatioStretchsegfault) was already fixed by d62b0f5 and can be closed separately.