paholg / typenum

Compile time numbers in Rust.
https://crates.io/crates/typenum
Other
513 stars 45 forks source link

Can unsafe code assume sane implementation of `Unsigned`? #173

Closed Qwaz closed 3 years ago

Qwaz commented 3 years ago

The documentation of Unsigned and other typenum marker traits says "This trait should not be implemented for anything outside this crate." However, the definition still allows custom implementation of them. This could affect the soundness of dependent crates if methods like Unsgined::to_usize() are used in unsafe context.

For a better context, here is a proof-of-concept code that triggers buffer overflow with flatk. Would this be considered a soundness issue in flatk, such that it shouldn't use Unsigned in unsafe context? Or, should the marker traits in typenum need to be updated to unsafe traits or sealed traits that actually prevent inconsistent downstream implementation?

#![forbid(unsafe_code)]
use std::sync::atomic::{AtomicU8, Ordering};

use bytemuck::Pod;
use flatk::{Array, SplitPrefix};
use typenum::Unsigned;

static COUNTER: AtomicU8 = AtomicU8::new(0);

#[derive(Default, Clone, Copy)]
struct FakeNum;

impl Unsigned for FakeNum {
    const U8: u8 = 0;
    const U16: u16 = 0;
    const U32: u32 = 0;
    const U64: u64 = 0;
    const USIZE: usize = 0;
    const I8: i8 = 0;
    const I16: i16 = 0;
    const I32: i32 = 0;
    const I64: i64 = 0;
    const ISIZE: isize = 0;

    fn to_u8() -> u8 {
        todo!()
    }

    fn to_u16() -> u16 {
        todo!()
    }

    fn to_u32() -> u32 {
        todo!()
    }

    fn to_u64() -> u64 {
        todo!()
    }

    fn to_usize() -> usize {
        if COUNTER.fetch_add(1, Ordering::SeqCst) % 4 == 3 {
            100000
        } else {
            0
        }
    }

    fn to_i8() -> i8 {
        todo!()
    }

    fn to_i16() -> i16 {
        todo!()
    }

    fn to_i32() -> i32 {
        todo!()
    }

    fn to_i64() -> i64 {
        todo!()
    }

    fn to_isize() -> isize {
        todo!()
    }
}

impl<T: Pod> Array<T> for FakeNum {
    type Array = [T; 1];

    fn iter_mut(_array: &mut Self::Array) -> std::slice::IterMut<T> {
        todo!()
    }

    fn iter(_array: &Self::Array) -> std::slice::Iter<T> {
        todo!()
    }

    fn as_slice(_array: &Self::Array) -> &[T] {
        todo!()
    }
}

fn main() {
    let vec = vec![1, 2, 3];
    // Copies 100000 elements unsafely to [T; 1]
    SplitPrefix::<FakeNum>::split_prefix(vec);
}
paholg commented 3 years ago

Or, should the marker traits in typenum need to be updated to unsafe traits or sealed traits that actually prevent inconsistent downstream implementation?

This. I would welcome a PR to update them to sealed traits and would not consider it a breaking change as the documentation has been clear on this point since before 1.0. I just was not familiar with that pattern at the time.

Edit: It was quick and easy, so I made the PR myself.

Qwaz commented 3 years ago

Thanks for the prompt fix!