Possible unsound unsafe code

help
1

I haven't worked on Rust for some time, I got a issue raised with a crate, and would like an opinion, see

2

It looks without great study to be probably OK.
The iterator consumes so the only other access is drop. The drop on the Iterator (not shown in issue) appears to take care of clearing out content.

3

I can see a potential soundness issue but nothing like the issue opener described.

The issue I can see is that the drop implementation of the IntoIterShortVec type does first consume all items in a loop and then sets the internal length to 0 so they aren't dropped again by destructor of the the ShortVec inside. But if one of the items' destructors panics, I think the inner ShortVec would still get dropped normally during unwinding and hence you could get some double-free.

6 Likes
4

IMO, BasicVec::get() is "technically unsound", since the documented safety condition is not enough to ensure UB free for the use of ptr::read(), quote from the documentation:

Ownership of the Returned Value

read creates a bitwise copy of T, regardless of whether T is Copy. If T is not Copy, using both the returned value and the value at *src can violate memory safety.

at least, there should be something like this statement in the safety documentation:

/// # Safety
/// [...]
/// `BasicVec::get(ix)` can only be called at most once for each valid `ix`

but as the type BasicVec is private, it's not a big deal, we only need to check the use of BasicVec::get() within the same module.

from a quick glance, I think the IntoIterShortVec type indeed has a double drop problem, because IntoIterShortVec<T> contains a ShortVec<T>, which also implements Drop. you should at least wrap it in a ManuallyDrop.

EDIT:

whoops, I missed the line of setting the internal length to 0, but still, there's a possibility of double drop in the case of panic, as pointed out by @steffahn

5

Thanks. I do have a memory of fixing issues related to panics in destructors when working on the supported crate ( pstd ). Hopefully the supported crate does not have such issues.

6

It does not have Drop so will leak memory if not manually emptied.

7

That is correct, set_alloc has to be called to free the memory. I think I designed it this way to minimise memory usage, that is it avoids having to store a reference to the Tuning (Allocator) in every ShortVec instance. I was trying quite hard to minimise memory usage in this BTreeMap.

Of course it would be simplest to use a plain Vec, but that has extra overhead.