`UnsafeCell` shared accross threads with no locking - this can cause UB right?

Question

Consider the following code:

use std::{cell::UnsafeCell, io, net::TcpStream, sync::Arc};

use native_tls::TlsStream;

#[derive(Debug)]
pub struct TcpStreamRecv(Arc<UnsafeCell<TlsStream<TcpStream>>>);

unsafe impl Send for TcpStreamRecv {}
unsafe impl Sync for TcpStreamRecv {}

impl io::Read for TcpStreamRecv {
    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
        unsafe { &mut *self.0.get() }.read(buf)
    }
}

#[derive(Debug)]
pub struct TcpStreamSend(Arc<UnsafeCell<TlsStream<TcpStream>>>);

unsafe impl Send for TcpStreamSend {}
unsafe impl Sync for TcpStreamSend {}

impl io::Write for TcpStreamSend {
    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
        unsafe { &mut *self.0.get() }.write(buf)
    }

    fn flush(&mut self) -> io::Result<()> {
        unsafe { &mut *self.0.get() }.flush()
    }
}

pub fn tcp_split(stream: TlsStream<TcpStream>) -> (TcpStreamSend, TcpStreamRecv) {
    let inner = Arc::new(UnsafeCell::new(stream));
    let send = TcpStreamSend(inner.clone());
    let recv = TcpStreamRecv(inner);
    (send, recv)
}

My reasoning is as folows:

Safe code can obtain a TcpStreamSend and a TcpStreamRecv with the same underlying TlsStream.

If each of these is sent to a separate thread, it is possible to call TcpStreamSend::write and TcpStreamRecv::read at the same time. Each of these functions obtains a &mut reference to the underlying TlsStream.

Therefore, since it is illegal to have 2 mutable references at the same time, this code can cause UB and should be considered unsound. Is this correct?

My coworker assured me that "if it works, it works", and the code indeed appears to function normally most of the time, apart from some random occasional panics. In my understanding, however, it can cause unpredictable problems anywhere in our codebase, and should be rewritten immediately. Did I miss something?

By the way, I did some research and the code seems to be inspired by this SO answer which is more convoluted, but, as far as I can see, equally bad.

The code is unequivocally UB. However, it is strange that `&native_tls::TlsStream` doesn't implement `Read` and `Write` like `&std::net::TcpStream` does. My best guess is that this is an oversight in the implementation of `native_tls`, but don't quote me on this. It does unfortunately mean that you will have to wrap `TlsStream` in a `Mutex` if you want to share it across threads unless you switch to a different TLS crate. — L. F., Aug 02 '23 at 10:23
To answer your second question: in production code, I've always seen a comment before `unsafe` blocks that explain why the unsafe requirement is upheld, and a comment before `unsafe` functions to explain what unsafe requirement must be upheld on call. — jthulhu, Aug 02 '23 at 14:36
@L.F. It is hard to impl `Read` and `Write` for `&TlsStream` because it is generic. It is possible, though, with HRTB. — Chayim Friedman, Aug 02 '23 at 16:09
Yes, this is indeed undefined behavior. You should use a mutex (or a read-write lock in some related cases). For future reference, if you are implementing some concurrency primitive that uses an underlying `UnsafeCell`, consider using [SyncUnsafeCell](https://doc.rust-lang.org/std/cell/struct.SyncUnsafeCell.html). The API is identical, but `SyncUnsafeCell` implements `Sync`, so you wouldn’t have to manually implement unsafe traits for your own structs. — Mark Saving, Aug 02 '23 at 18:44
Contrary to what 2 commenters prior have stated your code does **not** include any UB in and of itself, though your assessment that it's *unsound* as it can be used to invoke UB from safe code (and very likely is being used in a way that does so) is very much correct. — cafce25, Aug 02 '23 at 19:07
Yes, @cafce25's correction is what I meant to say - UB is invoked as soon as `read` and `write` are called synchronously on `send` and `recv`, which is most likely what the caller of `tcp_split` will do. — L. F., Aug 03 '23 at 01:09
@MarkSaving I argue the opposite: you should use bare `UnsafeCell` and impl `Send` manually. For two reasons: the least important is that `SyncUnsafeCell` is nightly-only. The more important is that by writing `unsafe impl Sync` manually, you get the chance to write a `// SAFETY` comment explaining the safety rationale. The main reason for `SyncUnsafeCell` existence is for use in `static`s, to not have to write your own type. — Chayim Friedman, Aug 03 '23 at 12:42

score 3 · Accepted Answer · answered Aug 04 '23 at 13:01

Definitely unsound.

Simply having two overlapping &mut references in existence at the same time in Rust is undefined behavior, which will happen with this if two threads call read and write at the same time. This is a problem even for non-threaded code - Rust assumes that no &mut references can alias.

Splitting a TlsStream without a lock like this is just not possible. Even if the read and write implementations in TcpStream are independent, TlsStream may need to alter how it writes based on messages from the server (ex. rekeying), which will require that the reader alter state that the writer uses.

`UnsafeCell` shared accross threads with no locking - this can cause UB right?

1 Answers1