From 266a7ade6ec40302db347e5402afcb50f28ace68 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 28 Jul 2020 15:56:48 +0200 Subject: [PATCH 1/6] add document on consts in patterns --- const.md | 2 ++ patterns.md | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 patterns.md diff --git a/const.md b/const.md index bd24e4d..2b9133b 100644 --- a/const.md +++ b/const.md @@ -12,6 +12,8 @@ To make this work, we need to ensure [const safety](const_safety.md). Based on this requirement, we allow other constants and [promoteds](promotion.md) to read from constants. This is why the value of a `const` is subject to validity checks. +Constants can also be [used in patterns](patterns.md), which brings its own soundness concerns. + ## References One issue is constants of reference type: diff --git a/patterns.md b/patterns.md new file mode 100644 index 0000000..9ecbe74 --- /dev/null +++ b/patterns.md @@ -0,0 +1,62 @@ +# Constants in patterns + +Constants that pass the [structural equality check](https://github.com/rust-lang/rust/issues/74446) can be used in patterns: +```rust +const FOO: i32 = 13; + +fn is_foo(x: i32) -> bool { + match x { + FOO => true, + _ => false, + } +} +``` +However, that check has some loopholes, so e.g. `&T` can be used in a pattern no matter the `T`. + +A const-pattern is compiled by calling `PartialEq::eq` to compare the subject of the `match` with the constant, +except for TODO where the constant is treated as if it was inlined as a pattern (and the usual `match` tree is constructed). + +## Soundness concerns + +Most of the time there are no extra soundness concerns due to const-patterns; except for surprising behavior nothing can go wrong when the structural equality check gives a wrong answer. +However, there is one exception: some constants participate in exhaustiveness checking. +If a pattern is incorrectly considered exhaustive, that leads to a critical soundness bug. + +Exhaustiveness checking is done for constants of type TODO, as well as `&[u8]` and `&str` (but no other types with references). +This means we can write: +```rust +#![feature(exclusive_range_pattern)] +#![feature(half_open_range_patterns)] +const PAT: &[u8] = &[0]; + +pub fn test(x: &[u8]) -> bool { + match x { + PAT => true, + &[] => false, + &[1..] => false, + &[_, _, ..] => false + } +} +``` + +To ensure soundness of exhaustiveness checking, it is crucial that all data considered this check is fully immutable. +In particular, for constants of reference type, it is important that they only point to immutable data. +For this reason, the static const checks reject references to `static` items. +This is a new soundness concern that otherwise does not come up during CTFE (where *reading from* a mutable `static` during const initialization is a problem, but just referencing a mutable `static` is not). +A more precise check could be possible, but is non-trivial: even an immutable `static` could point to a mutable `static`; that would have to be excluded. +(We could, in principle, also rely on it being UB to have a shared reference to mutable memory, but for now we prefer not to rely on the aliasing model like that---aliasing of global pointers is a tricky subject.) + +*Dynamic check.* +To dynamically ensure constants only point to read-only data, we maintain a global invariant: +pointers to a `static` (including memory that was created as part of interning a `static`) may never "leak" to a non-`static`. +All memory outside of `static`s is marked as immutable. +As a consequence, non-`static`s recursively only point to immutable memory. + +This check is uncomfortably close to the static checks, and fragile due to its reliance on a global invariant. +Longer-term, it would be better to move to a more local check, by making validation check if consts are recursively read-only. +This check (unlike our current validation) would have to descend through pointers to `static`. + +An alternative could be to wait for the [value tree proposal](https://github.com/rust-lang/compiler-team/issues/323) to be implemented. +Then we could check during value tree construction that all memory is read-only (if we use the CTFE machine in `const`-mode, that will happen implicitly), and thus we know for sure we can rely on the value tree to be sound to use for exhaustiveness checking. +How exactly this looks like depends on how we resolve [pattern matching allowing many things that do not fit a value tree](https://github.com/rust-lang/rust/issues/74446#issuecomment-663439899). +However, if only the well-behaved part of the value tree is used for exhaustiveness checking, handling these extra patterns should not interact closely with the soundness concerns discussed here. From 81a6b8ee0ac7c9eead5776d3d093d382c9685a1c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 28 Jul 2020 17:21:25 +0200 Subject: [PATCH 2/6] link to reading-mutable-static soundness concern --- patterns.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/patterns.md b/patterns.md index 9ecbe74..2c58026 100644 --- a/patterns.md +++ b/patterns.md @@ -42,7 +42,8 @@ pub fn test(x: &[u8]) -> bool { To ensure soundness of exhaustiveness checking, it is crucial that all data considered this check is fully immutable. In particular, for constants of reference type, it is important that they only point to immutable data. For this reason, the static const checks reject references to `static` items. -This is a new soundness concern that otherwise does not come up during CTFE (where *reading from* a mutable `static` during const initialization is a problem, but just referencing a mutable `static` is not). +This is a new soundness concern that otherwise does not come up during CTFE. +Note that this is orthogonal to the concern of [*reading from* a mutable `static`](const.md#reading-statics) during const initialization, which is a problem for all `const` (even when they are not used in patterns) because it breaks the "inlining" property. A more precise check could be possible, but is non-trivial: even an immutable `static` could point to a mutable `static`; that would have to be excluded. (We could, in principle, also rely on it being UB to have a shared reference to mutable memory, but for now we prefer not to rely on the aliasing model like that---aliasing of global pointers is a tricky subject.) From e8414f11f26e4213cc43555ae61129cc1411a6bc Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 29 Jul 2020 09:45:02 +0200 Subject: [PATCH 3/6] add some links to code --- patterns.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/patterns.md b/patterns.md index 2c58026..1357f69 100644 --- a/patterns.md +++ b/patterns.md @@ -1,6 +1,6 @@ # Constants in patterns -Constants that pass the [structural equality check](https://github.com/rust-lang/rust/issues/74446) can be used in patterns: +Constants that pass the [structural equality check][struct-eq] can be used in patterns: ```rust const FOO: i32 = 13; @@ -13,9 +13,12 @@ fn is_foo(x: i32) -> bool { ``` However, that check has some loopholes, so e.g. `&T` can be used in a pattern no matter the `T`. -A const-pattern is compiled by calling `PartialEq::eq` to compare the subject of the `match` with the constant, +A const-pattern is compiled by [calling `PartialEq::eq`][compile-partial-eq] to compare the subject of the `match` with the constant, except for TODO where the constant is treated as if it was inlined as a pattern (and the usual `match` tree is constructed). +[struct-eq]: https://github.com/rust-lang/rust/blob/2c28244cf0fc9868f55070e55b8f332d196eaf3f/src/librustc_mir_build/hair/pattern/const_to_pat.rs#L121 +[compile-partial-eq]: https://github.com/rust-lang/rust/blob/2c28244cf0fc9868f55070e55b8f332d196eaf3f/src/librustc_mir_build/build/matches/test.rs#L355 + ## Soundness concerns Most of the time there are no extra soundness concerns due to const-patterns; except for surprising behavior nothing can go wrong when the structural equality check gives a wrong answer. From c67ebb514020443f3253af617c05c1c4e1f9911c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 10 Aug 2020 14:37:24 +0200 Subject: [PATCH 4/6] document for which consts we do exhaustiveness checking --- patterns.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patterns.md b/patterns.md index 1357f69..debb3c9 100644 --- a/patterns.md +++ b/patterns.md @@ -25,7 +25,7 @@ Most of the time there are no extra soundness concerns due to const-patterns; ex However, there is one exception: some constants participate in exhaustiveness checking. If a pattern is incorrectly considered exhaustive, that leads to a critical soundness bug. -Exhaustiveness checking is done for constants of type TODO, as well as `&[u8]` and `&str` (but no other types with references). +Exhaustiveness checking is done for all constants that do not have reference type, as well as `&[u8]` and `&str`. This means we can write: ```rust #![feature(exclusive_range_pattern)] From bd37599e594db12aa87215700cf7d4114184bf0b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 10 Aug 2020 14:42:56 +0200 Subject: [PATCH 5/6] Explain how patterns are compiled Co-authored-by: Oliver Scherer --- patterns.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/patterns.md b/patterns.md index debb3c9..7e2b47f 100644 --- a/patterns.md +++ b/patterns.md @@ -13,8 +13,7 @@ fn is_foo(x: i32) -> bool { ``` However, that check has some loopholes, so e.g. `&T` can be used in a pattern no matter the `T`. -A const-pattern is compiled by [calling `PartialEq::eq`][compile-partial-eq] to compare the subject of the `match` with the constant, -except for TODO where the constant is treated as if it was inlined as a pattern (and the usual `match` tree is constructed). +Any reference type const-pattern is compiled by [calling `PartialEq::eq`][compile-partial-eq] to compare the subject of the `match` with the constant. Const-patterns with other types (enum, struct, tuple, array) are treated as if the constant was inlined as a pattern (and the usual `match` tree is constructed). [struct-eq]: https://github.com/rust-lang/rust/blob/2c28244cf0fc9868f55070e55b8f332d196eaf3f/src/librustc_mir_build/hair/pattern/const_to_pat.rs#L121 [compile-partial-eq]: https://github.com/rust-lang/rust/blob/2c28244cf0fc9868f55070e55b8f332d196eaf3f/src/librustc_mir_build/build/matches/test.rs#L355 From 18f3020b9fa40e21852b6ef0dce7ac0fab92946d Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 10 Aug 2020 14:43:53 +0200 Subject: [PATCH 6/6] compare with RFC 1445 --- patterns.md | 1 + 1 file changed, 1 insertion(+) diff --git a/patterns.md b/patterns.md index 7e2b47f..fa1b344 100644 --- a/patterns.md +++ b/patterns.md @@ -14,6 +14,7 @@ fn is_foo(x: i32) -> bool { However, that check has some loopholes, so e.g. `&T` can be used in a pattern no matter the `T`. Any reference type const-pattern is compiled by [calling `PartialEq::eq`][compile-partial-eq] to compare the subject of the `match` with the constant. Const-patterns with other types (enum, struct, tuple, array) are treated as if the constant was inlined as a pattern (and the usual `match` tree is constructed). +[RFC 1445](https://github.com/rust-lang/rfcs/blob/master/text/1445-restrict-constants-in-patterns.md) lays the groundwork for changing this in the future to always using `PartialEq::eq`, but no decision either way has been made yet. [struct-eq]: https://github.com/rust-lang/rust/blob/2c28244cf0fc9868f55070e55b8f332d196eaf3f/src/librustc_mir_build/hair/pattern/const_to_pat.rs#L121 [compile-partial-eq]: https://github.com/rust-lang/rust/blob/2c28244cf0fc9868f55070e55b8f332d196eaf3f/src/librustc_mir_build/build/matches/test.rs#L355