microsoft / security-utilities

Security utilities for key generation, string redaction, etc.
MIT License
24 stars 9 forks source link

Rust: `ScanEngine` is `!Sync` and `!Send` #76

Closed jdraaijer-microsoft closed 1 month ago

jdraaijer-microsoft commented 2 months ago

Because ScanEngine includes an Rc and a non-send-and-sync dyn Fn, it is neither Send nor Sync. This means that re-using it in multi-threaded applications is not (trivially) possible.

This makes it so neither of the following compile:

    const fn assert_send<T: Send>() {}
    const fn assert_sync<T: Sync>() {}

    // These do not compile :(
    const _: () = assert_sync::<ScanEngine>();
    const _: () = assert_send::<ScanEngine>();

    let engine = ScanEngine::new(Default::default());
    std::thread::scope(|s| {
        s.spawn(|| {
            let mut state = ScanState::default();
            engine.byte_scan(&mut state, &[]);
        });
    });

    let engine = ScanEngine::new(Default::default());
    std::thread::spawn(move || {
        let mut state = ScanState::default();
        engine.byte_scan(&mut state, &[]);
    });

To fix it, we can:

  1. Tighten the bound on all validators to in include + Send + Sync and replace the Rc with an Arc.
  2. Replace the Rc with a function pointer outright (i.e. fn(&[u8]) -> usize).

The effect of 1 is that the validator is a slightly less flexible, but for most practical applications this should be fine. All of the definitions in the project currently are effectively fn(&[u8]) -> usize, so they strictly adhere to it. Most other validators should also own their data and therefore adhere to the new bound, so impact should be minimal.

For 2, it means that the validators become, effectively, compile-time variables that can not change at runtime. That sounds undesirable, but I wanted to highlight it as a potential solution.