Some usage of Allocator::deallocate() is UB

[inahga]

I believe any situation where we take a reference and directly deallocate it is undefined behavior.

Consider deflate::Window::drop_in:

    pub unsafe fn drop_in(&mut self, alloc: &Allocator) {
        if !self.buf.is_empty() {
            let buf = core::mem::take(&mut self.buf);
            alloc.deallocate(buf.as_mut_ptr(), buf.len());
        }
    }

We are correct to take(&mut self.buf), so that we don't leave a dangling reference inside the struct. But, we then assign the reference to buf, then deallocate it afterwards. After the deallocate() call, buf is now dangling. It is indeed dropped immediately after, but references must not be dangling while they're live.

We can trivially tweak this into:

            let (buf, len) = {
                let buf = core::mem::take(&mut self.buf);
                (buf.as_mut_ptr(), buf.len())
            };
            alloc.deallocate(buf, len);

Now buf will be dropped at the end of the let scope, so it won't have the chance to become dangling.

But, the allocator API lends itself to this UB, since it returns references. It may be better for the allocator to only issue pointers, and structs should store pointers and cast to short-lived references only when required (i.e. whenever you can guarantee that the pointer outlives the reference). AFAICT other allocating structures in the standard library, e.g. Vec, more or less follow this pattern.

This isn't caught by Miri. I'm not 100% sure why, but it seems to be a limitation of its memory model .

[inahga]

This isn't caught by Miri. I'm not 100% sure why, but it seems to be a limitation of its memory model .

I raised the question in Zulip https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/Is.20this.20deallocation.20UB.3F/near/478350147. My conclusion is more strongly that the Allocator should only deal in pointers (that is, even my "fixed" example is questionable). I can play around with the code to see if this is reasonably feasible.

[folkertdev]

In general I'm on board with removing the deallocate functions and coming up with something better/safer :+1:

[inahga]

I played around for a bit with pointers https://github.com/trifectatechfoundation/zlib-rs/compare/main...inahga:zlib-rs:inahga/dealloc-safe, and it looks quite tractable (though I don't guarantee what I wrote works :smile:). Mostly mechanical changes. I do suggest simply having the allocator deal in raw pointers.

[inahga]

@folkertdev I believe you have resolved this? Looking at usage of deallocate() looks like we're only deallocating from the pointer from allocate() :tada:. Was there anything else you were going to do related to this issue?

[folkertdev]

no this is done, fixed by

• https://github.com/trifectatechfoundation/zlib-rs/pull/237
• https://github.com/trifectatechfoundation/zlib-rs/pull/239