Calling EventQueueRef::push with arbitrary Remote object can cause undefined behavior

[Qwaz]

Hello fellow Rustacean,
we (Rust group @sslab-gatech) are scanning Rust code on crates.io for potential memory safety and soundness bugs and found an issue in this crate which allows safe Rust code to exhibit an undefined behavior.

Issue Description

bottle/src/queue.rs

Lines 64 to 76 in c3d6ccb

	impl EventQueueRef {
	/// Push an event to the queue.
	pub fn push<E: 'static + Event, T: 'static + ?Sized + Handler<E>>(&self, receiver: Remote<T>, event: E) -> Future<T, E::Response> {
	let pending = Box::new(ToReceive::new(receiver, event));
	let future = Future::new(pending.state().clone());
	self.queue.push(pending);
	future
	}
	pub(crate) unsafe fn request_initialization<T: 'static, F: 'static>(&self, remote: Remote<T>, constructor: F) where F: Send + FnOnce() -> T {
	self.queue.push(Box::new(Initialize::new(remote, constructor)));
	}
	}

EventQueueRef::push(receiver, event) can be called on a queue that is not associated with the receiver. This allows ToReceive event to run before Initialize event and makes receiver's handler to access an uninitialized variable. A data race is also possible because two event queues can modify the same state concurrently.

bottle/src/remote.rs

Lines 69 to 77 in c3d6ccb

	// Why it is safe.
	// [1] We know the value won't be touched before initialization: the first
	// message received by the remote pointer is the initialization request.
	// This is because messages are processed in order in the queue. That is
	// why (among other things) posting a message directly to a remote
	// actor is unsafe.
	// [2] We know the actor won't be dropped before initialization: the
	// initialization request message holds a copy of the remote.
	data: MaybeUninit::uninit().assume_init()

This part of the code also needs a fix. Storing uninitialized memory to certain types is always unsound no matter whether it is touched or not. From MaybeUninit's Initialization invariant section:

As a consequence, zero-initializing a variable of reference type causes instantaneous undefined behavior, no matter whether that reference ever gets used to access memory

Reproduction

Below is an example program that accesses uninitialized memory using safe APIs of bottle.

Show Detail

#![forbid(unsafe_code)]
#![feature(arbitrary_self_types)]

use std::time::Duration;

use async_std::future;
use bottle::{Event, EventQueue, Handler, Output, Receiver, Remote};

struct EmptyEvent;

impl Event for EmptyEvent {
    type Response = ();
}

struct UninitChecker {
    // magic value, should be always 0x12345678
    magic: u64,
}

impl UninitChecker {
    pub fn new() -> Self {
        UninitChecker {
            magic: 0x12345678
        }
    }

    pub fn validate(&self) {
        if self.magic != 0x12345678 {
            panic!("Uninitialized value access! 0x{:x}", self.magic);
        }
    }
}

impl Handler<EmptyEvent> for UninitChecker {
    fn handle(self: Receiver<Self>, _event: EmptyEvent) -> Output<'static, ()> {
        self.validate();
        Output::Now(())
    }
}

#[async_std::main]
async fn main() {
    let queue1 = EventQueue::new();
    let queue1_ref = queue1.reference();
    let queue2 = EventQueue::new();
    let queue2_ref = queue2.reference();

    let remote = Remote::from(queue1_ref, || UninitChecker::new());

    std::thread::spawn(move || {
        async_std::task::block_on(queue2.process())
    });

    if let Err(_timeout) = future::timeout(Duration::from_secs(1), queue2_ref.push(remote.clone(), EmptyEvent)).await {
        println!("Future timeout");
    }
}

Output:

thread '<unnamed>' panicked at 'Uninitialized value access! 0x55a0a558d680', src/main.rs:47:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Future timeout

Return code 0

Tested Environment

• Crate: bottle
• Version: 1.1.0-alpha
• OS: Ubuntu 20.04.1 LTS
• Rustc version: rustc 1.50.0-nightly (1c389ffef 2020-11-24)
• 3rd party dependencies:
  • async-std = { version = "1.7.0", features = ["attributes"] }