Is there a resource leak problem when using pre-writing?

I've been practicing implementing a oneshot channel recently, and here's how the send method is implemented:

struct Inner<T> {
    state: AtomicU32,
    data: UnsafeCell<MaybeUninit<T>>,
}
impl<T> Inner<T> {
    const unsafe fn write(&self, val: &T) {
        unsafe {
            let uninit = &mut *self.data.get();
            std::ptr::copy_nonoverlapping(val, uninit.as_mut_ptr(), 1);
        };
    }
    fn send(&self, val: T) -> Result<(), T> {
        let mut status = Status::new(&self.state);
        if status.is_closed() {
            return Err(val);
        }
        // Pre-write. Since send is only called once, no additional checks are needed here
        // SAFETY: Single write, and status(!ready) ensures the receiver won't access data, as ready is only set later
        // Note: Before ready confirmation, there will be two copies of T data at the byte level in memory,
        // but since the pre-written memory is `MaybeUninit`,
        // that copy of `T` memory is only valid when status(ready) is confirmed,
        // and the original `T` memory becomes directly invalid (by `forget`), completing ownership transfer.
        // Therefore, this pre-write won't cause `T` to be dropped twice or not dropped at all
        unsafe { self.write(&val) };
        loop {
            // store fence, confirm write, clear waiting state to facilitate waking
            if status.try_setup_ready_release() {// weak cas
                forget(val); // ownership transferred
                self.try_wake_after_ready(&status);// check for waking
                return Ok(());
            }
            if status.is_closed() {
                return Err(val);
            }
        }
    }
}
impl<T> Sender<T> {
    pub fn send(self, val: T) -> Result<(), T> {...}
}

As shown above, the sender can only send once by consuming self, so in the send method, I adopted a pre-write strategy, which results in two memory regions: the pre-written value and the original value, which are actually the same value.
According to the uniqueness constraint of ownership, only one valid data can exist in both memory regions at the same time, and the other becomes "ghost data": after a successful CAS, ownership moves from the original value to the pre-written value (guaranteed by the ready flag), and the original value needs forget; otherwise, ownership remains with the original value, and the MaybeUninit in the pre-write region is still considered invalid data.
Therefore, regardless of whether the CAS succeeds or not, there is only one copy of ownership. The difference is that after success, the data will move, but this does not affect the correctness of resource deallocation.

Now the problem is, when I tried to use Copilot (GPT-5.2-Codex) to review my code, it pointed out a potential resource leak safety issue: if the CAS fails and the state becomes closed, then the original value is returned, but the copy of T in the pre-written value will never be dropped. No matter how I explain it to the AI, it insists that "although this is neither a double-free nor use-after-free, it is a resource leak".

I would like to know, does this pre-write strategy really have a resource leak problem?

Leaking is not a memory safety issue. Safe Rust can leak memory.

However I don't think pre-writing here will lead to resource leak, so my question is whether there is really a resource leak here

I don't think this can be answered without informations about Status's implementation and the readers

3 Likes

Status is just a readability wrapper for atomic states, its function is only to check the current state and modify it atomically using CAS.

struct Status<'a> {
    state: &'a AtomicU32,
    current: u32,
}

Besides, I don't think the specific details of 'receiver' will affect the implementation of 'send', or in other words, it's not important. From the perspective of the sender, it can either send successfully and set 'ready' bit, or it can only fail to send because the receiver was closed first.

So here I just want to discuss the issue of resource leakage in pre-writing strategies (because AI always insists on the existence of resource leakage)

Details like when exactly in the flow of send has the receiver "activated" the copy are missing.

Other things to consider are what happens if something unwinds and what Miri thinks if T contains a &mut (I think it retags during the call to forget).

As commented here:

the receiver can read from the Inner.data (assume_init_read) if and only if the ready bit was set by sender(try_setup_ready_release)

When working with this type of code, little details matter a lot. And i mean - a lot.

For example, in this code, depending on how Drop trait for Inner is implemented, it could lead to double free.

How do you distinguish if Inner::data has do be freed on Drop?

Let's say there is a scenario like this:

  1. active sender and writer,
  2. sender calls Inner::send, gets to self.write(&val) fails the line if status.try_setup_ready_release(),
  3. receiver closes channel,
  4. sender executes if status.is_closed() { and returns val.

Now the situation is such that, value is "copied" inside Inner::data and it is inside val. What happens next? If val is dropped and Inner Drop drops data as well, you will have double free. If data is Copy, probably there is no problem with that, if Clone, then there might be a problem. How is Drop handled?

If Drop does not free data, then there could be a scenario where:

  1. sender sends data and exits on branch if status.try_setup_ready_release(),
  2. receiver does not read data, not interested and closes channel.

What happens in this scenario?

These are just two scenarios what could happen depending on little details for other code parts. Maybe they are handled, maybe not, IDK, not enough code to tell.

There is a good tool for testing concurrent programs. loom - Rust

You should give it a try.

2 Likes

nice, I'll try it

(Your reply has more details than the comment.)

So, the receiver may read the value after try_setup_ready_release returns true, but hasn't necessarily done so yet? If so there may be a leak if it never reads it for whatever reason.

I tried another implementation of the 'send' function:

impl<T> Inner<T> {
    const unsafe fn write(&self, val: T) {
        unsafe {
            let uninit = &mut *self.data.get();
            uninit.write(val)
        };
    }
    fn send(&self, val: T) -> Result<(), T> {
        let mut status = Status::new(&self.state);
        if status.is_closed() {
            return Err(val);
        }
        // Pre-write, consuming the val
        // SAFETY: First unique write,
        // and status(!ready) ensures that the receiver will not access the data,
        // as status(ready) is only set later
        unsafe { self.write(val) };
        loop {
            // store fence
            // confirm writing, clear waiting status for subsequent wake-up
            if status.try_setup_ready_release() {// weak cas
                self.try_wake_after_ready(&status);
                return Ok(());
            }
            if status.is_closed() {
                // SAFETY: ready failed, but we know initialization is complete,
                // so we just read the data back and return it
                return Err(unsafe { self.read() });
            }
        }
    }
}

In this scenario, the new implementation should theoretically be completely equivalent to the original implementation. But AI no longer insists on its original viewpoint and can correctly understand the meaning of "pre-writing".

Moreover, the new implementation makes it easier for people to understand what send is doing.

I would not accept this as an answer, moreover i think it is incorrect (see below).

First of all, AI currently is not good enough for these kinds of tasks. It will not catch concurrency problems, nor memory leaks, etc., so concluding that given code is correct if AI does not complain is not a good basis for assumption that code is correct. The only thing that v2 code has solved is - AI does not complain anymore, or it has shown that you could trick AI not to catch a problem in given code. Most probably it is possible to inline assembly or do some tricky macro to achieve the same result, but that does not prove correctness in any way.

If you use unsafe code, you should also check your program more carefully. For this topics task Valgrind and Loom are your friends. And even then, when you have thoroughly tested various cases with these tools, it is hard to be 100% sure of correctness unless mathematically proven.

Another thing on the subject. The problem you are trying to solve is by no means an easy one. It takes time to appreciate the beauty of an existing solutions, of course, but eventually you'll get there, hopefully :slight_smile:

Now back to your v2 code. You have removed forget(val); in condition branch if status.try_setup_ready_release() {. This introduces two problems, use after free and double free if Drop trait is correctly implemented not to have a memory leak otherwise.

So the scenario goes like this:

  1. Let's say T is Clone, i.e. Vec. It has allocated memory on heap to store data,
  2. code line unsafe { self.write(val) }; copies pointer and some meta to Inner::data, but it does not copy Vec contents themselves that are located on heap, thus accessing them is allowed while region at given pointer is not freed,
  3. Then send executes the branch if status.try_setup_ready_release() {, on return Ok val is dropped, thus allocated memory for Vec is released, at this point it is not allowed to use Inner::data anymore, because it is use-after-free problem otherwise. At first you might not notice it, but if used in for loop this code will segfault at some point in time. Valgrind can catch this most of the time.
  4. Then receiver side at some point will drop this 'OneShot' channel's RX side, if Drop is not implemented, nothing bad happens, but the thing is that Drop trait in this case has to be implemented, and if it is correctly implemented, it will try to drop Inner::data and what happens there is a double-free problem, in other words, code is trying to free already freed memory region and as a result most probably process will segfault.

I think that a good help is to analyze the code with these types of scenarios. In each step think about what concurrent code can do, what happens when function returns, etc. It helps to draw them on paper to visually see.

This is because the write method in v2 code has been rewritten to move the val value to Inner:: data(of type MaybeUninit<T>), instead of the "bytes copy" in v1:

Therefore, it's necessary to remove forget(val) if ready succeed, or if is_closed checked true we just read the val back from Inner::data because we know it was initialized though the ready bit failed(which does mean data invalid outside)

Actually, Drop trait have already been implemented. And above all, the validity of Inner::data is only related to the ready bit of state. Whether the Inner::data will be read out(from receiver) or dropped(in place), it is always determined based on the ready status, to read or drop if is_ready checks true, or just ignore it otherwise.

This is why I do not want to introduce more complexity, as I only want to discuss the safety issues of pre-writing here. But sadly the previous always wanted to know more details (which I personally think are irrelevant, maybe).

I have tried the loom tool you mentioned, embedded it into my code, and with the help of AI, wrote 39 tests. All the tests just passed.

Actually, there were 49 tests at the beginning, but I found some tests that used too many threads (more than 3) would frequently timeout and freeze the progress, so I eventually gave up most of the multi-threaded tests.

My bad, i was skimming over the code, did not pay attention to missing &. Yes, value is moved.

I fall into the same group of people and believe that those details in this case are very important.

Anyhow, if it works for you, it works for you.

This is a code just to practice anyways, right?
[here we should embed a meme - Anakin and Padme, right?] :laughing:

The issue is that the safety issues of such pre-writing are conditional on what is the rest of the code. If try_setup_ready_release or try_wake_after_ready don't properly wake up the reader(s?), or neither the reader(s) or the writer consume the T, then that's going to be leaked. Similarly if status.is_closed() returns false. It's impossible to say this with certainty without seeing the rest of the code.

1 Like