Fix unsoundness via impure AsRef

[niklasf]

An impure AsRef implementation can cause UB in safe code, by presenting different slices on each call.

use std::sync::atomic::{Ordering, AtomicUsize};

#[derive(Default)]
struct Evil {
    toggle: AtomicUsize,
}

impl AsRef<[u8]> for Evil {
    fn as_ref(&self) -> &[u8] {
        if self.toggle.fetch_xor(1, Ordering::Relaxed) == 0 {
            b"hi"
        } else {
            b"there"
        }
    }
}

[stanislav-tkach]

Could you please elaborate in what case exactly situation UB can occur?

[aleksuss]

Doesn't the borrow checker check such "undefined behaviour" in a compile time?

[niklasf]

With evil: Evil as defined above:

let key = evil.as_ref().as_ptr(); // Pointer to "hi"
let key_len = evil.as_ref().len(); // Length of "there"

Then, passing key with key_len to RocksDB, out of bounds reads will occur.

The fix is simply to only ever call .as_ref() once in each of these contexts.

The implementation of Evil required interior mutability to hide from the borrow checker, so it takes some effort to break things, but it's still possible.

[stanislav-tkach]

I think technically it wouldn't be a safe code, because out-of-bounds reads can only occur through FFI border and FFI functions are unsafe. I see your point now and I think that using as_ref can make sense here, but I'm still not sure if a library should really be aware of such implementation.

[niklasf]

The FFI functions are unsafe, because the compiler cannot check the required invariants, but this library tries to offer a safe interface. So it needs to maintain all required invariants.

Suppose a buggy string interning library (written entirely without unsafe) uses interior mutability for caching. A bug in that safe code plus unsoundness in this library would then escalate to an actual memory safety issue.

https://docs.rs/dtolnay/latest/dtolnay/macro._03__soundness_bugs.html#soundness puts it well:

Soundness is a statement about whether all possible uses of a library or language feature uphold the intended invariants. In other words it describes functionality that cannot be misused, neither by mistake nor maliciously.

It is worth internalizing this understanding of soundness when evaluating soundness bugs; they are a very different sort of bug than typical exploitable memory safety vulnerabilities like use-after-free or buffer overflows. When a library is unsound, it tells you the library is possible to misuse in a way that could be a vulnerability, but it does not tell you that any code has already misused the library in such a way.

In my experience discovering unsound library code in my work codebase, so far it’s always only been hypothetical contrived code that could be broken; the existing uses of the unsound libraries have always been correct. We fix the soundness bugs to ensure it remains that way as the codebase scales.

[aleksuss]

The case you've provided could be possible only in multithreaded code. In such a case, you should guarantee sync access to the database on your side.

[niklasf]

For example, yes. And one of Rusts main selling points is to encode such requirements in the type system, so that they can be checked at compile time.

In most other languages such a requirement would be implied and that would be that. In Rust, libraries generally aim to be sound, i.e., to only rely on safety invariants that the compiler can enforce or mark the function as unsafe and document the exact requirements.

This requires some special care to uphold invariants when writing unsafe code, to not rely on any invariants that are only implied, not enforced, as can be seen here. Fortunately the fix is simple.

[stanislav-tkach]

You have convinced me. 🙃

[aleksuss]

I want a real example of the code with rocksdb that reproduces the issue this PR "fixes".

[niklasf]

Sure. I have completed the example from the PR description and added it as a test case.

rust-rocksdb git/evil-as-ref  6s
❯ git revert HEAD
[evil-as-ref 548bb16] Revert "Fix unsoundness via impure AsRef"
 3 files changed, 57 insertions(+), 103 deletions(-)

rust-rocksdb git/evil-as-ref
❯ cargo test evil_as_ref --test test_db
   Compiling rocksdb v0.19.0 (/home/niklas/Projekte/rust-rocksdb)
    Finished test [unoptimized + debuginfo] target(s) in 8.36s
     Running tests/test_db.rs (target/debug/deps/test_db-a8da022cf37ddac4)

running 1 test
error: test failed, to rerun pass `--test test_db`

Caused by:
  process didn't exit successfully: `/home/niklas/Projekte/rust-rocksdb/target/debug/deps/test_db-a8da022cf37ddac4 evil_as_ref` (signal: 11, SIGSEGV: invalid memory reference)

Since this is UB, the crash is not guaranteed, but I consistently see it if one of the return values of the .as_ref() is much longer than the other. The test case is only calling safe functions.