UB with yaml DeSer

[niluxv]

Miri detects Undefined Behaviour when executing integration_tests::mem_yaml: (this used to ICE due to a bug in miri)

Miri log

error: Undefined Behavior: type validation failed: encountered uninitialized bytes at .<enum-tag>, but expected a valid enum tag
   --> ~/.cargo/registry/src/github.com-1ecc6299db9ec823/linked-hash-map-0.5.3/src/lib.rs:114:16
    |
114 |     let Node { key, value, .. } = *Box::from_raw(the_box);
    |                ^^^ type validation failed: encountered uninitialized bytes at .<enum-tag>, but expected a valid enum tag
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
            
    = note: inside `linked_hash_map::drop_empty_node::<yaml_rust::yaml::Yaml, yaml_rust::yaml::Yaml>` at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/linked-hash-map-0.5.3/src/lib.rs:114:16
    = note: inside `<linked_hash_map::LinkedHashMap<yaml_rust::yaml::Yaml, yaml_rust::yaml::Yaml> as std::ops::Drop>::drop` at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/linked-hash-map-0.5.3/src/lib.rs:809:17
    = note: inside `std::intrinsics::drop_in_place::<linked_hash_map::LinkedHashMap<yaml_rust::yaml::Yaml, yaml_rust::yaml::Yaml>> - shim(Some(linked_hash_map::LinkedHashMap<yaml_rust::yaml::Yaml, yaml_rust::yaml::Yaml>))` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ptr/mod.rs:184:1
    = note: inside `std::intrinsics::drop_in_place::<yaml_rust::yaml::Yaml> - shim(Some(yaml_rust::yaml::Yaml))` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ptr/mod.rs:184:1
    = note: inside `serde_yaml::ser::to_writer::<&mut std::vec::Vec<u8>, std::collections::HashMap<u64, std::string::String>>` at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/serde_yaml-0.8.13/src/ser.rs:400:1
    = note: inside `serde_yaml::ser::to_vec::<std::collections::HashMap<u64, std::string::String>>` at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/serde_yaml-0.8.13/src/ser.rs:411:5
    = note: inside `serde_yaml::ser::to_string::<std::collections::HashMap<u64, std::string::String>>` at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/serde_yaml-0.8.13/src/ser.rs:423:26
    = note: inside `<rustbreak::deser::Yaml as rustbreak::DeSerializer<std::collections::HashMap<u64, std::string::String>>>::serialize` at ~/Documents/my_sources/rust/git_project/rustbreak/src/deser.rs:123:16
    = note: inside `rustbreak::Database::<std::collections::HashMap<u64, std::string::String>, rustbreak::backend::MemoryBackend, rustbreak::deser::Yaml>::save_data_locked::<std::sync::RwLockReadGuard<std::collections::HashMap<u64, std::string::String>>>` at ~/Documents/my_sources/rust/git_project/rustbreak/src/lib.rs:548:19
    = note: inside `rustbreak::Database::<std::collections::HashMap<u64, std::string::String>, rustbreak::backend::MemoryBackend, rustbreak::deser::Yaml>::save` at ~/Documents/my_sources/rust/git_project/rustbreak/src/lib.rs:562:9
note: inside `test_basic_save_load::<rustbreak::backend::MemoryBackend, rustbreak::deser::Yaml>` at tests/integration_tests.rs:28:5
   --> tests/integration_tests.rs:28:5
    |
28  |     db.save().expect("error while saving");
    |     ^^^^^^^^^
note: inside `mem_yaml` at tests/integration_tests.rs:148:13
   --> tests/integration_tests.rs:148:13
    |
148 |             test_basic_save_load(&db);
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^
...
165 | test_basic_save_load!(mem_yaml, create_memdb(), Yaml, miri = true);
    | ------------------------------------------------------------------- in this macro invocation
note: inside closure at tests/integration_tests.rs:146:9
   --> tests/integration_tests.rs:146:9
    |
146 | /         fn $name() {
147 | |             let db: Database<Data, _, $enc> = $db;
148 | |             test_basic_save_load(&db);
149 | |             test_put_data(&db);
150 | |             test_multi_borrow(&db);
151 | |             test_convert_data(db);
152 | |         }
    | |_________^
...
165 |   test_basic_save_load!(mem_yaml, create_memdb(), Yaml, miri = true);
    |   ------------------------------------------------------------------- in this macro invocation
    = note: inside `<[closure@tests/integration_tests.rs:146:9: 152:10] as std::ops::FnOnce<()>>::call_once - shim` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ops/function.rs:233:5
    = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ops/function.rs:233:5
    = note: inside `test::__rust_begin_short_backtrace::<fn()>` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:517:5
    = note: inside closure at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:508:30
    = note: inside `<[closure@test::run_test::{{closure}}#2 0:fn()] as std::ops::FnOnce<()>>::call_once - shim(vtable)` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ops/function.rs:233:5
    = note: inside `<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send> as std::ops::FnOnce<()>>::call_once` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/liballoc/boxed.rs:1081:9
    = note: inside `<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>> as std::ops::FnOnce<()>>::call_once` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panic.rs:318:9
    = note: inside `std::panicking::r#try::do_call::<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>, ()>` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:348:40
    = note: inside `std::panicking::r#try::<(), std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>>` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:325:15
    = note: inside `std::panic::catch_unwind::<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>, ()>` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panic.rs:394:14
    = note: inside `test::run_test_in_process` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:541:18
    = note: inside closure at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:450:39
    = note: inside `test::run_test::run_test_inner` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:475:13
    = note: inside `test::run_test` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:505:28
    = note: inside `test::run_tests::<[closure@test::run_tests_console::{{closure}}#2 0:&mut test::console::ConsoleTestState, 1:&mut std::boxed::Box<dyn test::formatters::OutputFormatter>]>` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:284:13
    = note: inside `test::run_tests_console` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/console.rs:280:5
    = note: inside `test::test_main` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:120:15
    = note: inside `test::test_main_static` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:139:5
    = note: inside `main`
    = note: inside closure at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:67:34
    = note: inside closure at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:52:73
    = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<[closure@std::rt::lang_start_internal::{{closure}}#0::{{closure}}#0 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/sys_common/backtrace.rs:130:5
    = note: inside closure at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:52:13
    = note: inside `std::panicking::r#try::do_call::<[closure@std::rt::lang_start_internal::{{closure}}#0 0:&&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:348:40
    = note: inside `std::panicking::r#try::<i32, [closure@std::rt::lang_start_internal::{{closure}}#0 0:&&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe]>` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:325:15
    = note: inside `std::panic::catch_unwind::<[closure@std::rt::lang_start_internal::{{closure}}#0 0:&&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panic.rs:394:14
    = note: inside `std::rt::lang_start_internal` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:51:25
    = note: inside `std::rt::lang_start::<()>` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:67:5
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

test mem_yaml ... 
error: could not compile `rustbreak`.

To learn more, run the command again with --verbose.

It happens in linked-hash-map, a dependency of serde_yaml. There is not much we can do about it, but causing UB just by calling Database::save is very undesirable. It shoud at least be stated in the documentation until it is fixed.

[TheNeikos]

I'd rather that it is noted at the Yaml Deser, or just be taken out completely. I find yaml personally not interesting and wouldn't mind it not being in the repo. Of course, users can easily create their own deser. Which is probably preferred anyway, as this prevents stale dependencies.

[niluxv]

I'm also okay with removing the Yaml DeSer altogether. That also fixed the annoying CI failures (I marked the miri check as optional but for some reason github still says that the ci fails...).

[niluxv]

With the release of 2.0.0 it is no longer possible to remove the Yaml DeSer because that is a breaking change...

[TheNeikos]

Yes, I have committed to keeping it in, however the (potential) UB needs to be documented and hopefully a future release will be able to fix this.

[niluxv]

I documented this in #92. I'd propose to keep this issue open as the tracking issue until the problem is properly fixed upstream (either in linked_hash_map or in yaml-rust by replacing the linked_hash_map dependency).