bits![static mut …] is unsound

[SimonSapin]

trafficstars

The unsafe-free code below creates two &mut references to the same buffer at the same time, despite claim of the contrary in https://github.com/bitvecto-rs/bitvec/blob/main/book/data-structures/bitslice.md#macro-constructor. Multiple calls to a function accessing a static item will use the same static value at the same address, even if the item is defined inside the function’s body.

use bitvec::prelude::*;

fn x() -> &'static mut BitSlice {
    bits![static mut 0; 64]
}

fn main() {
    let (_, a, _): (_, &mut [usize], _) = x().domain_mut().region().unwrap();
    let (_, b, _): (_, &mut [usize], _) = x().domain_mut().region().unwrap();
    dbg!(a.as_mut_ptr(), b.as_mut_ptr());
}

[src/main.rs:10] a.as_mut_ptr() = 0x0000560238751058
[src/main.rs:10] b.as_mut_ptr() = 0x0000560238751058

bits![mut …] without a static token seems to be unaffected:

error[E0515]: cannot return value referencing temporary value
 --> src/main.rs:4:5
  |
4 |     bits![mut 0; 64]
  |     ^^^^^^^^^^^^^^^^
  |     |
  |     returns a value referencing data owned by the current function
  |     temporary value created here

If bits![static mut 0; 64] were to be changed to return &mut Bitslice with a non-'static lifetime but still backed by a static item, aliasing would still be possible in reentrant functions.

[SimonSapin]

I think possibles ways to close the soundness hole are:

• Remove bits![static mut …] entirely, making its use a compile error in macro expansion
• Make it the same as bits![mut …] returning a non-'static borrow, making some uses work but others fail with borrow-checker compile errors
• Maybe something else that involves BitStore::Alias? I’m not sure it would be enough, given APIs that give &mut access to storage and given that Cell::get_mut exists (if you have &mut Cell<_>)

[myrrlyn]

*headdesk* my most common fault with testing this crate is that because I know what the pitfalls are, I instinctively avoid them and don't often come up with test cases that exercises pathological cases such as this because it doesn't occur to me to try this.

The static mut buffer is desirable to have, as it obviates the need to manage stack temporaries as you demonstrate in the non-static case.

As far as I am able to determine, there is no sidestepping this problem: any method that allows producing a static mut address multiple times can be eventually made to alias incorrectly. I believe that there are only two ways to repair this without removing the static mut argument pair completely (which I would prefer to not do):

1. Lie: Reroute the bits![static mut …] expansion to produce &BitSlice<T::Alias, O> instead of the current &mut BitSlice<T, O>. This would have very nearly the same API to end users (change a few .set() to .set_aliased(), rewrite type declarations).

2. Cop out entirely: I am willing to language-lawyer the assertion that this is unsound (it is; I'm not disputing that) specifically due to this clause:

   The unsafe-free code below

   What if I just … removed the unsafe block at the end of my macro expansion, and produced an expression that required the user to enclose bits![static mut …] in an unsafe block of their own, and document that "this macro must not be invoked in a position that can be called multiple times"?

[SimonSapin]

Shifting the unsafe responsibility to users would make this API follow the letter of Rust soundness conventions, but as far as I can tell it would be very hard to use correctly in practice. Doesn’t it require that the relevant code is only ever reached from a single thread? That seems impossible for a library, and unreasonable for all but every small applications. (You’d have to keep remembering this non-thread-safety as the project evolves over time.) Either that or have some external synchronization, but at that point you’d be better off with something like Mutex<BitVec<T>> IMO.

Regardless of whether the unsafe is inside or outside the macro, what’s an example of correct use of bits![static mut …]?