Alignment is not accounted for

[BurntSushi]

As far as I can tell, alignment isn't accounted for at all in this crate. You seem to assert that converting a *const u8 to a *const T is OK even when T has an alignment greater than 1. For example, your guarded_transmute function checks the sizes, but not alignment, and then proceeds to dereference a *const T pointer even though it might have an alignment greater than 1. That's UB. This is then used as an implementation of functions like guarded_transmute_pod to declare that it is safe, but it is in fact not safe.

[BurntSushi]

It may be possible salvage the safety guarantees you want to provide by using explicit unaligned loads: https://doc.rust-lang.org/std/ptr/fn.read_unaligned.html

[Enet4]

@BurntSushi Thanks for the finding. Admittedly, this crate tends to sail through some murky laundry-eating waters.

I recall that a previous version of the crate would perform the check with align_of instead of size_of, but eventually we realized that this wouldn't work for compound structs. Even so, this wasn't an actual alignment check of the byte slice, but a check for the target type's alignment with the length of the slice. Would it become safe if we ensure that the address pointed by the slice is a multiple of the target type's alignment?

I made a few tests that would read unaligned slices in #21. They all seem to pass, but I understand that this doesn't prove memory safety (good behaviour in UB is still possible...).

[BurntSushi]

Would it become safe if we ensure that the address pointed by the slice is a multiple of the target type's alignment?

This seems plausible, yes.

Note that I am not an expert in Rust's UB. Honestly, probably very few people are, because we don't have a formal semantics for it yet. I would strongly urge you to put a warning somewhere around this crate that it hasn't been thoroughly vetted, but that such vetting would be welcome. :-)

In general, I like the ideas motivating this crate, but I think we need to be super careful about what we're claiming is safe and what isn't.

[BurntSushi]

I made a few tests that would read unaligned slices in #21. They all seem to pass, but I understand that this doesn't prove memory safety (good behaviour in UB is still possible...).

Emphatically, yes. And it may vary by platform.

[Enet4]

Note that I am not an expert in Rust's UB. Honestly, probably very few people are, because we don't have a formal semantics for it yet. I would strongly urge you to put a warning somewhere around this crate that it hasn't been thoroughly vetted, but that such vetting would be welcome. :-)

Indeed. @nabijaczleweli can you work on this?

In general, I like the ideas motivating this crate, but I think we need to be super careful about what we're claiming is safe and what isn't.

I agree. This crate emerged with some exciting use cases, but once the first versions were laid out, we did realize that making it safe was not very trivial. The feedback from better informed members of the community has been invaluable so far, and I hope to ensure that this never hits 1.0.0 until we're sure that the non-unsafe API is really safe.

[nabijaczleweli]

this wouldn't work for compound structs

Was #[repr(C)] structs, actually.

put a warning somewhere

Cool and good. (Timing: post-PRs and pre-release.)

ensure that this never hits 1.0.0 until we're sure that the non-unsafe API is really safe

Well, since we've merged some magenta changes into master, which are doomed to release, we've to bump major (semver is cool and good). And, since major is 0, it will be 1. Frankly, I don't see people's obsession with v1.0.0 meaning something in semver-enforced environments.

[Enet4]

Well, since we've merged some magenta changes into master, which are doomed to release, we've to bump major (semver is cool and good). And, since major is 0, it will be 1.

That's not quite how semver works though. 0-major versions are pre-releases, so breaking changes are always permitted.

I wouldn't also underestimate the message that we are passing with a major release. The Rust API guidelines also suggest that a library should avoid launching a major release until all of its dependencies are also "stable" (>=1.0.0) releases. Even if we're placing a message in the documentation, I really don't want to give users of this crate this false sense of security.

[nabijaczleweli]

That 0-major thing sounds like a specialcase and specialcases are the incarnation of satan itself :angery:

The Rust API Guidelines? Where is that? Also, "avoid launching pre-release till all deps are >= 1.0.0" makes little sense. Also, futures-rs seems to absolutely not follow that and it's an official rust-lang-* group repo so 🤔 (also, remember the absolute shitshow that was breaking 0.1? that was both (a) comedy gold and (b) an illustration on why some standards (or at least parts themof) are better left untouched.

Anyway, re: #21. If you're really opposed to always running the tests (which, I admit, may be kinda dicey), we can mark them as #[ignore] and slam a warning on them to make them really stand out that you should only do that in a well-ventilated environment or a fume hood (like a CI server).

[Enet4]

On phone right now, so I'll be brief:

1. C-STABLE.
2. Two wrongs don't make a right. Also, where did you get that? futures is at version 0.2.0, it's definitely not a stable release yet.
3. It was a mistake that had to be addressed. It's not like people had the crate automatically updated to a breaking version. Besides, that was increasingly bad because it made us realize that we were far from attaining the feature that the crate was originally designed for: safe transmutation. We should strive towards no UB through our safe functions, and until then we ought to stick to pre-releases.

[nabijaczleweli]

Released in v0.9.0.