From c5b26cd3ea0d7b012f1a10395e0b2e01c06d0335 Mon Sep 17 00:00:00 2001 From: The Miri Conjob Bot Date: Wed, 26 Jul 2023 06:24:12 +0000 Subject: [PATCH 01/12] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 4958c377215e2..d18199ad3f41d 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -cec34a43b1b14f4e39363f3b283d7ac4f593ee81 +98db99f5f6273d95497dd83d1b3a62c2c00292b1 From 363fce5a9261cca041b4b4433b196bbee4e354dd Mon Sep 17 00:00:00 2001 From: The Miri Conjob Bot Date: Wed, 26 Jul 2023 06:34:30 +0000 Subject: [PATCH 02/12] fmt --- src/tools/miri/src/shims/intrinsics/simd.rs | 3 ++- src/tools/miri/tests/pass/intrinsics.rs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/tools/miri/src/shims/intrinsics/simd.rs b/src/tools/miri/src/shims/intrinsics/simd.rs index 3afe5214f08b2..bddfe58ee3cd8 100644 --- a/src/tools/miri/src/shims/intrinsics/simd.rs +++ b/src/tools/miri/src/shims/intrinsics/simd.rs @@ -528,7 +528,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { assert_eq!(dest_len, mask_len); for i in 0..dest_len { - let passthru = this.read_immediate(&this.project_index(&passthru, i)?.into())?; + let passthru = + this.read_immediate(&this.project_index(&passthru, i)?.into())?; let ptr = this.read_immediate(&this.project_index(&ptrs, i)?.into())?; let mask = this.read_immediate(&this.project_index(&mask, i)?.into())?; let dest = this.project_index(&dest, i)?; diff --git a/src/tools/miri/tests/pass/intrinsics.rs b/src/tools/miri/tests/pass/intrinsics.rs index a6a4a06f8600b..8c6eeab22195c 100644 --- a/src/tools/miri/tests/pass/intrinsics.rs +++ b/src/tools/miri/tests/pass/intrinsics.rs @@ -3,7 +3,7 @@ //! Tests for various intrinsics that do not fit anywhere else. use std::intrinsics; -use std::mem::{size_of, size_of_val, size_of_val_raw, discriminant}; +use std::mem::{discriminant, size_of, size_of_val, size_of_val_raw}; struct Bomb; From 58cb4849d3652ae583dae5813a9015ff0c3959b3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 26 Jul 2023 09:09:57 +0200 Subject: [PATCH 03/12] move nightly cron job a little earlier --- src/tools/miri/.github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/.github/workflows/ci.yml b/src/tools/miri/.github/workflows/ci.yml index 452677d914657..462c921d722e6 100644 --- a/src/tools/miri/.github/workflows/ci.yml +++ b/src/tools/miri/.github/workflows/ci.yml @@ -10,7 +10,7 @@ on: branches: - 'master' schedule: - - cron: '6 6 * * *' # At 6:06 UTC every day. + - cron: '11 5 * * *' # At 5:11 UTC every day. env: CARGO_UNSTABLE_SPARSE_REGISTRY: 'true' From b452f50317bbea8cf6eb7d9af2d8873329244cd3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 26 Jul 2023 09:10:50 +0200 Subject: [PATCH 04/12] sparse registry has been stable for a bit now --- src/tools/miri/.github/workflows/ci.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/tools/miri/.github/workflows/ci.yml b/src/tools/miri/.github/workflows/ci.yml index 462c921d722e6..f4ad966236c6a 100644 --- a/src/tools/miri/.github/workflows/ci.yml +++ b/src/tools/miri/.github/workflows/ci.yml @@ -12,9 +12,6 @@ on: schedule: - cron: '11 5 * * *' # At 5:11 UTC every day. -env: - CARGO_UNSTABLE_SPARSE_REGISTRY: 'true' - defaults: run: shell: bash From d81ab0d917c83f76e6f436ab8308a0249f9b9ba0 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 26 Jul 2023 09:11:25 +0200 Subject: [PATCH 05/12] move CI var uses after their declaration --- src/tools/miri/.github/workflows/ci.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/tools/miri/.github/workflows/ci.yml b/src/tools/miri/.github/workflows/ci.yml index f4ad966236c6a..042bb9bbd2ca9 100644 --- a/src/tools/miri/.github/workflows/ci.yml +++ b/src/tools/miri/.github/workflows/ci.yml @@ -18,10 +18,6 @@ defaults: jobs: build: - runs-on: ${{ matrix.os }} - env: - RUST_BACKTRACE: 1 - HOST_TARGET: ${{ matrix.host_target }} strategy: fail-fast: false matrix: @@ -32,6 +28,10 @@ jobs: host_target: x86_64-apple-darwin - os: windows-latest host_target: i686-pc-windows-msvc + runs-on: ${{ matrix.os }} + env: + RUST_BACKTRACE: 1 + HOST_TARGET: ${{ matrix.host_target }} steps: - uses: actions/checkout@v3 From 38665a12be721cb19a745bdbde6afd5c058d2b20 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 26 Jul 2023 11:08:41 +0200 Subject: [PATCH 06/12] normalize tree borrow diagnostics across targets --- src/tools/miri/tests/compiletest.rs | 2 ++ .../reserved/cell-protected-write.stderr | 4 ++-- .../reserved/int-protected-write.stderr | 4 ++-- .../tree_borrows/cell-alternate-writes.stderr | 8 +++---- .../pass/tree_borrows/end-of-protector.stderr | 16 ++++++------- .../tests/pass/tree_borrows/formatting.stderr | 8 +++---- .../pass/tree_borrows/reborrow-is-read.stderr | 8 +++---- .../tests/pass/tree_borrows/reserved.stderr | 24 +++++++++---------- .../pass/tree_borrows/unique.default.stderr | 12 +++++----- .../pass/tree_borrows/unique.uniq.stderr | 12 +++++----- .../tree_borrows/vec_unique.default.stderr | 4 ++-- .../pass/tree_borrows/vec_unique.uniq.stderr | 4 ++-- 12 files changed, 54 insertions(+), 52 deletions(-) diff --git a/src/tools/miri/tests/compiletest.rs b/src/tools/miri/tests/compiletest.rs index 59143550253d6..70a15b3fc9315 100644 --- a/src/tools/miri/tests/compiletest.rs +++ b/src/tools/miri/tests/compiletest.rs @@ -190,6 +190,8 @@ regexes! { // erase borrow tags "<[0-9]+>" => "", "<[0-9]+=" => " "$1", // erase whitespace that differs between platforms r" +at (.*\.rs)" => " at $1", // erase generics in backtraces diff --git a/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr b/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr index 5e910779621e3..c77f1492a9728 100644 --- a/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr @@ -1,4 +1,4 @@ -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── @@ -7,7 +7,7 @@ Warning: this tree is indicative only. Some tags may have been hidden. | Re*| │ └─┬── | Re*| │ └──── Strongly protected | Re*| └──── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── error: Undefined Behavior: write access through (y, callee:y, caller:y) is forbidden --> $DIR/cell-protected-write.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/tree_borrows/reserved/int-protected-write.stderr b/src/tools/miri/tests/fail/tree_borrows/reserved/int-protected-write.stderr index e28aac306cd54..ac8788112e908 100644 --- a/src/tools/miri/tests/fail/tree_borrows/reserved/int-protected-write.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/reserved/int-protected-write.stderr @@ -1,4 +1,4 @@ -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── @@ -7,7 +7,7 @@ Warning: this tree is indicative only. Some tags may have been hidden. | Res| │ └─┬── | Res| │ └──── Strongly protected | Res| └──── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── error: Undefined Behavior: write access through (y, callee:y, caller:y) is forbidden --> $DIR/int-protected-write.rs:LL:CC | diff --git a/src/tools/miri/tests/pass/tree_borrows/cell-alternate-writes.stderr b/src/tools/miri/tests/pass/tree_borrows/cell-alternate-writes.stderr index 1eab4685a35f5..f464e0b4f4948 100644 --- a/src/tools/miri/tests/pass/tree_borrows/cell-alternate-writes.stderr +++ b/src/tools/miri/tests/pass/tree_borrows/cell-alternate-writes.stderr @@ -1,12 +1,12 @@ -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── | Re*| └──── -────────────────────────────────────────────────────────────────────── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── | Act| └──── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── diff --git a/src/tools/miri/tests/pass/tree_borrows/end-of-protector.stderr b/src/tools/miri/tests/pass/tree_borrows/end-of-protector.stderr index c20da1a593fc0..265f6dfc9c834 100644 --- a/src/tools/miri/tests/pass/tree_borrows/end-of-protector.stderr +++ b/src/tools/miri/tests/pass/tree_borrows/end-of-protector.stderr @@ -1,11 +1,11 @@ -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── | Res| └─┬── | Res| └──── -────────────────────────────────────────────────────────────────────── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── @@ -13,8 +13,8 @@ Warning: this tree is indicative only. Some tags may have been hidden. | Res| └─┬── | Res| └─┬── | Res| └──── Strongly protected -────────────────────────────────────────────────────────────────────── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── @@ -23,8 +23,8 @@ Warning: this tree is indicative only. Some tags may have been hidden. | Res| │ └─┬── | Res| │ └──── | Res| └──── -────────────────────────────────────────────────────────────────────── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── @@ -33,4 +33,4 @@ Warning: this tree is indicative only. Some tags may have been hidden. | Dis| │ └─┬── | Dis| │ └──── | Act| └──── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── diff --git a/src/tools/miri/tests/pass/tree_borrows/formatting.stderr b/src/tools/miri/tests/pass/tree_borrows/formatting.stderr index effd0d9f9616b..673dae6210d88 100644 --- a/src/tools/miri/tests/pass/tree_borrows/formatting.stderr +++ b/src/tools/miri/tests/pass/tree_borrows/formatting.stderr @@ -1,4 +1,4 @@ -─────────────────────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1.. 2.. 10.. 11..100..101..1000..1001..1024 | Act| Act| Act| Act| Act| Act| Act| Act| Act| └─┬── @@ -7,8 +7,8 @@ Warning: this tree is indicative only. Some tags may have been hidden. |----|----|----| Act|----|?Dis| ----| ?Dis| ----| ├──── |----|----|----|----|----| Frz| ----| ?Dis| ----| ├──── |----|----|----|----|----|----| ----| Act| ----| └──── -─────────────────────────────────────────────────────────────────────────────────────── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── @@ -28,4 +28,4 @@ Warning: this tree is indicative only. Some tags may have been hidden. | Frz| └─┬── | Frz| ├──── | Frz| └──── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── diff --git a/src/tools/miri/tests/pass/tree_borrows/reborrow-is-read.stderr b/src/tools/miri/tests/pass/tree_borrows/reborrow-is-read.stderr index 8c4323b2f7fa7..b23d78a71566e 100644 --- a/src/tools/miri/tests/pass/tree_borrows/reborrow-is-read.stderr +++ b/src/tools/miri/tests/pass/tree_borrows/reborrow-is-read.stderr @@ -1,15 +1,15 @@ -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── | Act| └─┬── | Act| └──── -────────────────────────────────────────────────────────────────────── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── | Act| └─┬── | Frz| ├──── | Res| └──── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── diff --git a/src/tools/miri/tests/pass/tree_borrows/reserved.stderr b/src/tools/miri/tests/pass/tree_borrows/reserved.stderr index afb917002221e..691fe8b77444d 100644 --- a/src/tools/miri/tests/pass/tree_borrows/reserved.stderr +++ b/src/tools/miri/tests/pass/tree_borrows/reserved.stderr @@ -1,5 +1,5 @@ [interior mut + protected] Foreign Read: Re* -> Frz -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── @@ -8,27 +8,27 @@ Warning: this tree is indicative only. Some tags may have been hidden. | Re*| │ └─┬── | Frz| │ └──── | Re*| └──── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── [interior mut] Foreign Read: Re* -> Re* -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 8 | Act| └─┬── | Re*| └─┬── | Re*| ├──── | Re*| └──── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── [interior mut] Foreign Write: Re* -> Re* -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 8 | Act| └─┬── | Act| └─┬── | Re*| ├──── | Act| └──── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── [protected] Foreign Read: Res -> Frz -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── @@ -37,22 +37,22 @@ Warning: this tree is indicative only. Some tags may have been hidden. | Res| │ └─┬── | Frz| │ └──── | Res| └──── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── [] Foreign Read: Res -> Res -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── | Res| └─┬── | Res| ├──── | Res| └──── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── [] Foreign Write: Res -> Dis -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── | Act| └─┬── | Dis| ├──── | Act| └──── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── diff --git a/src/tools/miri/tests/pass/tree_borrows/unique.default.stderr b/src/tools/miri/tests/pass/tree_borrows/unique.default.stderr index 11e05d50f2cfc..f870d3bdec00c 100644 --- a/src/tools/miri/tests/pass/tree_borrows/unique.default.stderr +++ b/src/tools/miri/tests/pass/tree_borrows/unique.default.stderr @@ -1,21 +1,21 @@ -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── | Res| └─┬── | Res| └──── -────────────────────────────────────────────────────────────────────── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── | Act| └─┬── | Act| └──── -────────────────────────────────────────────────────────────────────── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── | Act| └─┬── | Act| └──── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── diff --git a/src/tools/miri/tests/pass/tree_borrows/unique.uniq.stderr b/src/tools/miri/tests/pass/tree_borrows/unique.uniq.stderr index 5008b66741aa4..9ab6b879aa762 100644 --- a/src/tools/miri/tests/pass/tree_borrows/unique.uniq.stderr +++ b/src/tools/miri/tests/pass/tree_borrows/unique.uniq.stderr @@ -1,24 +1,24 @@ -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── | Res| └─┬── | Res| └─┬── |----| └──── -────────────────────────────────────────────────────────────────────── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── | Act| └─┬── | Act| └─┬── | Act| └──── -────────────────────────────────────────────────────────────────────── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act| └─┬── | Act| └─┬── | Act| └─┬── | Dis| └──── -────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── diff --git a/src/tools/miri/tests/pass/tree_borrows/vec_unique.default.stderr b/src/tools/miri/tests/pass/tree_borrows/vec_unique.default.stderr index f1af1ea3d8bdf..a7712ae91fba8 100644 --- a/src/tools/miri/tests/pass/tree_borrows/vec_unique.default.stderr +++ b/src/tools/miri/tests/pass/tree_borrows/vec_unique.default.stderr @@ -1,6 +1,6 @@ -───────────────────────────────────────────────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 2 | Act| └─┬── | Res| └──── -───────────────────────────────────────────────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── diff --git a/src/tools/miri/tests/pass/tree_borrows/vec_unique.uniq.stderr b/src/tools/miri/tests/pass/tree_borrows/vec_unique.uniq.stderr index 00ff1ee00ebe3..e9f1cb3b1ed93 100644 --- a/src/tools/miri/tests/pass/tree_borrows/vec_unique.uniq.stderr +++ b/src/tools/miri/tests/pass/tree_borrows/vec_unique.uniq.stderr @@ -1,8 +1,8 @@ -────────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. 0.. 2 | Act| └─┬── |----| └─┬── |----| └─┬── |----| └──── -────────────────────────────────────────────────────────────────────────── +────────────────────────────────────────────────── From efc2af48510b52cb7a917b9e482d0a6abe5a0617 Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Wed, 26 Jul 2023 13:00:53 +0200 Subject: [PATCH 07/12] fix protectors so that all reads actually commute --- .../tree_borrows/diagnostics.rs | 15 ++-- .../src/borrow_tracker/tree_borrows/perms.rs | 75 +++++++------------ .../src/borrow_tracker/tree_borrows/tree.rs | 4 +- .../both_borrows/aliasing_mut4.tree.stderr | 4 +- .../box_noalias_violation.tree.stderr | 4 +- .../both_borrows/illegal_write6.tree.stderr | 4 +- .../invalidate_against_protector2.tree.stderr | 4 +- .../invalidate_against_protector3.tree.stderr | 4 +- .../newtype_pair_retagging.tree.stderr | 4 +- .../newtype_retagging.tree.stderr | 4 +- .../arg_inplace_mutate.tree.stderr | 4 +- .../arg_inplace_observe_during.tree.stderr | 4 +- .../return_pointer_aliasing.tree.stderr | 4 +- .../fail/tree_borrows/outside-range.stderr | 4 +- .../reserved/cell-protected-write.stderr | 4 +- .../reserved/int-protected-write.stderr | 4 +- 16 files changed, 59 insertions(+), 87 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs index 3b2d6f9608ee4..5fb298a54af33 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs @@ -227,10 +227,10 @@ pub(super) enum TransitionError { ChildAccessForbidden(Permission), /// A protector was triggered due to an invalid transition that loses /// too much permissions. - /// For example, if a protected tag goes from `Active` to `Frozen` due - /// to a foreign write this will produce a `ProtectedTransition(PermTransition(Active, Frozen))`. + /// For example, if a protected tag goes from `Active` to `Disabled` due + /// to a foreign write this will produce a `ProtectedDisabled(Active)`. /// This kind of error can only occur on foreign accesses. - ProtectedTransition(PermTransition), + ProtectedDisabled(Permission), /// Cannot deallocate because some tag in the allocation is strongly protected. /// This kind of error can only occur on deallocations. ProtectedDealloc, @@ -302,7 +302,7 @@ impl TbError<'_> { )); (title, details, conflicting_tag_name) } - ProtectedTransition(transition) => { + ProtectedDisabled(before_disabled) => { let conflicting_tag_name = "protected"; let access = cause.print_as_access(/* is_foreign */ true); let details = vec![ @@ -310,12 +310,9 @@ impl TbError<'_> { "the accessed tag {accessed} is foreign to the {conflicting_tag_name} tag {conflicting} (i.e., it is not a child)" ), format!( - "this {access} would cause the {conflicting_tag_name} tag {conflicting} to transition {transition}" - ), - format!( - "this transition would be {loss}, which is not allowed for protected tags", - loss = transition.summary(), + "this {access} would cause the {conflicting_tag_name} tag {conflicting} currently {before_disabled} to become Disabled" ), + format!("protected tags must not become Disabled"), ]; (title, details, conflicting_tag_name) } diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs index 362070f18578d..f68da455378b8 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs @@ -72,7 +72,12 @@ mod transition { // accesses, since the data is not being mutated. Hence the `{ .. }` res @ Reserved { .. } if !protected => res, Reserved { .. } => Frozen, // protected reserved - Active => Frozen, + Active => + if protected { + Disabled + } else { + Frozen + }, non_writeable @ (Frozen | Disabled) => non_writeable, }) } @@ -189,34 +194,9 @@ impl PermTransition { Permission { inner: self.from } } - /// Determines whether a transition that occured is compatible with the presence - /// of a Protector. This is not included in the `transition` functions because - /// it would distract from the few places where the transition is modified - /// because of a protector, but not forbidden. - /// - /// Note: this is not in charge of checking that there *is* a protector, - /// it should be used as - /// ``` - /// let no_protector_error = if is_protected(tag) { - /// transition.is_allowed_by_protector() - /// }; - /// ``` - pub fn is_allowed_by_protector(&self) -> bool { - assert!(self.is_possible()); - match (self.from, self.to) { - _ if self.from == self.to => true, - // It is always a protector violation to not be readable anymore - (_, Disabled) => false, - // In the case of a `Reserved` under a protector, both transitions - // `Reserved => Active` and `Reserved => Frozen` can legitimately occur. - // The first is standard (Child Write), the second is for Foreign Writes - // on protected Reserved where we must ensure that the pointer is not - // written to in the future. - (Reserved { .. }, Active) | (Reserved { .. }, Frozen) => true, - // This pointer should have stayed writeable for the whole function - (Active, Frozen) => false, - _ => unreachable!("Transition {} should never be possible", self), - } + /// Determines if this transition would disable the permission. + pub fn produces_disabled(self) -> bool { + self.to == Disabled } } @@ -298,14 +278,15 @@ pub mod diagnostics { /// /// This function assumes that its arguments apply to the same location /// and that they were obtained during a normal execution. It will panic otherwise. - /// - `err` cannot be a `ProtectedTransition(_)` of a noop transition, as those - /// never trigger protectors; /// - all transitions involved in `self` and `err` should be increasing /// (Reserved < Active < Frozen < Disabled); /// - between `self` and `err` the permission should also be increasing, /// so all permissions inside `err` should be greater than `self.1`; /// - `Active` and `Reserved` cannot cause an error due to insufficient permissions, /// so `err` cannot be a `ChildAccessForbidden(_)` of either of them; + /// - `err` should not be `ProtectedDisabled(Disabled)`, because the protected + /// tag should not have been `Disabled` in the first place (if this occurs it means + /// we have unprotected tags that become protected) pub(in super::super) fn is_relevant(&self, err: TransitionError) -> bool { // NOTE: `super::super` is the visibility of `TransitionError` assert!(self.is_possible()); @@ -342,17 +323,16 @@ pub mod diagnostics { unreachable!("permissions between self and err must be increasing"), } } - TransitionError::ProtectedTransition(forbidden) => { - assert!(!forbidden.is_noop()); + TransitionError::ProtectedDisabled(before_disabled) => { // Show how we got to the starting point of the forbidden transition, // but ignore what came before. // This eliminates transitions like `Reserved -> Active` // when the error is a `Frozen -> Disabled`. - match (self.to, forbidden.from, forbidden.to) { + match (self.to, before_disabled.inner) { // We absolutely want to know where it was activated. - (Active, Active, Frozen | Disabled) => true, + (Active, Active) => true, // And knowing where it became Frozen is also important. - (Frozen, Frozen, Disabled) => true, + (Frozen, Frozen) => true, // If the error is a transition `Frozen -> Disabled`, then we don't really // care whether before that was `Reserved -> Active -> Frozen` or // `Reserved -> Frozen` or even `Frozen` directly. @@ -360,27 +340,22 @@ pub mod diagnostics { // - created as Frozen, then Frozen -> Disabled is forbidden // - created as Reserved, later became Frozen, then Frozen -> Disabled is forbidden // In both cases the `Reserved -> Active` part is inexistant or irrelevant. - (Active, Frozen, Disabled) => false, + (Active, Frozen) => false, - // `Reserved -> Frozen` does not trigger protectors. - (_, Reserved { .. }, Frozen) => - unreachable!("this transition cannot cause an error"), + (_, Disabled) => + unreachable!( + "permission that results in Disabled should not itself be Disabled in the first place" + ), // No transition has `Reserved` as its `.to` unless it's a noop. - (Reserved { .. }, _, _) => unreachable!("self is a noop transition"), - (_, Disabled, Disabled) | (_, Frozen, Frozen) | (_, Active, Active) => - unreachable!("err contains a noop transition"), + (Reserved { .. }, _) => unreachable!("self is a noop transition"), // Permissions only evolve in the order `Reserved -> Active -> Frozen -> Disabled`, // so permissions found must be increasing in the order // `self.from < self.to <= forbidden.from < forbidden.to`. - (Disabled, Reserved { .. } | Active | Frozen, _) - | (Frozen, Reserved { .. } | Active, _) - | (Active, Reserved { .. }, _) => + (Disabled, Reserved { .. } | Active | Frozen) + | (Frozen, Reserved { .. } | Active) + | (Active, Reserved { .. }) => unreachable!("permissions between self and err must be increasing"), - (_, Disabled, Reserved { .. } | Active | Frozen) - | (_, Frozen, Reserved { .. } | Active) - | (_, Active, Reserved { .. }) => - unreachable!("permissions within err must be increasing"), } } // We don't care because protectors evolve independently from diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs index 9e8b1e1189918..d6e4700101404 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs @@ -456,9 +456,9 @@ impl<'tcx> Tree { if protected // Can't trigger Protector on uninitialized locations && old_state.initialized - && !transition.is_allowed_by_protector() + && transition.produces_disabled() { - return Err(TransitionError::ProtectedTransition(transition)); + return Err(TransitionError::ProtectedDisabled(old_perm)); } // Record the event as part of the history if !transition.is_noop() { diff --git a/src/tools/miri/tests/fail/both_borrows/aliasing_mut4.tree.stderr b/src/tools/miri/tests/fail/both_borrows/aliasing_mut4.tree.stderr index 4102c718af83f..8f278d0184a59 100644 --- a/src/tools/miri/tests/fail/both_borrows/aliasing_mut4.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/aliasing_mut4.tree.stderr @@ -6,8 +6,8 @@ LL | ptr::write(dest, src); | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag is foreign to the protected tag (i.e., it is not a child) - = help: this foreign write access would cause the protected tag to transition from Frozen to Disabled - = help: this transition would be a loss of read permissions, which is not allowed for protected tags + = help: this foreign write access would cause the protected tag currently Frozen to become Disabled + = help: protected tags must not become Disabled help: the accessed tag was created here --> $DIR/aliasing_mut4.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/both_borrows/box_noalias_violation.tree.stderr b/src/tools/miri/tests/fail/both_borrows/box_noalias_violation.tree.stderr index 1d1ed820324a6..6b4e86fa0c522 100644 --- a/src/tools/miri/tests/fail/both_borrows/box_noalias_violation.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/box_noalias_violation.tree.stderr @@ -6,8 +6,8 @@ LL | *y | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag is foreign to the protected tag (i.e., it is not a child) - = help: this foreign read access would cause the protected tag to transition from Active to Frozen - = help: this transition would be a loss of write permissions, which is not allowed for protected tags + = help: this foreign read access would cause the protected tag currently Active to become Disabled + = help: protected tags must not become Disabled help: the accessed tag was created here --> $DIR/box_noalias_violation.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/both_borrows/illegal_write6.tree.stderr b/src/tools/miri/tests/fail/both_borrows/illegal_write6.tree.stderr index 7308d045d193d..5cfbb87fe2765 100644 --- a/src/tools/miri/tests/fail/both_borrows/illegal_write6.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/illegal_write6.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { *y = 2 }; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag is foreign to the protected tag (i.e., it is not a child) - = help: this foreign write access would cause the protected tag to transition from Active to Disabled - = help: this transition would be a loss of read and write permissions, which is not allowed for protected tags + = help: this foreign write access would cause the protected tag currently Active to become Disabled + = help: protected tags must not become Disabled help: the accessed tag was created here --> $DIR/illegal_write6.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector2.tree.stderr b/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector2.tree.stderr index ee64a66cc21ae..ee882db8ac8f5 100644 --- a/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector2.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector2.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { *x = 0 }; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag is foreign to the protected tag (i.e., it is not a child) - = help: this foreign write access would cause the protected tag to transition from Frozen to Disabled - = help: this transition would be a loss of read permissions, which is not allowed for protected tags + = help: this foreign write access would cause the protected tag currently Frozen to become Disabled + = help: protected tags must not become Disabled help: the accessed tag was created here --> $DIR/invalidate_against_protector2.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector3.tree.stderr b/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector3.tree.stderr index e217170afb9ec..30fd8215b6e2a 100644 --- a/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector3.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector3.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { *x = 0 }; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag (root of the allocation) is foreign to the protected tag (i.e., it is not a child) - = help: this foreign write access would cause the protected tag to transition from Frozen to Disabled - = help: this transition would be a loss of read permissions, which is not allowed for protected tags + = help: this foreign write access would cause the protected tag currently Frozen to become Disabled + = help: protected tags must not become Disabled help: the accessed tag was created here --> $DIR/invalidate_against_protector3.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/both_borrows/newtype_pair_retagging.tree.stderr b/src/tools/miri/tests/fail/both_borrows/newtype_pair_retagging.tree.stderr index 00b2708a651c8..0263c906b9b64 100644 --- a/src/tools/miri/tests/fail/both_borrows/newtype_pair_retagging.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/newtype_pair_retagging.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { __rust_dealloc(ptr, layout.size(), layout.align()) } | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag is foreign to the protected tag (i.e., it is not a child) - = help: this deallocation (acting as a foreign write access) would cause the protected tag to transition from Frozen to Disabled - = help: this transition would be a loss of read permissions, which is not allowed for protected tags + = help: this deallocation (acting as a foreign write access) would cause the protected tag currently Frozen to become Disabled + = help: protected tags must not become Disabled help: the accessed tag was created here --> $DIR/newtype_pair_retagging.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/both_borrows/newtype_retagging.tree.stderr b/src/tools/miri/tests/fail/both_borrows/newtype_retagging.tree.stderr index aed2e7abebc0f..710308fb86586 100644 --- a/src/tools/miri/tests/fail/both_borrows/newtype_retagging.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/newtype_retagging.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { __rust_dealloc(ptr, layout.size(), layout.align()) } | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag is foreign to the protected tag (i.e., it is not a child) - = help: this deallocation (acting as a foreign write access) would cause the protected tag to transition from Frozen to Disabled - = help: this transition would be a loss of read permissions, which is not allowed for protected tags + = help: this deallocation (acting as a foreign write access) would cause the protected tag currently Frozen to become Disabled + = help: protected tags must not become Disabled help: the accessed tag was created here --> $DIR/newtype_retagging.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.tree.stderr b/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.tree.stderr index 35c02cc2ebde4..f1bfe138def28 100644 --- a/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.tree.stderr +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { ptr.write(S(0)) }; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag (root of the allocation) is foreign to the protected tag (i.e., it is not a child) - = help: this foreign write access would cause the protected tag to transition from Active to Disabled - = help: this transition would be a loss of read and write permissions, which is not allowed for protected tags + = help: this foreign write access would cause the protected tag currently Active to become Disabled + = help: protected tags must not become Disabled help: the accessed tag was created here --> $DIR/arg_inplace_mutate.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.tree.stderr b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.tree.stderr index cbd76c38f6279..2cf61488e4379 100644 --- a/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.tree.stderr +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { ptr.read() }; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag (root of the allocation) is foreign to the protected tag (i.e., it is not a child) - = help: this foreign read access would cause the protected tag to transition from Active to Frozen - = help: this transition would be a loss of write permissions, which is not allowed for protected tags + = help: this foreign read access would cause the protected tag currently Active to become Disabled + = help: protected tags must not become Disabled help: the accessed tag was created here --> $DIR/arg_inplace_observe_during.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.tree.stderr b/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.tree.stderr index c2c9de3f4eed6..0bda8e0e343bc 100644 --- a/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.tree.stderr +++ b/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { ptr.read() }; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag (root of the allocation) is foreign to the protected tag (i.e., it is not a child) - = help: this foreign read access would cause the protected tag to transition from Active to Frozen - = help: this transition would be a loss of write permissions, which is not allowed for protected tags + = help: this foreign read access would cause the protected tag currently Active to become Disabled + = help: protected tags must not become Disabled help: the accessed tag was created here --> $DIR/return_pointer_aliasing.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/tree_borrows/outside-range.stderr b/src/tools/miri/tests/fail/tree_borrows/outside-range.stderr index 0feafb1a71e65..f490f12b4c88a 100644 --- a/src/tools/miri/tests/fail/tree_borrows/outside-range.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/outside-range.stderr @@ -6,8 +6,8 @@ LL | *y.add(3) = 42; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag is foreign to the protected tag (i.e., it is not a child) - = help: this foreign write access would cause the protected tag to transition from Reserved to Disabled - = help: this transition would be a loss of read and write permissions, which is not allowed for protected tags + = help: this foreign write access would cause the protected tag currently Reserved to become Disabled + = help: protected tags must not become Disabled help: the accessed tag was created here --> $DIR/outside-range.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr b/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr index c77f1492a9728..35361822af0bd 100644 --- a/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr @@ -16,8 +16,8 @@ LL | *y = 1; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag (y, callee:y, caller:y) is foreign to the protected tag (callee:x) (i.e., it is not a child) - = help: this foreign write access would cause the protected tag (callee:x) to transition from Reserved to Disabled - = help: this transition would be a loss of read and write permissions, which is not allowed for protected tags + = help: this foreign write access would cause the protected tag (callee:x) currently Reserved to become Disabled + = help: protected tags must not become Disabled help: the accessed tag was created here --> $DIR/cell-protected-write.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/tree_borrows/reserved/int-protected-write.stderr b/src/tools/miri/tests/fail/tree_borrows/reserved/int-protected-write.stderr index ac8788112e908..83e495ce1932c 100644 --- a/src/tools/miri/tests/fail/tree_borrows/reserved/int-protected-write.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/reserved/int-protected-write.stderr @@ -16,8 +16,8 @@ LL | *y = 0; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag (y, callee:y, caller:y) is foreign to the protected tag (callee:x) (i.e., it is not a child) - = help: this foreign write access would cause the protected tag (callee:x) to transition from Reserved to Disabled - = help: this transition would be a loss of read and write permissions, which is not allowed for protected tags + = help: this foreign write access would cause the protected tag (callee:x) currently Reserved to become Disabled + = help: protected tags must not become Disabled help: the accessed tag was created here --> $DIR/int-protected-write.rs:LL:CC | From 5a460a08ae10920f97176118835c6c5937253cc8 Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Wed, 26 Jul 2023 13:17:24 +0200 Subject: [PATCH 08/12] we correctly check that the perm is not lazy when triggering protectors --- .../tests/pass/tree_borrows/protected_uninit.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 src/tools/miri/tests/pass/tree_borrows/protected_uninit.rs diff --git a/src/tools/miri/tests/pass/tree_borrows/protected_uninit.rs b/src/tools/miri/tests/pass/tree_borrows/protected_uninit.rs new file mode 100644 index 0000000000000..120b2ac46d1d8 --- /dev/null +++ b/src/tools/miri/tests/pass/tree_borrows/protected_uninit.rs @@ -0,0 +1,16 @@ +//@compile-flags: -Zmiri-tree-borrows + +// Protectors should not trigger on uninitialized locations, +// otherwise we can write safe code that triggers UB. +// To test this we do a write access that disables a protected +// location, but the location is actually outside of the protected range. + +// Both x and y are protected here +fn write_second(_x: &mut u8, y: &mut u8) { + *y = 1; +} + +fn main() { + let mut data = (0u8, 1u8); + write_second(&mut data.0, &mut data.1); +} From 3a1a03a1cc56067b54ae198b1426245677954fd7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 27 Jul 2023 09:26:13 +0200 Subject: [PATCH 09/12] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index d18199ad3f41d..1d5dd4d3f6351 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -98db99f5f6273d95497dd83d1b3a62c2c00292b1 +d150dbb067e66f351a0b33a54e7d4b464ef51e47 From bf296bb1ce0796afc4a36ff4588e3389a952c6aa Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 27 Jul 2023 09:29:38 +0200 Subject: [PATCH 10/12] fmt --- src/tools/miri/src/shims/backtrace.rs | 10 ++-------- src/tools/miri/src/shims/time.rs | 5 +---- src/tools/miri/src/shims/unix/macos/foreign_items.rs | 12 +++++++----- src/tools/miri/src/shims/unix/sync.rs | 4 +--- 4 files changed, 11 insertions(+), 20 deletions(-) diff --git a/src/tools/miri/src/shims/backtrace.rs b/src/tools/miri/src/shims/backtrace.rs index e89b2e01a39a8..9f4bb37a4690a 100644 --- a/src/tools/miri/src/shims/backtrace.rs +++ b/src/tools/miri/src/shims/backtrace.rs @@ -194,14 +194,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let filename_alloc = this.allocate_str(&filename, MiriMemoryKind::Rust.into(), Mutability::Mut)?; - this.write_immediate( - name_alloc.to_ref(this), - &this.project_field(&dest, 0)?, - )?; - this.write_immediate( - filename_alloc.to_ref(this), - &this.project_field(&dest, 1)?, - )?; + this.write_immediate(name_alloc.to_ref(this), &this.project_field(&dest, 0)?)?; + this.write_immediate(filename_alloc.to_ref(this), &this.project_field(&dest, 1)?)?; } 1 => { this.write_scalar( diff --git a/src/tools/miri/src/shims/time.rs b/src/tools/miri/src/shims/time.rs index 6667c4d751ffe..935447fd89d14 100644 --- a/src/tools/miri/src/shims/time.rs +++ b/src/tools/miri/src/shims/time.rs @@ -156,10 +156,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let qpc = i64::try_from(duration.as_nanos()).map_err(|_| { err_unsup_format!("programs running longer than 2^63 nanoseconds are not supported") })?; - this.write_scalar( - Scalar::from_i64(qpc), - &this.deref_operand(lpPerformanceCount_op)?, - )?; + this.write_scalar(Scalar::from_i64(qpc), &this.deref_operand(lpPerformanceCount_op)?)?; Ok(Scalar::from_i32(-1)) // return non-zero on success } diff --git a/src/tools/miri/src/shims/unix/macos/foreign_items.rs b/src/tools/miri/src/shims/unix/macos/foreign_items.rs index 3673ca5aee367..5141b29b6835d 100644 --- a/src/tools/miri/src/shims/unix/macos/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/macos/foreign_items.rs @@ -86,7 +86,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { "_NSGetEnviron" => { let [] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; this.write_pointer( - this.machine.env_vars.environ.as_ref().expect("machine must be initialized").ptr, + this.machine + .env_vars + .environ + .as_ref() + .expect("machine must be initialized") + .ptr, dest, )?; } @@ -139,10 +144,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { if written { this.write_null(dest)?; } else { - this.write_scalar( - Scalar::from_u32(size_needed.try_into().unwrap()), - &bufsize, - )?; + this.write_scalar(Scalar::from_u32(size_needed.try_into().unwrap()), &bufsize)?; this.write_int(-1, dest)?; } } diff --git a/src/tools/miri/src/shims/unix/sync.rs b/src/tools/miri/src/shims/unix/sync.rs index 428581801d953..0aeb8ae95a743 100644 --- a/src/tools/miri/src/shims/unix/sync.rs +++ b/src/tools/miri/src/shims/unix/sync.rs @@ -867,9 +867,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { cond_get_clock_id(this, cond_op)?; // This might lead to false positives, see comment in pthread_mutexattr_destroy - this.write_uninit( - &this.deref_operand_as(cond_op, this.libc_ty_layout("pthread_cond_t"))?, - )?; + this.write_uninit(&this.deref_operand_as(cond_op, this.libc_ty_layout("pthread_cond_t"))?)?; // FIXME: delete interpreter state associated with this condvar. Ok(0) From de056754da5a226e779553685a73e723cd0d7085 Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Fri, 28 Jul 2023 18:43:25 +0200 Subject: [PATCH 11/12] exract a perform_access, check read-read commutation exhaustively --- .../tree_borrows/diagnostics.rs | 4 +- .../src/borrow_tracker/tree_borrows/perms.rs | 12 +- .../src/borrow_tracker/tree_borrows/tree.rs | 191 +++++++++++++----- .../both_borrows/aliasing_mut4.tree.stderr | 4 +- .../box_noalias_violation.tree.stderr | 4 +- .../both_borrows/illegal_write6.tree.stderr | 4 +- .../invalidate_against_protector2.tree.stderr | 4 +- .../invalidate_against_protector3.tree.stderr | 4 +- .../newtype_pair_retagging.tree.stderr | 4 +- .../newtype_retagging.tree.stderr | 4 +- .../arg_inplace_mutate.tree.stderr | 4 +- .../arg_inplace_observe_during.tree.stderr | 4 +- .../return_pointer_aliasing.tree.stderr | 4 +- .../fail/tree_borrows/outside-range.stderr | 4 +- .../reserved/cell-protected-write.stderr | 4 +- .../reserved/int-protected-write.stderr | 4 +- .../pass/tree_borrows/protected_uninit.rs | 16 -- .../tests/pass/tree_borrows/tree-borrows.rs | 18 ++ 18 files changed, 193 insertions(+), 100 deletions(-) delete mode 100644 src/tools/miri/tests/pass/tree_borrows/protected_uninit.rs diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs index 5fb298a54af33..7723f06f29688 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs @@ -310,9 +310,9 @@ impl TbError<'_> { "the accessed tag {accessed} is foreign to the {conflicting_tag_name} tag {conflicting} (i.e., it is not a child)" ), format!( - "this {access} would cause the {conflicting_tag_name} tag {conflicting} currently {before_disabled} to become Disabled" + "this {access} would cause the {conflicting_tag_name} tag {conflicting} (currently {before_disabled}) to become Disabled" ), - format!("protected tags must not become Disabled"), + format!("protected tags must never be Disabled"), ]; (title, details, conflicting_tag_name) } diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs index f68da455378b8..051b209da176d 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs @@ -381,7 +381,7 @@ mod propagation_optimization_checks { pub use super::*; impl PermissionPriv { /// Enumerate all states - pub fn all() -> impl Iterator { + pub fn all() -> impl Iterator { vec![ Active, Reserved { ty_is_freeze: true }, @@ -393,9 +393,15 @@ mod propagation_optimization_checks { } } + impl Permission { + pub fn all() -> impl Iterator { + PermissionPriv::all().map(|inner| Self { inner }) + } + } + impl AccessKind { /// Enumerate all AccessKind. - pub fn all() -> impl Iterator { + pub fn all() -> impl Iterator { use AccessKind::*; [Read, Write].into_iter() } @@ -403,7 +409,7 @@ mod propagation_optimization_checks { impl AccessRelatedness { /// Enumerate all relative positions - pub fn all() -> impl Iterator { + pub fn all() -> impl Iterator { use AccessRelatedness::*; [This, StrictChildAccess, AncestorAccess, DistantAccess].into_iter() } diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs index d6e4700101404..6a9b10374241a 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs @@ -19,6 +19,7 @@ use rustc_target::abi::Size; use crate::borrow_tracker::tree_borrows::{ diagnostics::{self, NodeDebugInfo, TbError, TransitionError}, + perms::PermTransition, unimap::{UniEntry, UniIndex, UniKeyMap, UniValMap}, Permission, }; @@ -69,6 +70,74 @@ impl LocationState { pub fn permission(&self) -> Permission { self.permission } + + /// Apply the effect of an access to one location, including + /// - applying `Permission::perform_access` to the inner `Permission`, + /// - emitting protector UB if the location is initialized, + /// - updating the initialized status (child accesses produce initialized locations). + fn perform_access( + &mut self, + access_kind: AccessKind, + rel_pos: AccessRelatedness, + protected: bool, + ) -> Result { + let old_perm = self.permission; + let transition = Permission::perform_access(access_kind, rel_pos, old_perm, protected) + .ok_or(TransitionError::ChildAccessForbidden(old_perm))?; + if protected + // Can't trigger Protector on uninitialized locations + && self.initialized + && transition.produces_disabled() + { + return Err(TransitionError::ProtectedDisabled(old_perm)); + } + self.permission = transition.applied(old_perm).unwrap(); + self.initialized |= !rel_pos.is_foreign(); + Ok(transition) + } + + // Optimize the tree traversal. + // The optimization here consists of observing thanks to the tests + // `foreign_read_is_noop_after_write` and `all_transitions_idempotent` + // that if we apply twice in a row the effects of a foreign access + // we can skip some branches. + // "two foreign accesses in a row" occurs when `perm.latest_foreign_access` is `Some(_)` + // AND the `rel_pos` of the current access corresponds to a foreign access. + fn skip_if_known_noop( + &mut self, + access_kind: AccessKind, + rel_pos: AccessRelatedness, + ) -> ContinueTraversal { + if rel_pos.is_foreign() { + let new_access_noop = match (self.latest_foreign_access, access_kind) { + // Previously applied transition makes the new one a guaranteed + // noop in the two following cases: + // (1) justified by `foreign_read_is_noop_after_write` + (Some(AccessKind::Write), AccessKind::Read) => true, + // (2) justified by `all_transitions_idempotent` + (Some(old), new) if old == new => true, + // In all other cases there has been a recent enough + // child access that the effects of the new foreign access + // need to be applied to this subtree. + _ => false, + }; + if new_access_noop { + // Abort traversal if the new transition is indeed guaranteed + // to be noop. + return ContinueTraversal::SkipChildren; + } else { + // Otherwise propagate this time, and also record the + // access that just occurred so that we can skip the propagation + // next time. + self.latest_foreign_access = Some(access_kind); + } + } else { + // A child access occurred, this breaks the streak of "two foreign + // accesses in a row" and we reset this field. + self.latest_foreign_access = None; + } + ContinueTraversal::Recurse + } } /// Tree structure with both parents and children since we want to be @@ -387,11 +456,15 @@ impl<'tcx> Tree { Ok(()) } - /// Maps the following propagation procedure to each range: - /// - initialize if needed; - /// - compute new state after transition; - /// - check that there is no protector that would forbid this; - /// - record this specific location as accessed. + /// Map the per-node and per-location `LocationState::perform_access` + /// to each location of `access_range`, on every tag of the allocation. + /// + /// `LocationState::perform_access` will take care of raising transition + /// errors and updating the `initialized` status of each location, + /// this traversal adds to that: + /// - inserting into the map locations that do not exist yet, + /// - trimming the traversal, + /// - recording the history. pub fn perform_access( &mut self, access_kind: AccessKind, @@ -411,55 +484,16 @@ impl<'tcx> Tree { let old_state = perm.or_insert_with(|| LocationState::new(node.default_initial_perm)); - // Optimize the tree traversal. - // The optimization here consists of observing thanks to the tests - // `foreign_read_is_noop_after_write` and `all_transitions_idempotent` - // that if we apply twice in a row the effects of a foreign access - // we can skip some branches. - // "two foreign accesses in a row" occurs when `perm.latest_foreign_access` is `Some(_)` - // AND the `rel_pos` of the current access corresponds to a foreign access. - if rel_pos.is_foreign() { - let new_access_noop = - match (old_state.latest_foreign_access, access_kind) { - // Previously applied transition makes the new one a guaranteed - // noop in the two following cases: - // (1) justified by `foreign_read_is_noop_after_write` - (Some(AccessKind::Write), AccessKind::Read) => true, - // (2) justified by `all_transitions_idempotent` - (Some(old), new) if old == new => true, - // In all other cases there has been a recent enough - // child access that the effects of the new foreign access - // need to be applied to this subtree. - _ => false, - }; - if new_access_noop { - // Abort traversal if the new transition is indeed guaranteed - // to be noop. - return Ok(ContinueTraversal::SkipChildren); - } else { - // Otherwise propagate this time, and also record the - // access that just occurred so that we can skip the propagation - // next time. - old_state.latest_foreign_access = Some(access_kind); - } - } else { - // A child access occurred, this breaks the streak of "two foreign - // accesses in a row" and we reset this field. - old_state.latest_foreign_access = None; + match old_state.skip_if_known_noop(access_kind, rel_pos) { + ContinueTraversal::SkipChildren => + return Ok(ContinueTraversal::SkipChildren), + _ => {} } - let old_perm = old_state.permission; let protected = global.borrow().protected_tags.contains_key(&node.tag); let transition = - Permission::perform_access(access_kind, rel_pos, old_perm, protected) - .ok_or(TransitionError::ChildAccessForbidden(old_perm))?; - if protected - // Can't trigger Protector on uninitialized locations - && old_state.initialized - && transition.produces_disabled() - { - return Err(TransitionError::ProtectedDisabled(old_perm)); - } + old_state.perform_access(access_kind, rel_pos, protected)?; + // Record the event as part of the history if !transition.is_noop() { node.debug_info.history.push(diagnostics::Event { @@ -470,10 +504,7 @@ impl<'tcx> Tree { transition_range: perms_range.clone(), span, }); - old_state.permission = - transition.applied(old_state.permission).unwrap(); } - old_state.initialized |= !rel_pos.is_foreign(); Ok(ContinueTraversal::Recurse) }, |args: ErrHandlerArgs<'_, TransitionError>| -> InterpError<'tcx> { @@ -602,3 +633,57 @@ impl AccessRelatedness { } } } + +#[cfg(test)] +mod commutation_tests { + use super::*; + impl LocationState { + pub fn all_without_access() -> impl Iterator { + Permission::all().flat_map(|permission| { + [false, true].into_iter().map(move |initialized| { + Self { permission, initialized, latest_foreign_access: None } + }) + }) + } + } + + #[test] + #[rustfmt::skip] + // Exhaustive check that for any starting configuration loc, + // for any two read accesses r1 and r2, if `loc + r1 + r2` is not UB + // and results in `loc'`, then `loc + r2 + r1` is also not UB and results + // in the same final state `loc'`. + // This lets us justify arbitrary read-read reorderings. + fn all_read_accesses_commute() { + let kind = AccessKind::Read; + // Two of the four combinations of `AccessRelatedness` are trivial, + // but we might as well check them all. + for rel1 in AccessRelatedness::all() { + for rel2 in AccessRelatedness::all() { + // Any protector state works, but we can't move reads across function boundaries + // so the two read accesses occur under the same protector. + for &protected in &[true, false] { + for loc in LocationState::all_without_access() { + // Apply 1 then 2. Failure here means that there is UB in the source + // and we skip the check in the target. + let mut loc12 = loc; + let Ok(_) = loc12.perform_access(kind, rel1, protected) else { continue; }; + let Ok(_) = loc12.perform_access(kind, rel2, protected) else { continue; }; + + // If 1 followed by 2 succeeded, then 2 followed by 1 must also succeed... + let mut loc21 = loc; + loc21.perform_access(kind, rel2, protected).unwrap(); + loc21.perform_access(kind, rel1, protected).unwrap(); + + // ... and produce the same final result. + assert_eq!( + loc12, loc21, + "Read accesses {:?} followed by {:?} do not commute !", + rel1, rel2 + ); + } + } + } + } + } +} diff --git a/src/tools/miri/tests/fail/both_borrows/aliasing_mut4.tree.stderr b/src/tools/miri/tests/fail/both_borrows/aliasing_mut4.tree.stderr index 8f278d0184a59..106e5c19bf277 100644 --- a/src/tools/miri/tests/fail/both_borrows/aliasing_mut4.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/aliasing_mut4.tree.stderr @@ -6,8 +6,8 @@ LL | ptr::write(dest, src); | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag is foreign to the protected tag (i.e., it is not a child) - = help: this foreign write access would cause the protected tag currently Frozen to become Disabled - = help: protected tags must not become Disabled + = help: this foreign write access would cause the protected tag (currently Frozen) to become Disabled + = help: protected tags must never be Disabled help: the accessed tag was created here --> $DIR/aliasing_mut4.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/both_borrows/box_noalias_violation.tree.stderr b/src/tools/miri/tests/fail/both_borrows/box_noalias_violation.tree.stderr index 6b4e86fa0c522..1ecd6620806ec 100644 --- a/src/tools/miri/tests/fail/both_borrows/box_noalias_violation.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/box_noalias_violation.tree.stderr @@ -6,8 +6,8 @@ LL | *y | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag is foreign to the protected tag (i.e., it is not a child) - = help: this foreign read access would cause the protected tag currently Active to become Disabled - = help: protected tags must not become Disabled + = help: this foreign read access would cause the protected tag (currently Active) to become Disabled + = help: protected tags must never be Disabled help: the accessed tag was created here --> $DIR/box_noalias_violation.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/both_borrows/illegal_write6.tree.stderr b/src/tools/miri/tests/fail/both_borrows/illegal_write6.tree.stderr index 5cfbb87fe2765..64e08f545e339 100644 --- a/src/tools/miri/tests/fail/both_borrows/illegal_write6.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/illegal_write6.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { *y = 2 }; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag is foreign to the protected tag (i.e., it is not a child) - = help: this foreign write access would cause the protected tag currently Active to become Disabled - = help: protected tags must not become Disabled + = help: this foreign write access would cause the protected tag (currently Active) to become Disabled + = help: protected tags must never be Disabled help: the accessed tag was created here --> $DIR/illegal_write6.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector2.tree.stderr b/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector2.tree.stderr index ee882db8ac8f5..66f7f1788e4d5 100644 --- a/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector2.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector2.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { *x = 0 }; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag is foreign to the protected tag (i.e., it is not a child) - = help: this foreign write access would cause the protected tag currently Frozen to become Disabled - = help: protected tags must not become Disabled + = help: this foreign write access would cause the protected tag (currently Frozen) to become Disabled + = help: protected tags must never be Disabled help: the accessed tag was created here --> $DIR/invalidate_against_protector2.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector3.tree.stderr b/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector3.tree.stderr index 30fd8215b6e2a..ef807d7362e44 100644 --- a/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector3.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/invalidate_against_protector3.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { *x = 0 }; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag (root of the allocation) is foreign to the protected tag (i.e., it is not a child) - = help: this foreign write access would cause the protected tag currently Frozen to become Disabled - = help: protected tags must not become Disabled + = help: this foreign write access would cause the protected tag (currently Frozen) to become Disabled + = help: protected tags must never be Disabled help: the accessed tag was created here --> $DIR/invalidate_against_protector3.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/both_borrows/newtype_pair_retagging.tree.stderr b/src/tools/miri/tests/fail/both_borrows/newtype_pair_retagging.tree.stderr index 0263c906b9b64..f26fc6cbaae26 100644 --- a/src/tools/miri/tests/fail/both_borrows/newtype_pair_retagging.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/newtype_pair_retagging.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { __rust_dealloc(ptr, layout.size(), layout.align()) } | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag is foreign to the protected tag (i.e., it is not a child) - = help: this deallocation (acting as a foreign write access) would cause the protected tag currently Frozen to become Disabled - = help: protected tags must not become Disabled + = help: this deallocation (acting as a foreign write access) would cause the protected tag (currently Frozen) to become Disabled + = help: protected tags must never be Disabled help: the accessed tag was created here --> $DIR/newtype_pair_retagging.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/both_borrows/newtype_retagging.tree.stderr b/src/tools/miri/tests/fail/both_borrows/newtype_retagging.tree.stderr index 710308fb86586..687c72d574d0d 100644 --- a/src/tools/miri/tests/fail/both_borrows/newtype_retagging.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/newtype_retagging.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { __rust_dealloc(ptr, layout.size(), layout.align()) } | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag is foreign to the protected tag (i.e., it is not a child) - = help: this deallocation (acting as a foreign write access) would cause the protected tag currently Frozen to become Disabled - = help: protected tags must not become Disabled + = help: this deallocation (acting as a foreign write access) would cause the protected tag (currently Frozen) to become Disabled + = help: protected tags must never be Disabled help: the accessed tag was created here --> $DIR/newtype_retagging.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.tree.stderr b/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.tree.stderr index f1bfe138def28..8393b80f25b5d 100644 --- a/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.tree.stderr +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { ptr.write(S(0)) }; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag (root of the allocation) is foreign to the protected tag (i.e., it is not a child) - = help: this foreign write access would cause the protected tag currently Active to become Disabled - = help: protected tags must not become Disabled + = help: this foreign write access would cause the protected tag (currently Active) to become Disabled + = help: protected tags must never be Disabled help: the accessed tag was created here --> $DIR/arg_inplace_mutate.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.tree.stderr b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.tree.stderr index 2cf61488e4379..5af4856bbe3b6 100644 --- a/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.tree.stderr +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { ptr.read() }; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag (root of the allocation) is foreign to the protected tag (i.e., it is not a child) - = help: this foreign read access would cause the protected tag currently Active to become Disabled - = help: protected tags must not become Disabled + = help: this foreign read access would cause the protected tag (currently Active) to become Disabled + = help: protected tags must never be Disabled help: the accessed tag was created here --> $DIR/arg_inplace_observe_during.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.tree.stderr b/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.tree.stderr index 0bda8e0e343bc..c491a904a108b 100644 --- a/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.tree.stderr +++ b/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.tree.stderr @@ -6,8 +6,8 @@ LL | unsafe { ptr.read() }; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag (root of the allocation) is foreign to the protected tag (i.e., it is not a child) - = help: this foreign read access would cause the protected tag currently Active to become Disabled - = help: protected tags must not become Disabled + = help: this foreign read access would cause the protected tag (currently Active) to become Disabled + = help: protected tags must never be Disabled help: the accessed tag was created here --> $DIR/return_pointer_aliasing.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/tree_borrows/outside-range.stderr b/src/tools/miri/tests/fail/tree_borrows/outside-range.stderr index f490f12b4c88a..8b3bf8414db03 100644 --- a/src/tools/miri/tests/fail/tree_borrows/outside-range.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/outside-range.stderr @@ -6,8 +6,8 @@ LL | *y.add(3) = 42; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag is foreign to the protected tag (i.e., it is not a child) - = help: this foreign write access would cause the protected tag currently Reserved to become Disabled - = help: protected tags must not become Disabled + = help: this foreign write access would cause the protected tag (currently Reserved) to become Disabled + = help: protected tags must never be Disabled help: the accessed tag was created here --> $DIR/outside-range.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr b/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr index 35361822af0bd..769769d957dbe 100644 --- a/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr @@ -16,8 +16,8 @@ LL | *y = 1; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag (y, callee:y, caller:y) is foreign to the protected tag (callee:x) (i.e., it is not a child) - = help: this foreign write access would cause the protected tag (callee:x) currently Reserved to become Disabled - = help: protected tags must not become Disabled + = help: this foreign write access would cause the protected tag (callee:x) (currently Reserved) to become Disabled + = help: protected tags must never be Disabled help: the accessed tag was created here --> $DIR/cell-protected-write.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/tree_borrows/reserved/int-protected-write.stderr b/src/tools/miri/tests/fail/tree_borrows/reserved/int-protected-write.stderr index 83e495ce1932c..f7e9fb9e3c3a4 100644 --- a/src/tools/miri/tests/fail/tree_borrows/reserved/int-protected-write.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/reserved/int-protected-write.stderr @@ -16,8 +16,8 @@ LL | *y = 0; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag (y, callee:y, caller:y) is foreign to the protected tag (callee:x) (i.e., it is not a child) - = help: this foreign write access would cause the protected tag (callee:x) currently Reserved to become Disabled - = help: protected tags must not become Disabled + = help: this foreign write access would cause the protected tag (callee:x) (currently Reserved) to become Disabled + = help: protected tags must never be Disabled help: the accessed tag was created here --> $DIR/int-protected-write.rs:LL:CC | diff --git a/src/tools/miri/tests/pass/tree_borrows/protected_uninit.rs b/src/tools/miri/tests/pass/tree_borrows/protected_uninit.rs deleted file mode 100644 index 120b2ac46d1d8..0000000000000 --- a/src/tools/miri/tests/pass/tree_borrows/protected_uninit.rs +++ /dev/null @@ -1,16 +0,0 @@ -//@compile-flags: -Zmiri-tree-borrows - -// Protectors should not trigger on uninitialized locations, -// otherwise we can write safe code that triggers UB. -// To test this we do a write access that disables a protected -// location, but the location is actually outside of the protected range. - -// Both x and y are protected here -fn write_second(_x: &mut u8, y: &mut u8) { - *y = 1; -} - -fn main() { - let mut data = (0u8, 1u8); - write_second(&mut data.0, &mut data.1); -} diff --git a/src/tools/miri/tests/pass/tree_borrows/tree-borrows.rs b/src/tools/miri/tests/pass/tree_borrows/tree-borrows.rs index 6bdad69596590..0d50d54faf6ac 100644 --- a/src/tools/miri/tests/pass/tree_borrows/tree-borrows.rs +++ b/src/tools/miri/tests/pass/tree_borrows/tree-borrows.rs @@ -9,6 +9,7 @@ use std::ptr; fn main() { aliasing_read_only_mutable_refs(); string_as_mut_ptr(); + two_mut_protected_same_alloc(); // Stacked Borrows tests read_does_not_invalidate1(); @@ -62,6 +63,23 @@ pub fn string_as_mut_ptr() { } } +// This function checks that there is no issue with having two mutable references +// from the same allocation both under a protector. +// This is safe code, it must absolutely not be UB. +// This test failing is a symptom of forgetting to check that only initialized +// locations can cause protector UB. +fn two_mut_protected_same_alloc() { + fn write_second(_x: &mut u8, y: &mut u8) { + // write through `y` will make some locations of `x` (protected) + // become Disabled. Those locations are outside of the range on which + // `x` is initialized, and the protector must not trigger. + *y = 1; + } + + let mut data = (0u8, 1u8); + write_second(&mut data.0, &mut data.1); +} + // ----- The tests below were taken from Stacked Borrows ---- // Make sure that reading from an `&mut` does, like reborrowing to `&`, From 53f0cb4b8a0055b13db9fbf645b8370769490b0d Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Sat, 29 Jul 2023 18:17:34 +0200 Subject: [PATCH 12/12] doc comment suggestions --- .../src/borrow_tracker/tree_borrows/tree.rs | 82 +++++++++++++------ 1 file changed, 57 insertions(+), 25 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs index 6a9b10374241a..355356b743a7d 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs @@ -29,17 +29,19 @@ use crate::*; /// Data for a single *location*. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) struct LocationState { - /// This pointer's current permission - permission: Permission, - /// A location is initialized when it is child accessed for the first time, - /// and it then stays initialized forever. - /// Before initialization we still apply some preemptive transitions on - /// `permission` to know what to do in case it ever gets initialized, - /// but these can never cause any immediate UB. There can however be UB - /// the moment we attempt to initialize (i.e. child-access) because some - /// foreign access done between the creation and the initialization is - /// incompatible with child accesses. + /// A location is initialized when it is child-accessed for the first time (and the initial + /// retag initializes the location for the range covered by the type), and it then stays + /// initialized forever. + /// For initialized locations, "permission" is the current permission. However, for + /// uninitialized locations, we still need to track the "future initial permission": this will + /// start out to be `default_initial_perm`, but foreign accesses need to be taken into account. + /// Crucially however, while transitions to `Disabled` would usually be UB if this location is + /// protected, that is *not* the case for uninitialized locations. Instead we just have a latent + /// "future initial permission" of `Disabled`, causing UB only if an access is ever actually + /// performed. initialized: bool, + /// This pointer's current permission / future initial permission. + permission: Permission, /// Strongest foreign access whose effects have already been applied to /// this node and all its children since the last child access. /// This is `None` if the most recent access is a child access, @@ -84,11 +86,21 @@ impl LocationState { let old_perm = self.permission; let transition = Permission::perform_access(access_kind, rel_pos, old_perm, protected) .ok_or(TransitionError::ChildAccessForbidden(old_perm))?; - if protected - // Can't trigger Protector on uninitialized locations - && self.initialized - && transition.produces_disabled() - { + // Why do only initialized locations cause protector errors? + // Consider two mutable references `x`, `y` into disjoint parts of + // the same allocation. A priori, these may actually both be used to + // access the entire allocation, as long as only reads occur. However, + // a write to `y` needs to somehow record that `x` can no longer be used + // on that location at all. For these uninitialized locations (i.e., locations + // that haven't been accessed with `x` yet), we track the "future initial state": + // it defaults to whatever the initial state of the tag is, + // but the access to `y` moves that "future initial state" of `x` to `Disabled`. + // However, usually a `Reserved -> Disabled` transition would be UB due to the protector! + // So clearly protectors shouldn't fire for such "future initial state" transitions. + // + // See the test `two_mut_protected_same_alloc` in `tests/pass/tree_borrows/tree-borrows.rs` + // for an example of safe code that would be UB if we forgot to check `self.initialized`. + if protected && self.initialized && transition.produces_disabled() { return Err(TransitionError::ProtectedDisabled(old_perm)); } self.permission = transition.applied(old_perm).unwrap(); @@ -96,13 +108,28 @@ impl LocationState { Ok(transition) } - // Optimize the tree traversal. + // Helper to optimize the tree traversal. // The optimization here consists of observing thanks to the tests - // `foreign_read_is_noop_after_write` and `all_transitions_idempotent` - // that if we apply twice in a row the effects of a foreign access - // we can skip some branches. - // "two foreign accesses in a row" occurs when `perm.latest_foreign_access` is `Some(_)` - // AND the `rel_pos` of the current access corresponds to a foreign access. + // `foreign_read_is_noop_after_write` and `all_transitions_idempotent`, + // that there are actually just three possible sequences of events that can occur + // in between two child accesses that produce different results. + // + // Indeed, + // - applying any number of foreign read accesses is the same as applying + // exactly one foreign read, + // - applying any number of foreign read or write accesses is the same + // as applying exactly one foreign write. + // therefore the three sequences of events that can produce different + // outcomes are + // - an empty sequence (`self.latest_foreign_access = None`) + // - a nonempty read-only sequence (`self.latest_foreign_access = Some(Read)`) + // - a nonempty sequence with at least one write (`self.latest_foreign_access = Some(Write)`) + // + // This function not only determines if skipping the propagation right now + // is possible, it also updates the internal state to keep track of whether + // the propagation can be skipped next time. + // It is a performance loss not to call this function when a foreign access occurs. + // It is unsound not to call this function when a child access occurs. fn skip_if_known_noop( &mut self, access_kind: AccessKind, @@ -124,19 +151,24 @@ impl LocationState { if new_access_noop { // Abort traversal if the new transition is indeed guaranteed // to be noop. - return ContinueTraversal::SkipChildren; + // No need to update `self.latest_foreign_access`, + // the type of the current streak among nonempty read-only + // or nonempty with at least one write has not changed. + ContinueTraversal::SkipChildren } else { // Otherwise propagate this time, and also record the // access that just occurred so that we can skip the propagation // next time. self.latest_foreign_access = Some(access_kind); + ContinueTraversal::Recurse } } else { - // A child access occurred, this breaks the streak of "two foreign - // accesses in a row" and we reset this field. + // A child access occurred, this breaks the streak of foreign + // accesses in a row and the sequence since the previous child access + // is now empty. self.latest_foreign_access = None; + ContinueTraversal::Recurse } - ContinueTraversal::Recurse } }