MichaelBurge/nes-emulator

UB while loading Vec<bool>

Closed this issue · 1 comments

kpp commented

let mut x:T = unsafe { std::mem::uninitialized() };

impl<T: Savable> Savable for Vec<T> {
...
    fn load(&mut self, fh: &mut Read) {
        let mut len = 0usize;
        len.load(fh);
        self.truncate(0);
        self.reserve(len);
        for i in 0..len {
            let mut x:T = unsafe { std::mem::uninitialized() };
            x.load(fh);
            self.push(x);
        }
}

In case of Vec<bool> we got:

let mut x:bool = unsafe { std::mem::uninitialized() };

Which is UB by definition: https://doc.rust-lang.org/reference/behavior-considered-undefined.html

See comment rust-lang/rust#53491 (comment)

Thanks for catching this. I've pushed a commit

f7cfcb9

that uses a Default trait instead. The only T that is instantiated is u8 - used for the video recording feature - which already implements Default.

I notice that your NewSavable trait creates new objects rather than replacing the existing ones. Since my emulator currently uses pointers in places like CpuPpuInterconnect

pub struct CpuPpuInterconnect {

it would actually be dangerous to replace all of the save/load functionality to return new objects because these pointers wouldn't be updated to point to them. I think you only changed the input recording feature, which is safe, but I'd prefer not to have two different serialization traits.