chyh1990 / yaml-rust

A pure rust YAML implementation.
Apache License 2.0
605 stars 140 forks source link

Valgrind complains about uninitialized values #161

Closed xTibor closed 4 years ago

xTibor commented 4 years ago

I noticed while debugging some Rust apps this crate was throwing uninitialized value warnings in valgrind. I don't know if this is real UB or bogus warnings, just reporting it to be safe.

Rust version: rustc 1.43.1 (8d69840ab 2020-05-04)

Test case: https://gist.github.com/xTibor/8858ecf5cbd276bc8673267ee5d324ac

Valgrind logs:

==11197== Conditional jump or move depends on uninitialised value(s)
==11197==    at 0x1252C3: yaml_rust::parser::Parser<T>::parser_process_directives (in /home/user/git/yaml-rust/target/release/examples/overrides)
==11197==    by 0x1250F3: yaml_rust::parser::Parser<T>::document_start (in /home/user/git/yaml-rust/target/release/examples/overrides)
==11197==    by 0x122112: yaml_rust::parser::Parser<T>::next (in /home/user/git/yaml-rust/target/release/examples/overrides)
==11197==    by 0x12360A: yaml_rust::parser::Parser<T>::load (in /home/user/git/yaml-rust/target/release/examples/overrides)
==11197==    by 0x11E247: yaml_rust::yaml::YamlLoader::load_from_str (in /home/user/git/yaml-rust/target/release/examples/overrides)
==11197==    by 0x10D892: overrides::main (in /home/user/git/yaml-rust/target/release/examples/overrides)
==11197==    by 0x10D4B2: std::rt::lang_start::{{closure}} (in /home/user/git/yaml-rust/target/release/examples/overrides)
==11197==    by 0x1362B2: {{closure}} (rt.rs:52)
==11197==    by 0x1362B2: std::panicking::try::do_call (panicking.rs:303)
==11197==    by 0x137FF6: __rust_maybe_catch_panic (lib.rs:86)
==11197==    by 0x136CBB: try<i32,closure-0> (panicking.rs:281)
==11197==    by 0x136CBB: catch_unwind<closure-0,i32> (panic.rs:394)
==11197==    by 0x136CBB: std::rt::lang_start_internal (rt.rs:51)
==11197==    by 0x10DA31: main (in /home/user/git/yaml-rust/target/release/examples/overrides)
==11197==  Uninitialised value was created by a stack allocation
==11197==    at 0x125250: yaml_rust::parser::Parser<T>::parser_process_directives (in /home/user/git/yaml-rust/target/release/examples/overrides)
dtolnay commented 4 years ago

This crate contains zero unsafe code so the root cause is almost surely not in this crate.

hoodie commented 4 years ago

linked hashmap does though https://github.com/contain-rs/linked-hash-map/blob/4f681be6f3b1b2c7d67647ce3abbe48470e20855/src/lib.rs#L73

dtolnay commented 4 years ago

This reproduces without linked-hash-map though, so it looks like an LLVM or rustc bug. Reduced somewhat:

enum Token {
    DocumentStart,
    #[allow(dead_code)]
    Tag(String, String),
}

fn main() {
    let _ = peek_token(&mut None);
    if process_directives(&mut None).is_err() {
        let _ = process_directives(&mut None);
    }
    skip(&mut None);
}

fn peek_token(token: &mut Option<Token>) -> Result<&Token, String> {
    *token = Some(*Box::new(Token::DocumentStart));
    Ok(token.as_ref().unwrap())
}

fn skip(token: &mut Option<Token>) {
    *token = None;
}

fn process_directives(token: &mut Option<Token>) -> Result<(), String> {
    loop {
        match peek_token(token)? {
            Token::Tag(..) => {}
            _ => break,
        }
        skip(token);
    }
    Ok(())
}
$ cargo build --release && valgrind target/release/testing 
   Compiling testing v0.0.0
    Finished release [optimized] target(s) in 0.25s
==12801== Memcheck, a memory error detector
==12801== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==12801== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==12801== Command: target/release/testing
==12801== 
==12801== Conditional jump or move depends on uninitialised value(s)
==12801==    at 0x10D8E3: testing::parser_process_directives (in /git/testing/target/release/testing)
==12801==    by 0x10D3B6: testing::main (in /git/testing/target/release/testing)
==12801==    by 0x10DAC2: std::rt::lang_start::{{closure}} (in /git/testing/target/release/testing)
==12801==    by 0x115887: {{closure}} (rt.rs:52)
==12801==    by 0x115887: do_call<closure-0,i32> (panicking.rs:297)
==12801==    by 0x115887: try<i32,closure-0> (panicking.rs:274)
==12801==    by 0x115887: catch_unwind<closure-0,i32> (panic.rs:394)
==12801==    by 0x115887: std::rt::lang_start_internal (rt.rs:51)
==12801==    by 0x10D971: main (in /git/testing/target/release/testing)
==12801== 
==12801== 
==12801== HEAP SUMMARY:
==12801==     in use at exit: 0 bytes in 0 blocks
==12801==   total heap usage: 12 allocs, 12 frees, 2,561 bytes allocated
==12801== 
==12801== All heap blocks were freed -- no leaks are possible
==12801== 
==12801== Use --track-origins=yes to see where uninitialised values come from
==12801== For lists of detected and suppressed errors, rerun with: -s
==12801== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
dtolnay commented 4 years ago

According to https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/sanitizer.html#memorysanitizer you need to build with -Zbuild-std in order for initializations inside the standard library to be understood by MemorySanitizer, otherwise you get false positives. That looks like what this is because building with -Zbuid-std does not report any uninitialized accesses.

$ cargo clean
$ export RUSTFLAGS='-Zsanitizer=memory -Zsanitizer-memory-track-origins'
$ cargo run -Zbuild-std --target x86_64-unknown-linux-gnu --release