Add runtime validity checks inside MaybeUninit::assume_init #98073

5225225 · 2022-06-13T21:38:35Z

This is very much an experiment, I have no idea if this will work or is a good idea.

However, it (when combined with memory sanitizer), did find the use of uninit memory inside http. It introduces branches based on all fields it finds, even if those fields are of a type that's valid at any byte value. Granted, that was also found with miri, but this can work where miri doesn't.

This probably absolutely kills perf, so calls to assert_validity_of might need to be configured out unless a flag is given when building the stdlib.

One thing we could do is write out a maximally invalid type inside MaybeUninit::uninit, so if you do MaybeUninit<bool>::uninit(), it writes out the value 2, so you don't even need memory sanitizer to detect assume_init of uninit memory, if there is a niche value we can write to mean "uninit". That would mean miri and memory sanitizer can't detect the uninit memory, which would be a problem. Still, would be neat.

Still need to:

Deduplicate invariants generated
Don't generate invariants for types like u8 when not running under memory sanitizer
Clean up the unwraps
Check out the perf impact

rust-highfive · 2022-06-13T21:38:38Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-06-13T21:38:40Z

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

eggyal · 2022-06-13T22:30:32Z

(I haven't reviewed the PR, just commenting on the idea).

I think that rather than being added into assume_init, this should be exposed in a (possibly safe, if we're sure about the guarantees) fn try_init(self) -> Result<T, Self> (or similar) instead? Often people are using MaybeUninit for performance reasons, so extra runtime checks could be very unwelcome.

5225225 · 2022-06-13T22:40:20Z

This is mainly just a safety net to try and catch UB. There is already some checks similar to this in the stdlib already, see #92686. This is just a pretty extreme runtime check, I'm not proposing to make it a safe method.

One case where this can't detect UB is use of uninit memory. Without memory sanitizer, we just can't notice when someone forgot to write to a MaybeUninit.

Another case that's UB that we can't detect here (even with memory sanitizer), is use of pointer bytes as an integer. Miri can detect both of these, since it has the extra runtime tracking to know what bytes are what.

Also, we're not checking enums here, but that's not a strict limitation, it just complicates the code since you need to have conditionals (check byte 1 is in range 0..1 if byte 0 is 42).

library/core/src/intrinsics.rs

5225225 · 2022-06-14T00:41:17Z

Also I forgot to add them at first so I don't think highfive will ping you but hello @bjorn3 I believe rustc_codegen_cranelift is yours I'm once again messing with your intrinsics

By visual inspection it looks fine to me, I did roughly the same thing as in rustc_codegen_ssa, but is there a way for me to actually test this?

library/core/src/intrinsics.rs

bjorn3 · 2022-06-14T06:50:04Z

The cg_clif change looks fine.

5225225 · 2022-06-18T19:06:52Z

This fails a test, and I'm not sure what the best way to get around it is.

It's calling mem::replace and then looking to see if any other memcpys exist in the code, which this PR introduces (in debug assertions), and i don't think tests can rebuild the stdlib (without debug assertions).

I could cfg out the whole of the assertions here so you both have to build the stdlib and pass a flag to enable the assertions (like we do for core/debug_refcell to add locations), which might be the best bet depending on how slow they are. That does mean this code isn't being tested though.

That, or let the tests ask to rebuild the stdlib, disabling the debug assertions.

rust-log-analyzer · 2022-06-18T23:20:10Z

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

...............................................iii...................................... 13024/13088
................................................................
failures:

---- [ui] src/test/ui/intrinsics/validity_invariants_of.rs#sanitized stdout ----

error in revision `sanitized`: test compilation failed although it shouldn't!
status: exit status: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/intrinsics/validity_invariants_of.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--cfg" "sanitized" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/intrinsics/validity_invariants_of.sanitized/a" "-Crpath" "-O" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Z" "sanitizer=memory" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/intrinsics/validity_invariants_of.sanitized/auxiliary"
stdout: none
--- stderr -------------------------------
error: linking with `cc` failed: exit status: 1
   |
   = note: "cc" "-m64" "/tmp/rustc90BlYu/symbols.o" "-Wl,-Bstatic" "-Wl,--whole-archive" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc-nightly_rt.msan.a" "-Wl,--no-whole-archive" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/intrinsics/validity_invariants_of.sanitized/a.validity_invariants_of.f6faa112-cgu.0.rcgu.o" "-Wl,--as-needed" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/intrinsics/validity_invariants_of.sanitized/auxiliary" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-Wl,--start-group" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-Wl,-Bdynamic" "-lstd-d678279e1a099a18" "-Wl,--end-group" "-Wl,-Bstatic" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcompiler_builtins-f7cd8351a1acf669.rlib" "-Wl,-Bdynamic" "-lgcc_s" "-lutil" "-lrt" "-lpthread" "-lm" "-ldl" "-lc" "-Wl,--eh-frame-hdr" "-Wl,-znoexecstack" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/intrinsics/validity_invariants_of.sanitized/a" "-Wl,--gc-sections" "-pie" "-Wl,-zrelro,-znow" "-Wl,-O1" "-nodefaultlibs" "-Wl,-rpath,$ORIGIN/../../../../stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-Wl,--enable-new-dtags" "-Wl,-z,origin"
   = note: cc: error: /checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc-nightly_rt.msan.a: No such file or directory

error: aborting due to previous error
------------------------------------------




failures:
    [ui] src/test/ui/intrinsics/validity_invariants_of.rs#sanitized
test result: FAILED. 12972 passed; 1 failed; 115 ignored; 0 measured; 0 filtered out; finished in 153.19s

Some tests failed in compiletest suite=ui mode=ui host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu
Build completed unsuccessfully in 0:14:09

nagisa · 2022-06-19T14:21:46Z

library/core/src/mem/maybe_uninit.rs

    pub const unsafe fn assume_init(self) -> T {
        // SAFETY: the caller must guarantee that `self` is initialized.
        // This also means that `self` must be a `value` variant.
        unsafe {
            intrinsics::assert_inhabited::<T>();
+
+            intrinsics::assert_unsafe_precondition!(intrinsics::assert_validity_of::<T>(


Implementation wise, a more appropriate way would probably be to make this entire method an intrinsic/language item/whatever, rather than just the validity assertion. Doing so would also mean that the concerns about “how was libstd built” no longer matter since the callers would be calling into an intrinsic, giving an opportunity to codegen this function into the calling crate according to that crate’s flags.

nagisa · 2022-06-19T14:24:03Z

src/test/ui/sanitize/memory.rs

@@ -1,7 +1,7 @@
 // needs-sanitizer-support
 // needs-sanitizer-memory
 //
-// compile-flags: -Z sanitizer=memory -Zsanitizer-memory-track-origins -O
+// compile-flags: -Z sanitizer=memory -Zno-validity-invariant-checks -Zsanitizer-memory-track-origins -O


Before landing – the flag should be inverted and enabling it by default would need to be considered alongside stabilization of this feature as a whole.

nagisa · 2022-06-19T14:24:56Z

src/test/ui/intrinsics/validity_invariants_of.rs

+// run-pass
+// revisions: disabled normal sanitized
+// [disabled]compile-flags: -Zno-validity-invariant-checks
+// [sanitized]compile-flags: -Z sanitizer=memory


You will need to ignore this test based on needs-sanitizer-*. grep for needs-sanitizer in the test suite for examples of use.

Yeah, I tried just requiring needs-sanitizer-memory for just the sanitized revision, but that doesn't seem to be allowed for a revision.

I'll split the test into two and just require the sanitizer to exist for the -Z sanitizer=memory test. Alternatively, I could try and get compiletest to allow [sanitized]needs-sanitizer-memory, or just put the whole test as requiring it.

nagisa · 2022-06-19T14:25:55Z

library/core/src/lib.rs

@@ -156,7 +156,6 @@
 #![feature(const_slice_from_ref)]
 #![feature(const_slice_index)]
 #![feature(const_is_char_boundary)]
-//


Unrelated change, probably should undo (the extra space does give some sense of separation between blocks that isn’t really there with just a comment alone)

nagisa · 2022-06-19T14:28:39Z

compiler/rustc_middle/src/mir/interpret/value.rs

@@ -37,6 +37,9 @@ pub enum ConstValue<'tcx> {
    /// Used only for `&[u8]` and `&str`
    Slice { data: ConstAllocation<'tcx>, start: usize, end: usize },

+    /// Like `Slice`, but for types that aren't 1 byte long.
+    CustomSlice { data: ConstAllocation<'tcx>, length: usize },


Unclear to me why Slice cannot be adapted for this, but my opinion here is probably not worth much since I don’t fiddle much around mir/interpret.

nagisa · 2022-06-19T14:30:27Z

r? @oli-obk or @RalfJung since majority of the changes here involve mir/interpret.

RalfJung · 2022-06-19T16:21:44Z

It's calling mem::replace and then looking to see if any other memcpys exist in the code, which this PR introduces (in debug assertions), and i don't think tests can rebuild the stdlib (without debug assertions).

I think there is a way to ignore tests on debug builds. ignore-debug or so? Some existing tests do that.

bjorn3 · 2022-06-19T16:25:38Z

ignore-debug or so?

Indeed

oli-obk · 2022-06-21T10:13:37Z

@rustbot author

bors · 2022-07-09T20:29:54Z

☔ The latest upstream changes (presumably #98957) made this pull request unmergeable. Please resolve the merge conflicts.

5225225 · 2022-07-09T20:35:55Z

Closing this since I'm unlikely to get to it any time soon, and I'll be doing it as a MIR pass instead of checks inside core, with not very much shared code.

rust-highfive assigned nagisa Jun 13, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 13, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2022

This comment has been minimized.

Sign in to view

est31 reviewed Jun 14, 2022

View reviewed changes

library/core/src/intrinsics.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

RalfJung reviewed Jun 14, 2022

View reviewed changes

library/core/src/intrinsics.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

5225225 added 11 commits June 18, 2022 15:04

Initial work on runtime type validity checks

11c833e

Add more todos/safety comments

ea5be8b

Add allow unstable, fix stability of validity_invariants_of

5d42de2

Fix feature flag for validity_invariants_of

a515b9f

Add validity_invariants_of to cranelift codegen (untested)

831d2ed

Adjust documentation wording

16f7536

Calculate layout of Invariant as needed, use enum for size

19e0f6f

Deduplicate invariants generated

97ce1fb

Properly emit a slice of Invariant, add tests

3d94e25

Reformat

c24ef46

Add option to disable validity invariant generation

8f79b5a

5225225 force-pushed the validity-invariants branch from 4935692 to 8f79b5a Compare June 18, 2022 18:41

This comment has been minimized.

Sign in to view

Fix cranelift CustomSlice codegen

81bda49

nagisa reviewed Jun 19, 2022

View reviewed changes

rust-highfive assigned oli-obk and unassigned nagisa Jun 19, 2022

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 21, 2022

5225225 closed this Jul 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add runtime validity checks inside MaybeUninit::assume_init #98073

Add runtime validity checks inside MaybeUninit::assume_init #98073

5225225 commented Jun 13, 2022 •

edited

Loading

rust-highfive commented Jun 13, 2022

rust-highfive commented Jun 13, 2022

This comment has been minimized.

This comment has been minimized.

eggyal commented Jun 13, 2022 •

edited

Loading

5225225 commented Jun 13, 2022

5225225 commented Jun 14, 2022

This comment has been minimized.

bjorn3 commented Jun 14, 2022

This comment has been minimized.

5225225 commented Jun 18, 2022

This comment has been minimized.

rust-log-analyzer commented Jun 18, 2022

nagisa Jun 19, 2022

nagisa Jun 19, 2022

nagisa Jun 19, 2022

5225225 Jun 19, 2022

nagisa Jun 19, 2022

nagisa Jun 19, 2022

nagisa commented Jun 19, 2022

RalfJung commented Jun 19, 2022

bjorn3 commented Jun 19, 2022

oli-obk commented Jun 21, 2022

bors commented Jul 9, 2022

5225225 commented Jul 9, 2022

Add runtime validity checks inside MaybeUninit::assume_init #98073

Add runtime validity checks inside MaybeUninit::assume_init #98073

Conversation

5225225 commented Jun 13, 2022 • edited Loading

rust-highfive commented Jun 13, 2022

rust-highfive commented Jun 13, 2022

This comment has been minimized.

This comment has been minimized.

eggyal commented Jun 13, 2022 • edited Loading

5225225 commented Jun 13, 2022

5225225 commented Jun 14, 2022

This comment has been minimized.

bjorn3 commented Jun 14, 2022

This comment has been minimized.

5225225 commented Jun 18, 2022

This comment has been minimized.

rust-log-analyzer commented Jun 18, 2022

nagisa Jun 19, 2022

Choose a reason for hiding this comment

nagisa Jun 19, 2022

Choose a reason for hiding this comment

nagisa Jun 19, 2022

Choose a reason for hiding this comment

5225225 Jun 19, 2022

Choose a reason for hiding this comment

nagisa Jun 19, 2022

Choose a reason for hiding this comment

nagisa Jun 19, 2022

Choose a reason for hiding this comment

nagisa commented Jun 19, 2022

RalfJung commented Jun 19, 2022

bjorn3 commented Jun 19, 2022

oli-obk commented Jun 21, 2022

bors commented Jul 9, 2022

5225225 commented Jul 9, 2022

5225225 commented Jun 13, 2022 •

edited

Loading

eggyal commented Jun 13, 2022 •

edited

Loading