Unsoundness

[cynecx]

event-listener in its current state (531c106) is unsound. It's possible to trigger a use-after-free bug completely with safe Rust.

PoC:

use event_listener::{Event, EventListener};

fn main() {
    let event = Event::new();
    let event2 = Event::new();

    let mut listener = Box::pin(EventListener::<()>::new());
    listener.as_mut().listen(&event);
    listener.as_mut().listen(&event2);

    drop(listener);
    event.notify(1);
}

cargo miri run:

paul@Pauls-MBP ~/dev/event-listener-test (git)-[master] % cargo miri run
Preparing a sysroot for Miri (target: aarch64-apple-darwin)... done
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
     Running `/Users/paul/.rustup/toolchains/nightly-aarch64-apple-darwin/bin/cargo-miri runner target/miri/aarch64-apple-darwin/debug/event-listener-test`
error: Undefined Behavior: out-of-bounds pointer use: alloc846 has been freed, so this pointer is dangling
   --> /Users/paul/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ptr/non_null.rs:399:18
    |
399 |         unsafe { &*self.as_ptr().cast_const() }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer use: alloc846 has been freed, so this pointer is dangling
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
help: alloc846 was allocated here:
   --> src/main.rs:7:24
    |
7   |     let mut listener = Box::pin(EventListener::<()>::new());
    |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: alloc846 was deallocated here:
   --> src/main.rs:11:5
    |
11  |     drop(listener);
    |     ^^^^^^^^^^^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `std::ptr::NonNull::<event_listener::sys::Link<()>>::as_ref::<'_>` at /Users/paul/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ptr/non_null.rs:399:18: 399:46
    = note: inside `event_listener::sys::Inner::<()>::notify::<event_listener::notify::Notify>` at /Users/paul/.cargo/registry/src/index.crates.io-6f17d22bba15001f/event-listener-4.0.0/src/std.rs:263:42: 263:52
    = note: inside `event_listener::sys::<impl event_listener::Inner<()>>::notify::<event_listener::notify::Notify>` at /Users/paul/.cargo/registry/src/index.crates.io-6f17d22bba15001f/event-listener-4.0.0/src/std.rs:119:9: 119:35
    = note: inside `event_listener::Event::notify::<i32>` at /Users/paul/.cargo/registry/src/index.crates.io-6f17d22bba15001f/event-listener-4.0.0/src/lib.rs:432:24: 432:44
note: inside `main`
   --> src/main.rs:12:5
    |
12  |     event.notify(1);
    |     ^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

The issue is that EventListener::listen(mut self: Pin<&mut Self>, event: &Event<T>) doesn't deal with the case when the EventListener is already currently linked/associated with another/or same Event. It just unconditionally overwrites the content of the EventListener's Listener.

In my opinion, the general api design of Event/EventListener is rather unnecessarily complex and introduces more overhead than needed. Tokio's Notify api is much "better" in that regard. 🤷‍♂️

[notgull]

Thanks for the report! I should've known there was something I missed in #94.

In the short term this can be fixed by checking the Event being passed into listen() and making sure to replace it properly. Unfortunately it looks like #94 introduced more footguns than it fixed, in addition to the performance loss. I'd like to revert this, but I think 3 breaking changes in six months is a little much for a crate that's supposed to be stable.

@smol-rs/admins I won't have time to fix this until the weekend, any thoughts here?

[zeenix]

In my opinion, the general api design of Event/EventListener is rather unnecessarily complex and introduces more overhead than needed. Tokio's Notify api is much "better" in that regard. 🤷‍♂️

Ours was simple and straight forward too before 3.0.

In the short term this can be fixed by checking the Event being passed into listen() and making sure to replace it properly. Unfortunately it looks like #94 introduced more footguns than it fixed, in addition to the performance loss. I'd like to revert this, but I think 3 breaking changes in six months is a little much for a crate that's supposed to be stable.

True. I wonder if we should revert back to 2.x API. That API was quite stable and lacked all these footguns and unsoundness behaviors. I'm fine with some allocations if that's the cost.

[notgull]

The 2.X API, while admittedly simpler, pretty recklessly abused heap allocations. The fact that the 3.0 API doubled its speed should be pretty good evidence of this. Therefore I would oppose going back to the 2.X API.

There is very little overhead in the 3.X API. Some overhead was introduced in #94, but I only ever measured it as a 7% performance drop. The no_std API has a higher overhead, but that's the tradeoff for embedded system support and a lack of clean mutexes.

Although I do think smol should prioritize simplicity over performance, a 50% time reduction is pretty hard to say no to. Not to mention, the simple API still exists. You can just call event.listen() if you don't mind the heap allocation and just use event-listener almost exactly the same as in 2.X. Restricting the API to something that performs worse in 100% of cases because some users might become confused isn't what I want for this crate.

[notgull]

Tokio's Notify api is much "better" in that regard. 🤷‍♂️

I'd like to clarify that tokio's Notify API uses something very similar to what event-listener v3.0 used. The only real difference is that polling Notified automatically inserted the listener into the list, as opposed to listen() needing to be called to manually insert the listener. In my opinion this is a footgun that can cause deadlocks in certain circumstances.

[fogti]

In my opinion this is a footgun that can cause deadlocks in certain circumstances.

Please elaborate on that.

---

In general, it appears a bit weird to me that Listener can be in a "completely empty" state at all. It does not directly seem particularly useful to me; at least not more useful than it without the Option (s?) inside, and being wrapped by a (potentially pinned) Option. It might make more sense to provide an API which gets passed a closure (the closure should be receiving a pinned, listening Listener, but such that the Pin can't escape (e.g. due to being bound by a forall lifetime)), and one which allows the user to provide its own wrapping method to wrap it such that it is pinned, but the wrapper being movable (that is, Unpin + DerefMut<Target = EventListener>). It might be useful to provide some utility functions to deal with swapping out Listeners in Options, etc., but normally that should be handled by Drop and similar traits. (As this seems like over-adaption to a use case which I haven't encountered much at all in the wild, although I might be wrong about that usage distribution)

[notgull]

Please elaborate on that.

I elaborate more in this comment.

In general, it appears a bit weird to me that Listener can be in a "completely empty" state at all. It does not directly seem particularly useful to me; at least not more useful than it without the Option (s?) inside, and being wrapped by a (potentially pinned) Option.

Unfortunately we can't create an already-pinned EventListener, unless you want to add a macro that uses a private constructor to create it. That's one way of doing it, I suppose.

It might make more sense to provide an API which gets passed a closure (the closure should be receiving a pinned, listening Listener, but such that the Pin can't escape (e.g. due to being bound by a forall lifetime))

Closures can't really be used in async functions.

[fogti]

Closures can't really be used in async functions

why? I'd understand if that were about closures returning Futures or such, but why can't "normal" closures, or simple functions, e.g. Box::pin or such be used?

[zeenix]

Although I do think smol should prioritize simplicity over performance

Sure but as a Rust crate, it should priorities soundness, safety and correctness above all IMO.

a 50% time reduction is pretty hard to say no to.

In general, yes but it depends on two other factors: what was the performance like w/o this reduction (i-e the actual gain) and the price to pay for this. If the price is having easy footguns and strange API (as @fogti also pointed out), then we have to be critical of the performance gain being worth it.

Unfortunately we can't create an already-pinned EventListener, unless you want to add a macro that uses a private constructor to create it. That's one way of doing it, I suppose.

I think that would be a lot better than having a footgun.

[notgull]

Yes, all of the APIs proposed have problems. The 2.x API had serious performance implications and the 3.x API had the footgun. If we're considering another API break, I'd rather use one that takes advantage of Rust's type system. Maybe:

• We have an EventListener trait.
• We have a HeapListener implementation that uses the heap.
• We have a StackSlot structure that sits on the stack.
• Pin<&mut StackSlot> can be combined with an &Event to create a StackListener which also implements the EventListener trait. Combining inserts the slot into the listener list.

Regarding the issue, #101 is the short term fix for this problem

[zeenix]

• We have an EventListener trait.

• We have a HeapListener implementation that uses the heap.

• We have a StackSlot structure that sits on the stack.

• Pin<&mut StackSlot> can be combined with an &Event to create a StackListener which also implements the EventListener trait. Combining inserts the slot into the listener list.

That sounds like a great idea but how about the new trait is called Listener and we name HeapListener as just EventListener. Since we have had a few API breaks in a very short amount of time, I think a lot of people would be porting directly from 2.x to 5.x and it'd be good to make it easy for them to port.

Also a macro might be good for handling the StackSlot creation.