Library is not thread safe

[corvusrabus]

Hi there, I think that the library is currently not thread safe as pointed out in the test in #41. The Problem is that the library defines a global variable static mut RECV_BUF: SharedRecv = SharedRecv::null(); that can be accessed from anywhere without any checks. I guess it's up to the maintainers to decide how to change the architecture to fix that (maybe use an individual buffer for every open websocket?).

I would suggest to remove unsafe impl Send for SharedRecv {} until this is fixed.

[littledivy]

Thanks for opening an issue for this. This has mostly gone unnoticed because of the single threaded use in Deno.

I think we should have a RECV_BUF per thread. (thread_local?)

[corvusrabus]

I think we should have a RECV_BUF per thread. (thread_local?)

This would still be buggy because two instances of Websocket on the same thread would still use the same buffer and when you read a frame the Payload has a reference in that shared unsynchronized buffer. I think the simplest solution is to have one buffer per Websocket instance and let the Payload hold a reference into that buffer. That will also force that the payload reference won't be used after the buffer gets modified. I guess in this implementation you also shouldn't preallocate as much memory for the buffer as is done right now and instead dynamically grow the buffer.

[corvusrabus]

PS: The crate does not currently not build for me and the fix for me was to update the crate thiserror in Cargo.toml to the most recent version 1.0.43

[quetz]

How adding thread_local! to RECV_BUF fixes that, given that two different Websocket instances on same thread will still compete for RECV_BUF?

[A248]

@littledivy It looks like the use of recv buffer doesn't conform to shared mutability restrictions at all. Rust is supposed to prevent you from using a shared mutable pointer because of the consequences for memory safety. Take a look at this code snippet:

https://github.com/denoland/fastwebsockets/blob/35a1930fdbcdfe9034bb531fcd1690ba9f7737ec/src/lib.rs#L652-L668

Every await point is a place where the async function can suspend while other operations happen concurrently. When I say concurrently, I'm referring to Rust's async/await, not OS threads. This means that even a "single-threaded" program can have multiple concurrent WebSockets intermittently suspending themselves at different await points.

This means that when you call await, other async functions can run inside that time-slice. Here, that means the recv buffer can be initialized twice, thrice, four times, in the span of that await call. WebSockets can therefore overwrite each others' data in an unpredictable fashion.

Technically speaking, there are two problems with how the code snippet is written. One is that mutable aliasing rules are violated: multiple mutable references can be created to the same recv buffer. This is immediate undefined behavior in Rust. The other problem is what I already discussed: logically, you're manipulating the same shared buffer across await calls, a practice which will inevitably lead to different WebSockets seeing each others' data.

Normally, lifetimes and ownership are designed to prevent shared mutability. However, due to fastwebsockets using unsafe in an unsafe manner, all of these safeguards are broken. For example, the lifetime of recv::init_once() is being extended to 'static is a wholly incorrect. Unsafe isn't an escape hatch to do whatever you like; it requires a firm understanding of the ownership and safety concerns to use properly.

[quetz]

I ended up removing that shared buffer completely and switched to just plain-old Vec<u8> buffer for each websocket. Switching to something like bytes::Bytes might be better idea though.

[erebe]

Hello,

Just to let you know, I have made a patch that fixes the issue by using a ByteMut buffer per websocket connection. https://github.com/erebe/fastwebsockets/commit/ca6fe98d872c6f90c089125d95efc70c9ce5d778

It introduces a breaking change in the API as it introduces a new Payload type that holds a Bytes https://github.com/erebe/fastwebsockets/commit/ca6fe98d872c6f90c089125d95efc70c9ce5d778#diff-7d1b2470e6d199fdd5634da8b81a47dd48b61923c2b7550f9ba2bd41afe65d70R48

As it is kind of impossible to return a &[u8] anymore without the unsage &'static [u8] coming from the thread local storage.

Let me know if you are interested in this patch turning into a PR.

[littledivy]

Thanks for taking the time to look into this. Yes, happy to accept the change the payload to use bytes::BytesMut.

[erebe]

Here we are for the PR https://github.com/denoland/fastwebsockets/pull/63