How to safely clear bump buffer

[wlohrmann-hf]

Hi, I'm using the client in a no-std environment with a bump buffer. I'm currently running into some issues with running out of memory leading to Alloc issues. If I put a

        unsafe {
            client.buffer().reset();
        }

after receiving and sending messages that works but I'm not sure if I've done that correctly. Can you please help me understand how to do this in a consistent and safe way?

[julian-graf]

Hi!

As the documentation of BumpBuffer states, when calling it, none of the previously allocated values may be alive. The documentation there is actually slightly outdated. I will update it and make it clearer.

The essence of what we must avoid is aliasing references. Particularly a &mut [u8] referend must not be referenced by any other reference.
This can be caused if you hold the first BumpBuffer return value, reset it and then call its BufferProvider::provide_buffer again (this can happen with any value, not just the first, but it's particularly comprehensible in this example). At this point, you have an immutable reference to the start of the underlying buffer and a mutable reference freshly allocated which is UB. As of now, anything the client returns that is gated by lifetime lives in the BumpBuffers backing buffer. These are the values of the Bytes, MqttBinary, MqttString or other newtype types around those. The client only allocates in the backing buffer when receiving from the network.

I'll show this with an example:

let mut client = ...;

let ConnectInfo {
    client_identifier, ..   // This is a `MqttString` value living in `BumpBuffer`s underlying buffer.
} = client
    .connect(todo!(), &ConnectOptions::new(), None)
    .await
    .unwrap();

unsafe { client.buffer().reset() };

// This is sound because the client hasn't had a chance yet to allocate a buffer.
println!("{:?}", client_identifier);

// This can cause an allocation if we for example receive a PUBLISH packet with a `TopicName` and a `Bytes` message.
client.poll().await;

// This would be unsound because the client may already have allocated an aliasing reference to what is `client_identifier`.
// println!("{:?}", client_identifier);

I am not entirely sure this moment whether Rust will drop the reference after the first println! statement or if it does it at the end of the scope. To be on the safe (pun not intended) side, the following is definitely ok. Ideally, you would also not reference have the client_identifier reference after the BumpBuffer::reset call - this is against the current documentation - but it shouldn't cause UB.

let mut client = ...;

{
    let ConnectInfo {
        client_identifier, ..   // This is a `MqttString` value living in `BumpBuffer`s underlying buffer.
    } = client
        .connect(todo!(), &ConnectOptions::new(), None)
        .await
        .unwrap();
        
    // This is sound because the we can do anything we want before calling `BumpBuffer::reset`.
    println!("{:?}", client_identifier);
}

unsafe { client.buffer().reset() };


// This can cause an allocation if we for example receive a PUBLISH packet with a `TopicName` and a `Bytes` message.
client.poll().await;

// This would be unsound because the client may already have allocated an aliasing reference to what is `client_identifier`.
// But now it doesn't even compile anymore!
// println!("{:?}", client_identifier);

I hope this answers your question. If anything remains unclear, feel free to bring it up! I will update the docs...

PS: BumpBuffer unfortunately isn't sound at all in 0.4.1 and below due to using a mutable reference for the underlying buffer, but it shouldn't cause issues realistically. This is fixed with #92 though

[wlohrmann-hf]

Thanks for the speedy reply! That's working for me now.

Given that you essentially have to use a fresh buffer for every call anyway, do you think it would make sense to change the API so that the user passes the buffer directly into methods like poll_body and connect? That way we can get some lifetime guarantees tying the returned object references and buffer and the compiler will make sure we don't clear it prematurely. I may be interested in helping out if you think this is a reasonable API change.

[julian-graf]

I think this is definitely a reasonable API change as it can eliminate all usages of unsafe for the caller when using BumpBuffer. Thanks for the interest in contributing!

If we supply the buffer on a per-method basis, this could either be a BufferProvider impl directly or a &[u8] which can easily be turned into the required BumpBuffer internally. Here are my ideas on how your suggestion could be integrated:

• Remove the BufferProvider as a field of the Client struct entirely and make connect and poll_body generic against a new BufferProvider implementation. The user can then reuse the same BumpBuffer over and over again and use BumpBuffer::reset or just create a new BumpBuffer -> compiler can verify everything
• Add congruent _buffered (suffix just a suggestion) methods for connect and poll_body which take an additional argument as you described. This seems annoying at first glance because we have two places to maintain same functionality but perhaps this could be worked around by making the poll_body/recv_body methods call the _buffered method with self.buf as the buffer. (Just tried this on Raw-level since the buffer is managed there, the borrow checker doesn't allow it like I described, it could be worked around though)

I think the first option removes an abstraction that the client currently manages on what feels like the right level (though one could argue having to call into the inner buffer breaks this abstraction). The second option comes with the consequences of having either almost identical methods which is something I really want to avoid or requiring an perhaps ugly workaround with a few downsides.

What's your opinion and do you have an alternative in mind?