Changing Tendril Atomicity with SendTendril produces garbage.

[sqwishy]

My understanding is that this should not modify the text of the tendril and that this is a reasonable thing to do.

#[cfg(test)]
mod tests {
    use tendril::{fmt::UTF8, Atomic, NonAtomic, Tendril};

    #[test]
    fn it_works() {
        let s: Tendril<UTF8, NonAtomic> = "<p><i>Hello!</p>, World!</i>".into();
        dbg!(&s);
        let s: Tendril<UTF8, Atomic> = s.into_send().into();
        dbg!(&s);
    }
}

But the output is something like:

[src/lib.rs:8] &s = Tendril<UTF8>(owned: "<p><i>Hello!</p>, World!</i>")
[src/lib.rs:10] &s = Tendril<UTF8>(owned: "i>Hello!</p>, World!</i>\u{0}\u{0}\u{0}\u{0}")

It turns out the headers are different sizes so the transmute is weird.

std::mem::size_of::<Header<Atomic>>() = 16
std::mem::size_of::<Header<NonAtomic>>() = 12

I don't know if the Header should get #[repr(align(16))] or if there's a less platform-specific thing or something; but it seems like it might be a bug idk. ¯\_(ツ)_/¯

[SimonSapin]

This is definitely a bug. It looks like this functionality is not tested at all, lovely.

My suspicion is on PackedUsize, which has #[repr(C, packed)] but is only used for NonAtomic. It was introduced in #46 … maybe as an attempt to preserve some of the effect of #[repr(packed)] that was previously on Header? @xfix, is that right?

This in turn was added in b39ee1f which talks about the C API. Since we removed that, we should probably remove PackedUsize as well.

[SimonSapin]

Removing PackedUsize fixes this immediate issue, but that it happened in the first place only reinforces my feeling that Tendril’s casual and pervasive use of unsafe is dangerous. It might have been sound once, but when all of the library is responsible to maintain invariant that are only known by the original author it’s not surprising that things break is subtle ways when other folks make changer over the years.

I’d recommend not to use Tendril if you can, though presumably you need it in order to use html5ever.

I started a rewrite of Tendril at https://github.com/servo/html5ever/tree/zbuf/ with the idea of having unsafe much more limited, self-contained, and documented. But I never quite finished it or ported html5ever to it. Though maybe making yet another unsafe-using library is not a great plan and html5ever should rather use something better maintained like https://crates.io/crates/bytes instead.

[KamilaBorowska]

Oops, didn't realize that into_send is a method. I wonder why did the tests not break...

The intent of PackedUsize was to have an usize whose alignment was 4 to stay compatible with C API. The reason why I did that for non-atomic API only is that packing is incorrect for atomic variables (CPUs generally don't guarantee atomicity without proper alignment).

[KamilaBorowska]

But yeah, there is way too much unsafe in this crate. I believe I did write something to that effect in https://lib.rs/crates/tendril/crev: "There is ridiculous amount of unsafe. I have no idea how anything in this crate works." I did try fixing some things (things that were obviously wrong), but to be fair, it's still one of crates I'm least certain about in my program dependencies (and I use this due to using ammonia).