Skip to content

Commit

Permalink
Use Plain trait for ComponentBytes soundness
Browse files Browse the repository at this point in the history
  • Loading branch information
kornelski committed Jun 14, 2020
1 parent ffa7935 commit 2691083
Show file tree
Hide file tree
Showing 6 changed files with 41 additions and 5 deletions.
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ travis-ci = { repository = "kornelski/rust-rgb" }

[dependencies]
serde = { version = "1.0", optional = true, features = ["derive"] }
plain = "0.2.3"

[dev-dependencies]
serde_json = "1.0"
Expand Down
8 changes: 6 additions & 2 deletions src/alt.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use plain::Plain;
use crate::internal::pixel::*;
use core::mem;
use core::ops;
Expand Down Expand Up @@ -113,6 +114,9 @@ pub struct GrayAlpha<ComponentType, AlphaComponentType = ComponentType>(
pub AlphaComponentType,
);

unsafe impl<T> Plain for Gray<T> where T: Plain {}
unsafe impl<T, A> Plain for GrayAlpha<T, A> where T: Plain, A: Plain {}

/// 8-bit gray
pub type GRAY8 = Gray<u8>;

Expand Down Expand Up @@ -219,7 +223,7 @@ impl<T> ComponentSlice<T> for [GrayAlpha<T>] {
}
}

impl<T: Copy + Send + Sync + 'static> ComponentBytes<T> for [GrayAlpha<T>] {}
impl<T: Plain> ComponentBytes<T> for [GrayAlpha<T>] {}

impl<T> ComponentSlice<T> for Gray<T> {
#[inline(always)]
Expand Down Expand Up @@ -252,7 +256,7 @@ impl<T> ComponentSlice<T> for [Gray<T>] {
}
}

impl<T: Copy + Send + Sync + 'static> ComponentBytes<T> for [Gray<T>] {}
impl<T: Plain> ComponentBytes<T> for [Gray<T>] {}

/// Assumes 255 is opaque
impl<T: Copy> From<Gray<T>> for GrayAlpha<T, u8> {
Expand Down
13 changes: 12 additions & 1 deletion src/internal/pixel.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use plain::Plain;
use core;

/// Casting the struct to slices of its components
Expand All @@ -14,7 +15,17 @@ pub trait ComponentSlice<T> {
}

/// Casting a slice of `RGB/A` values to a slice of `u8`
pub trait ComponentBytes<T: Copy + Send + Sync + 'static> where Self: ComponentSlice<T> {
///
/// If instead of `RGB8` you use `RGB<MyCustomType>`, and you want to cast from/to that custom type,
/// implement the `Plain` trait for it:
///
/// ```rust
/// # struct MyCustomType;
/// unsafe impl rgb::plain::Plain for MyCustomType {}
/// ```
///
/// Plain types are not allowed to contain struct padding, booleans, chars, enums, references or pointers.
pub trait ComponentBytes<T: Plain> where Self: ComponentSlice<T> {
/// The components interpreted as raw bytes, in machine's native endian. In `RGB` bytes of the red component are first.
#[inline]
fn as_bytes(&self) -> &[u8] {
Expand Down
6 changes: 5 additions & 1 deletion src/internal/rgb.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use plain::Plain;
use super::pixel::*;
use crate::alt::BGR;
use crate::alt::BGRA;
Expand Down Expand Up @@ -25,6 +26,9 @@ impl<T> BGR<T> {
}
}

unsafe impl<T> Plain for RGB<T> where T: Plain {}
unsafe impl<T> Plain for BGR<T> where T: Plain {}

macro_rules! impl_rgb {
($RGB:ident, $RGBA:ident) => {
impl<T: Clone> $RGB<T> {
Expand Down Expand Up @@ -101,7 +105,7 @@ macro_rules! impl_rgb {
}
}

impl<T: Copy + Send + Sync + 'static> ComponentBytes<T> for [$RGB<T>] {}
impl<T: Plain> ComponentBytes<T> for [$RGB<T>] {}
}
}

Expand Down
12 changes: 11 additions & 1 deletion src/internal/rgba.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ impl<T, A> BGRA<T,A> {
}
}

unsafe impl<T, A> plain::Plain for RGBA<T, A> where T: plain::Plain, A: plain::Plain {}
unsafe impl<T, A> plain::Plain for BGRA<T, A> where T: plain::Plain, A: plain::Plain {}

#[cfg(feature = "argb")]
impl<T> ARGB<T> {
#[inline(always)]
Expand Down Expand Up @@ -87,6 +90,11 @@ impl<T, A> ABGR<T,A> {
}
}

#[cfg(feature = "argb")]
unsafe impl<T, A> plain::Plain for ARGB<T, A> where T: plain::Plain, A: plain::Plain {}
#[cfg(feature = "argb")]
unsafe impl<T, A> plain::Plain for ABGR<T, A> where T: plain::Plain, A: plain::Plain {}

macro_rules! impl_rgba {
($RGBA:ident) => {
impl<T: Clone> $RGBA<T> {
Expand Down Expand Up @@ -188,7 +196,7 @@ macro_rules! impl_rgba {
}
}

impl<T: Copy + Send + Sync + 'static> ComponentBytes<T> for [$RGBA<T>] {}
impl<T: plain::Plain> ComponentBytes<T> for [$RGBA<T>] {}
}
}

Expand Down Expand Up @@ -364,6 +372,8 @@ fn abgr_test() {
#[allow(deprecated)]
fn bgra_test() {
let neg = BGRA::new(1, 2, 3i32, 1000).map(|x| -x);
let _ = neg.as_slice();
let _ = [neg].as_bytes();
assert_eq!(neg.r, -1);
assert_eq!(neg.bgr().r, -1);
assert_eq!(neg.g, -2);
Expand Down
6 changes: 6 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ mod internal {
/// BGR might be useful for some Windows or OpenGL APIs.
pub mod alt;

/// Re-export from `plain` crate
pub mod plain {
// wrapped in mod to prevent `use rgb::*` picking it
pub use plain::Plain;
}

pub use crate::internal::convert::*;
pub use crate::internal::ops::*;
pub use crate::internal::pixel::*;
Expand Down

12 comments on commit 2691083

@Shnatsel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do in fact have a whole lot of competing traits for Plain 🤓

https://crates.io/crates/bytemuck has its own Pod trait, this is what's already used in image. Don't think it has a derive macro but then again Plain doesn't either. https://crates.io/crates/zerocopy also has a similar trait and it does have a derive macro, but it's a heavier dependency.

@danielhenrymantilla has figured out a way to implement the derive with a macro-by-example so that compile time would not be inflated by syn, but I'm not aware of any crate incorporating that yet.

@Shnatsel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An effort to unify the traits is underway but is not in a usable shape yet: https://github.com/rust-secure-code/mem-markers

@HeroicKatora
Copy link

@HeroicKatora HeroicKatora commented on 2691083 Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A significant reason for choosing bytemuck over Plain was its scope: It has willingly committed to a small core of operations in order to have a 1.0 release very early. Since the trait bound is part of the interface, it was very important to choose a stable dependency in image.

@HeroicKatora
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this change is not SemVer compatible to 0.8.18.

@kornelski
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's technically a semver break, but:

  • it's a crate for interoperability, so a major version bump is also very problematic
  • it's for a soundness fix, so it's in the gray area between a break and bugfix
  • in practice it affects only a small very specialized case

@kornelski
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed source of plain and bytemuck, and I prefer plain. It's much smaller, simpler and does exactly that one thing. Bytemuck throws in zeroing, casts, and tries to be clever with marking Option<NonZero<T>> as Pod type. It's not unsafe per Rust's definition, but nonsense for pixel types.

@kornelski
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your comments. They're helpful.

@HeroicKatora
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plain probably didn't add them in because the stabilization of Option<NonZero> layout happened sometime in the 3 years of Rust development that plain appears to be lagging behind. For example:

At this moment, Drop types are also not legal, because compiler adds a special "drop flag" into the type. This is slated to change in the future.

This paragraph from its documentation is similarly outdated. It did change and it was slated.

@kornelski
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shnatsel that crate looks nice. At a first glance the distinction between AsBytes and ByteComplete looks useful for as_bytes and as_bytes_mut respectively.

Do you expect to release at least the "pod" subset of the crate anytime soon?

@Shnatsel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was discussion on releasing a 0.1 on crates.io soon; I believe the presence of a motivating example would accelerate that.

I suggest talking directly to the people responsible for mem-markers. Their discussion is hosted on Rust Zulip in #project-safe-transmute.

@Shnatsel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding semver, I'd prefer my broken code to stop compiling instead of silently staying broken, so I'm in favor of this shipping in a semver-compatible release even though it is an API break.

@kornelski
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#35 fixed in 0.8.20

Please # to comment.