Skip to content

Possible unsound API usages #4

@charlesxsh

Description

@charlesxsh

Hi there,

At version 0.2.5, src/mapper.rs

line 585

pub fn new(cart: C) -> Self {
        let prg_nbank = cart.get_size(BankType::PrgRom) >> 13;
        let chr_nbank = cart.get_size(BankType::ChrRom) >> 10;
        unsafe {
            let mut m = Mapper4 {
                cart,
                prg_nbank,
                chr_nbank,
                prg_mode: 0,
                chr_inv: 0,
                reg_idx: 0,
                regs: [0; 8],
                prg_banks: MaybeUninit::uninit().assume_init(),
                chr_banks: MaybeUninit::uninit().assume_init(),
                sram: core::slice::from_raw_parts_mut(core::ptr::null_mut(), 0),
                irq_reload: 0,
                irq_counter: 0,
                irq_enable: false,
            };

line 336

 pub fn new(cart: C) -> Self {
        let nbank = cart.get_size(BankType::PrgRom) >> 14;
        unsafe {
            let mut m = Mapper2 {
                cart,
                prg_nbank: nbank,
                prg_banks: MaybeUninit::uninit().assume_init(),
                chr_bank: core::slice::from_raw_parts_mut(
                    core::ptr::null_mut(),
                    0,
                ),
                sram: core::slice::from_raw_parts_mut(core::ptr::null_mut(), 0),
            };
            let c = &mut m.cart;
            m.prg_banks = [
                c.get_bank(0, 0x4000, BankType::PrgRom),
                c.get_bank((nbank - 1) << 14, 0x4000, BankType::PrgRom),
            ];
            m.chr_bank = c.get_bank_mut(0, 0x2000, BankType::ChrRom);
            m.sram = c.get_bank_mut(0, 0x2000, BankType::Sram);
            m
        }
    }

line 93

    pub fn new(cart: C) -> Self {
        let prg_nbank = cart.get_size(BankType::PrgRom) >> 14;
        let chr_nbank = cart.get_size(BankType::ChrRom) >> 13;
        unsafe {
            let mut m = Mapper1 {
                cart,
                prg_nbank,
                chr_nbank,
                load_reg: 0x10,
                ctl_reg: 0x0c,
                prg_banks: MaybeUninit::uninit().assume_init(),
                chr_banks: MaybeUninit::uninit().assume_init(),
                sram: core::slice::from_raw_parts_mut(core::ptr::null_mut(), 0),
            };
            let c = &mut m.cart;
            m.prg_banks = [
                c.get_bank(0, 0x4000, BankType::PrgRom),
                c.get_bank((prg_nbank - 1) << 14, 0x4000, BankType::PrgRom),
            ];
            m.chr_banks = [
                c.get_bank_mut(0, 0x1000, BankType::ChrRom),
                c.get_bank_mut(0x1000, 0x1000, BankType::ChrRom),
            ];
            m.sram = c.get_bank_mut(0, 0x2000, BankType::Sram);
            m
        }
    }

The usage of assume_init and from_raw_parts_mut violate the safety requirements mentioned in https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#method.assume_init and https://doc.rust-lang.org/std/slice/fn.from_raw_parts_mut.html.

Quote an example of incorrect usage of assume_init here:
"
Moreover, uninitialized memory is special in that it does not have a fixed value (“fixed” meaning “it won’t change without being written to”). Reading the same uninitialized byte multiple times can give different results. This makes it undefined behavior to have uninitialized data in a variable even if that variable has an integer type, which otherwise can hold any fixed bit pattern:

use std::mem::{self, MaybeUninit};

let x: i32 = unsafe { mem::uninitialized() }; // undefined behavior! ⚠️
// The equivalent code with `MaybeUninit<i32>`:
let x: i32 = unsafe { MaybeUninit::uninit().assume_init() }; // undefined behavior! ⚠️

"

For the from_raw_parts_mut, the safety requirements explicitly says that the data(first parameter) must be not-null.

Suggestions:

  1. keep uninitialized until proper initialization is done (ex. write())
  2. Use optional to represent null instead of using core::slice::from_raw_parts_mut(core::ptr::null_mut(), 0),

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions