Fix free after clone Pt. 2 #7
Conversation
src/lib.rs
Outdated
| pub const fn borrowed(&self) -> MownStr { | ||
| MownStr { | ||
| addr: self.addr, | ||
| xlen: self.xlen & LEN_MASK, | ||
| _phd: PhantomData, | ||
| } | ||
| } |
There was a problem hiding this comment.
@pchampin this method was particularly finicky in triggering use after free UB behaviour in a lot of sophia term methods
I strongly recommend removing it altogether and rely on Clone + Arc semantics to conserve on string allocation
mkatychev
force-pushed
the
mkatychev-jklin/fix-free-after-clone
branch
from
e9ecd6e to
2921d05
Compare
| len: other.len(), | ||
| _phd: PhantomData, | ||
| owners: None, |
There was a problem hiding this comment.
This change goes against the aim of MownStr, which is to cost no more space than a regular &str or Box<str>. If we go for this cost, we might as well use Cow<str> instead of MownStr.
| fn clone(&self) -> Self { | ||
| self.owners | ||
| .as_ref() | ||
| .map(|o| o.fetch_add(1, Ordering::Relaxed)); | ||
|
|
||
| MownStr { | ||
| addr: self.addr, | ||
| len: self.len, | ||
| _phd: self._phd, | ||
| owners: self.owners.clone(), |
There was a problem hiding this comment.
This radically changes the way MownStr was supposed to work. Namely, when cloning an owned MownStr, the text data should be duplicated, just like when cloning a Box<str>. That's why, IMO, there should not be any double-free even after a clone.
There was a problem hiding this comment.
@pchampin Totally agree, the intent was simply to satisfy miri
|
Closed in favour of #8 |
mownstr/src/lib.rs
Line 29 in 95e193a
Addresses #5 further