japaric / ufmt

a smaller, faster and panic-free alternative to core::fmt
Apache License 2.0
352 stars 40 forks source link

Undefined behavior in provided method `uWrite::write_char` #30

Open jrvanwhy opened 3 years ago

jrvanwhy commented 3 years ago

uWrite::write_char contains an unsound use of core::mem::uninitialized (see the docs for std::mem::MaybeUninit):

    fn write_char(&mut self, c: char) -> Result<(), Self::Error> {
        let mut buf: [u8; 4] = unsafe { uninitialized() };
        self.write_str(c.encode_utf8(&mut buf))
    }

The usual fix is to use std::mem::MaybeUninit, but this crate has a minimum supported Rust version of 1.34, which predates the stabilization of std::mem::MaybeUninit in 1.36.

There are a few possible fixes:

  1. Zero-initialize the array, either accepting extra generated code size or hoping the compiler optimizes it away.
  2. Increase the minimum supported Rust version to 1.36 and use std::mem::MaybeUninit.
  3. Re-implement std::mem::MaybeUninit in this crate (i.e. use a union to make it sound), which involves significantly more unsafe code.
jrvanwhy commented 3 years ago

Actually, looking again, encode_utf8 takes a &mut [u8], so it requires its input to be initialized. Therefore I believe solutions 2 and 3 from my post are unsound.

An additional option to avoid unnecessary overhead is to implement our own char-to-utf8 logic.

jrvanwhy commented 3 years ago

Actually, grepping through the source shows that uninitialized is used in several more places, which complicates the fix.