Skip to content

fix UB in minifb example #191

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

Merged
merged 1 commit into from
Sep 28, 2020
Merged

fix UB in minifb example #191

merged 1 commit into from
Sep 28, 2020

Conversation

TheEdward162
Copy link
Contributor

@TheEdward162 TheEdward162 commented Sep 27, 2020

In the minifb example a Vec<u8> was allocated and then used as bitmap backend target. Later this same buffer was cast to &[u32] using this code:

unsafe {
    std::slice::from_raw_parts(&buf[0] as *const _ as *const _, H * W)
}

This is incorrect because of two things:

  • &buf[0] semantically borrows the first element and then creates a pointer from the borrow. This is ok in partice but in the abstract machine it creates an access past the borrowed memory of the first element and it might break in the future.
  • The alignment of u8 is 1 while the alignment of u32 is 4. This is not a huge problem on x86, but would probably crash on ARM. Run this playground in Miri to see it panics: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=d7e15efe2d8bc611692d6350919e7943

The first problem can be solved by using the std functions vec.as_ptr() and vec.as_mut_ptr() which are guaranteed to work correctly because it's std.

The second problem can be solved by using Vec<u32> and casting it to &mut [u8], since having higher alignment than required is ok.

minifb example was incorrectly casting Vec<u8> to &[u32]
@codecov
Copy link

codecov bot commented Sep 27, 2020

Codecov Report

Merging #191 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #191   +/-   ##
=======================================
  Coverage   68.83%   68.83%           
=======================================
  Files          56       56           
  Lines        4313     4313           
=======================================
  Hits         2969     2969           
  Misses       1344     1344           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6da6db4...91d8a99. Read the comment docs.

@38
Copy link
Member

38 commented Sep 28, 2020

Hi thanks for the PR and the explain! This is very helpful and I don't realize this is an issue until you point this out. Actually I tried this demo on ARM it actually works - I guess this is probably due to the behavior of memory allocator, but you are right !

Really appreciated for your help!

@38 38 merged commit 96d5184 into plotters-rs:master Sep 28, 2020
@TheEdward162 TheEdward162 deleted the fix-minifb-example-ub branch September 29, 2020 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants