near / borsh-rs

Rust implementation of Binary Object Representation Serializer for Hashing
https://borsh.io/
Apache License 2.0
310 stars 67 forks source link

UB in BorshSerialize with padding or uninitialized data #17

Closed dtolnay closed 3 years ago

dtolnay commented 3 years ago

The following safe code exhibits UB by reading and dumping out uninitialized memory.

use borsh::BorshSerialize;
use std::io::Result;
use std::mem::MaybeUninit;

#[derive(Copy, Clone)]
struct B(MaybeUninit<u8>);

impl BorshSerialize for B {
    fn serialize<W>(&self, _writer: &mut W) -> Result<()> {
        unreachable!()
    }
    fn is_u8() -> bool {
        true
    }
}

fn main() {
    let x = [B(MaybeUninit::uninit()); 1024];
    println!("{:?}", String::from_utf8_lossy(&x.try_to_vec().unwrap()));
}
$ cargo run --release
"\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}00 103:02
 75240665                  /usr/lib/x86_64-linux-gnu/libgcc_s.so.1\n7f03f10ad000-7f03f10af
000 rw-p 00000000 00:00 0 \n7f03f10af000-7f03f10b0000 r--p 00000000 103:02 75236957       
           /usr/lib/x86_64-linux-gnu/ld-2.31.so\n7f03f10b0000-7f03f10d3000 r-xp 00001000 1
03:02 75236957                  /usr/lib/x86_64-linux-gnu/ld-2.31.so\n7f03f10d3000-7f03f10
db000 r--p 00024000 103:02 75236957                  /usr/lib/x86_64-linux-gnu/ld-2.31.so\
n7f03f10dc000-7f03f10dd000 r--p 0002c000 103:02 75236957                  /usr/lib/x86_64-
linux-gnu/ld-2.31.so\n7f03f10dd000-7f03f10de000 rw-p 0002d000 103:02 75236957             
     /usr/lib/x86_64-linux-gnu/ld-2.31.so\n7f03f10de000-7f03f10df000 rw-p 00000000 00:00 0
 \n7ffeb4418000-7ffeb443a000 rw-p 00000000 00:00 0                          [stack]\n7ffeb
4450000-7ffeb4453000 r--p 00000000 00:00 0                          [vvar]\n7ffeb4453000-7
ffeb4454000 r-xp 00000000 00:00 0                          [vdso]\nffffffffff600000-ffffff
ffff601000 --xp 0000"
frol commented 3 years ago

@evgenykuzyakov Seems to be caused by the same fact of poor communication around safety of is_u8 as in https://github.com/near/borsh-rs/issues/18#issuecomment-799599593

evgenykuzyakov commented 3 years ago

Yeah, reimplementing is_u8 has to be marked unsafe.

evgenykuzyakov commented 3 years ago

Ideally we would not even export it, but I'm not sure how to implement it. Basically, is_u8 only for u8 type, but Rust is not very flexible.

dtolnay commented 3 years ago

I think that fix is still wrong. :frowning_face: I filed https://github.com/near/borsh-rs/issues/24 to follow up. A function/method marked unsafe exclusively means that it is unsafe to call, not unsafe to implement.