Use of static mut and &'static mut has a high risk of undefined behaviour

[huonw]

static mut (and similarly &'static mut) is quite dangerous, because they make it very easy to trigger undefined behaviour in any unsafe using them (rust-lang/rust#53639, rust-lang/unsafe-code-guidelines#269), both in multi-threaded programs (due to unsynchronised mutation) and in single threaded ones too (due to, for instance, aliasing of &mut).

The code_coverage_sensor module contains many of these, and exposes the shared_sensor function:

fuzzcheck-rs/fuzzcheck/src/code_coverage_sensor/mod.rs

Lines 22 to 24 in ebd01f5

	pub fn shared_sensor() -> &'static mut CodeCoverageSensor {
	unsafe { &mut *SHARED_SENSOR.as_mut_ptr() }
	}

which is particularly dangerous: if this is called in multiple stack frames, I believe it is considered undefined behaviour (and so, 'seems to work' is unreliable, if anything is changed about the environment/code) instantly, even if the references aren't used/mutated. As an example:

fn f() {
    let sensor_f = shared_sensor();
    g()
}

fn g() {
    let sensor_g = shared_sensor(); // Undefined behaviour: sensor_g and sensor_f alias
}

https://github.com/rust-lang/miri may be able to detect errors like this, and in particular may highlight if this occurs in practice within fuzzcheck.

Notably, the &'static mut lifetime bounds makes this particularly easy, because those &mut references can be returned from any function returning a reference, meaning it's easy to accidentally (and non-obviously) have the aliasing. For instance, I believe the following would compile:

struct SomeStruct;

impl SomeStruct {
    fn some_method(&mut self) -> &mut bool { // implicitly &'a mut self -> &'a mut bool
        &mut shared_sensor().is_recording // 'static > the implicit &mut self lifetime, so this is okay
    }
}

fn h() {
    let mut struct_a = SomeStruct;
    let a = struct_a.some_method();

    let mut struct_b = SomeStruct;
    let b = struct_b.some_method();
    // undefined behaviour: b and a alias, despite seeming to come from entirely separate structs
}

Alternative designs that would minimise the risk here might be:

• instead of exposing a mutable global singleton as a &mut ... reference with methods, only expose free functions like start_recording directly. These can be implemented in terms of a mutable global singleton, at least to start with (to be able to progressively reduce the risk of undefined behaviour rather than have to do major refactoring to see any benefit), as long as no reference are exposed from the module (and it looks like this would be fine: I think the only reference currently exposed by the module is the shared_sensor function). For instance,

  fuzzcheck-rs/fuzzcheck/src/code_coverage_sensor/mod.rs

  Lines 39 to 44 in ebd01f5

  	impl CodeCoverageSensor {
  	pub fn start_recording(&mut self) {
  	self.is_recording = true;
  	self.lowest_stack = usize::MAX;
  	*self._lowest_stack = usize::MAX;
  	}

  
  could become a top level function (this first refactoring does require pushing unsafe into each of the methods, but this is somewhat more 'honest': each of these functions is (thread) unsafe):

  pub fn start_recording() {
      unsafe {
          let sensor = SHARED_SENSOR.as_mut_ptr();
          sensor.is_recording = true;
          sensor.lowest_stack = usize::MAX;
          *sensor._lowest_stack = usize::MAX;
      }
  }

  The use-sites like would also need changing. For instance,

  fuzzcheck-rs/fuzzcheck/src/fuzzer.rs

  Lines 206 to 221 in ebd01f5

  	let sensor = shared_sensor();
  	sensor.clear();
  	if timeout != 0 {
  	set_timer(timeout);
  	}
  	sensor.start_recording();
  	let cell = NotUnwindSafe { value: test };
  	let input_cell = NotUnwindSafe {
  	value: input.value.borrow(),
  	};
  	let result = catch_unwind(|| (cell.value)(input_cell.value));
  	sensor.stop_recording();

  
  could become

        code_coverage_sensor::clear();
  
        if timeout != 0 {
            set_timer(timeout);
        }
  
        code_coverage_sensor::start_recording();
  
        let cell = NotUnwindSafe { value: test };
        let input_cell = NotUnwindSafe {
            value: input.value.borrow(),
        };
        let result = catch_unwind(|| (cell.value)(input_cell.value));
  
        code_coverage_sensor::stop_recording();

• use *mut (or maybe &raw mut?) instead of &mut, to avoid undefined behaviour due to aliasing of &mut. There's still danger here (like needing synchronisation in multi-threaded code, and the risk of dangling pointers), but there's no risk of violating aliasing rules.

• use atomics instead of static mut and &mut/*mut. For instance, static mut X: uintptr_t becomes static X: AtomicUsize and &'static mut uintptr_t becomes &'static AtomicUsize. If the program isn't multithreaded, one can use relaxed orderings to have reduced cost but still be correct in the face of re-entrancy, and let the compiler help ensure safety more.

These are somewhat in order, in that they build on each other.

I hope this makes sense!

[loiclec]

Thanks a lot for the explanation and suggestions!

I have never felt good about the code coverage sensor (and signal handling) code, but also didn't really know what to do about it, or even what exactly could be wrong with it. I will do the refactoring you suggested and try to look out for similar mistakes elsewhere.

I think there will still be a lot of problems with multithreaded code, which is more difficult to solve. So maybe I should warn to use fuzzcheck only for single-threaded programs at the moment. For example, when using the trace_compares instrumentation (which is enabled by default), I know that the HBitSet structure may be simultaneously modified from different threads, which I suspect is both incorrect and unsafe.

When I have the time, I will post links to the refactors here.

[loiclec]

Oh, I was about to post a link to the commit, but GitHub does it automatically! Nice. So, for now, I have replaced instances of shared_sensor() with free functions. Will do more to limit unsafety later :)