miri does not detect reading undefined bytes if simple unions are involved #51330
Labels
Comments
oli-obk
added
I-unsound
|
not really unsound, because you already were doing UB via unsafe code. Just miri should be better at detecting it |
bors
added a commit
that referenced
this issue
Jul 19, 2018
Do a basic sanity check for all constant values ## Motivation and high level overview There has been some back and forth in this PR between @RalfJung and me in here about the motivation for this change and the stance it takes on unsafe coding guidelines. The initial implementation ran its checks on every value read (so `*x`, `y = x`, ...). In unsafe code that isn't reasonable, because we might be invalidating invariants for a short time in order to build up a proper value. The current implementation is a lint that runs its checks statics and constants. There is no need to check array lengths and enum variants, because it's a hard error to end up with anything but a number, and that one even has to have the required bits to be defined. ## What checks are done? * Some type related checks * `char` needs to be a correct unicode character as defined by `char::from_u32` * A reference to a ZST must have the correct alignment (and be nonzero) * A reference to anything is dereferenced and its value is checked * Layout checks use the information from `ty::Layout` to check * all fields of structs * all elements of arrays * enum discriminants * the fields of an enum variant (the variant is decided by the discriminant) * whether any union field succeeds in being checked (if none match the memory pattern, the check fails) * if the value is in the range described by the layout (e.g. for `NonZero*` types) Changing the layout of a type will thus automatically cause the checks to check for the new layout. fixes #51330 fixes #51471 cc @RalfJung r? @eddyb
Mark-Simulacrum
added a commit
to Mark-Simulacrum/rust
that referenced
this issue
Jul 27, 2018
…matsakis Do a basic sanity check for all constant values ## Motivation and high level overview There has been some back and forth in this PR between @RalfJung and me in here about the motivation for this change and the stance it takes on unsafe coding guidelines. The initial implementation ran its checks on every value read (so `*x`, `y = x`, ...). In unsafe code that isn't reasonable, because we might be invalidating invariants for a short time in order to build up a proper value. The current implementation is a lint that runs its checks statics and constants. There is no need to check array lengths and enum variants, because it's a hard error to end up with anything but a number, and that one even has to have the required bits to be defined. ## What checks are done? * Some type related checks * `char` needs to be a correct unicode character as defined by `char::from_u32` * A reference to a ZST must have the correct alignment (and be nonzero) * A reference to anything is dereferenced and its value is checked * Layout checks use the information from `ty::Layout` to check * all fields of structs * all elements of arrays * enum discriminants * the fields of an enum variant (the variant is decided by the discriminant) * whether any union field succeeds in being checked (if none match the memory pattern, the check fails) * if the value is in the range described by the layout (e.g. for `NonZero*` types) Changing the layout of a type will thus automatically cause the checks to check for the new layout. fixes rust-lang#51330 fixes rust-lang#51471 cc @RalfJung r? @eddyb
bors
added a commit
that referenced
this issue
Jul 29, 2018
Do a basic sanity check for all constant values ## Motivation and high level overview There has been some back and forth in this PR between @RalfJung and me in here about the motivation for this change and the stance it takes on unsafe coding guidelines. The initial implementation ran its checks on every value read (so `*x`, `y = x`, ...). In unsafe code that isn't reasonable, because we might be invalidating invariants for a short time in order to build up a proper value. The current implementation is a lint that runs its checks statics and constants. There is no need to check array lengths and enum variants, because it's a hard error to end up with anything but a number, and that one even has to have the required bits to be defined. ## What checks are done? * Some type related checks * `char` needs to be a correct unicode character as defined by `char::from_u32` * A reference to a ZST must have the correct alignment (and be nonzero) * A reference to anything is dereferenced and its value is checked * Layout checks use the information from `ty::Layout` to check * all fields of structs * all elements of arrays * enum discriminants * the fields of an enum variant (the variant is decided by the discriminant) * whether any union field succeeds in being checked (if none match the memory pattern, the check fails) * if the value is in the range described by the layout (e.g. for `NonZero*` types) Changing the layout of a type will thus automatically cause the checks to check for the new layout. fixes #51330 fixes #51471 cc @RalfJung r? @eddyb
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
http://play.rust-lang.org/?gist=99912c4c87d4d676817c8a7263289c9b&version=undefined&mode=undefined
shows an example where miri returns zeroes instead of erroring because of undefined bytes being read.
The text was updated successfully, but these errors were encountered: