Skip to content

static FOO:Foo=FOO; doesn't cause cycle error for zero-sized-type with no public constructor. #71078

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
rodrimati1992 opened this issue Apr 12, 2020 · 6 comments · Fixed by #71140
Assignees
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-visibility Area: Visibility / privacy C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team

Comments

@rodrimati1992
Copy link
Contributor

rodrimati1992 commented Apr 12, 2020

I tried this code:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=b8f15edf4886c9c8bb753636feb70ec2

pub mod foo {
    #[derive(Debug, Copy, Clone)]
    pub struct Foo {
        x: (),
    }
}

pub static FOO: foo::Foo = FOO;

fn main() {
    dbg!(FOO);
}

I expected the code to fail to compile with a cycle error.

Instead, it compiled and ran,printing the value of FOO .

Meta

I was able to verify that this bug happens for the 1.42.0 stable, 1.43-beta.5, and 1.44.0-nightly(2020-04-11 e82734e) versions.

Trying this code in rust.godbolt.org it errors up to 1.36.0,and doesn't error anymore from 1.37.0 onwards.

@rodrimati1992 rodrimati1992 added the C-bug Category: This is a bug. label Apr 12, 2020
@jonas-schievink jonas-schievink added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team labels Apr 12, 2020
@eddyb
Copy link
Member

eddyb commented Apr 12, 2020

cc @rust-lang/wg-const-eval

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Apr 13, 2020

cargo-bisect-rustc narrowed to the following:

Auto merge of #63015 - Centril:rollup-ydhpcas, r=Centril
Auto merge of #62086 - petrochenkov:builtout, r=eddyb
Auto merge of #62748 - luca-barbieri:optimize-refcell-borrow, r=RalfJung
Auto merge of #63043 - Centril:rollup-f4baee4, r=Centril
Auto merge of #63029 - petrochenkov:rpass, r=Centril

Possibly #62982?

@Centril
Copy link
Contributor

Centril commented Apr 13, 2020

Weaponized soundness hole (using https://docs.rs/refl/0.2.1/refl/ as a foundation):

pub static FOO: refl::Id<usize, Box<u8>> = FOO;

fn main() {
    let x: Box<_> = FOO.cast(42usize);
    drop(x);
}

mod refl {
    pub struct Id<S: ?Sized, T: ?Sized>(core::marker::PhantomData<(fn(S) -> S, fn(T) -> T)>);

    impl<S: ?Sized, T: ?Sized> Copy for Id<S, T> {}
    impl<S: ?Sized, T: ?Sized> Clone for Id<S, T> { fn clone(&self) -> Self { *self } }

    impl<S: ?Sized, T: ?Sized> Id<S, T> {
        /// Casts a value of type `S` to `T`.
        ///
        /// This is safe because the `Id` type is always guaranteed to
        /// only be inhabited by `Id<T, T>` types by construction.
        pub fn cast(self, value: S) -> T where S: Sized, T: Sized {
            unsafe {
                // Transmute the value;
                // This is safe since we know by construction that
                // S == T (including lifetime invariance) always holds.
                let cast_value = core::mem::transmute_copy(&value);
    
                // Forget the value;
                // otherwise the destructor of S would be run.
                core::mem::forget(value);
    
                cast_value
            }
        }
    }
}

==>

   Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 0.60s
     Running `target/debug/playground`
timeout: the monitored command dumped core
/playground/tools/entrypoint.sh: line 11:     7 Segmentation fault      timeout --signal=KILL ${timeout} "$@"

@Centril Centril added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness I-prioritize Issue: Indicates that prioritization has been requested for this issue. P-high High priority labels Apr 13, 2020
@Centril
Copy link
Contributor

Centril commented Apr 13, 2020

cc @oli-obk @RalfJung

@Centril Centril added A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-visibility Area: Visibility / privacy labels Apr 13, 2020
@RalfJung
Copy link
Member

oopsie yeah... #62982 is probably where this started. But that is crucial to fix #62189.

The problem is that "reading from a ZST" isn't actually doing anything. Here's the MIR for FOO:

static  FOO: foo::Foo = {
    let mut _0: foo::Foo;                // return place in scope 0 at src/main.rs:8:17: 8:25
    let mut _1: &foo::Foo;               // in scope 0 at src/main.rs:8:28: 8:31

    bb0: {
        StorageLive(_1);                 // bb0[0]: scope 0 at src/main.rs:8:28: 8:31
        _1 = const Scalar(alloc0+0): &foo::Foo; // bb0[1]: scope 0 at src/main.rs:8:28: 8:31
                                         // ty::Const
                                         // + ty: &foo::Foo
                                         // + val: Value(Scalar(alloc0+0))
                                         // mir::Constant
                                         // + span: src/main.rs:8:28: 8:31
                                         // + literal: Const { ty: &foo::Foo, val: Value(Scalar(alloc0+0)) }
        _0 = (*_1);                      // bb0[2]: scope 0 at src/main.rs:8:28: 8:31
        StorageDead(_1);                 // bb0[3]: scope 0 at src/main.rs:8:30: 8:31
        return;                          // bb0[4]: scope 0 at src/main.rs:8:1: 8:32
    }
}

So there actually is a *_1 that loads from this static (I didn't expect that). But in Miri, a ZST load just checks that the address is in-bounds; no actual access to the underlying memory happens so there is no cycle error.

@spastorino spastorino added I-prioritize Issue: Indicates that prioritization has been requested for this issue. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Apr 14, 2020
@spastorino
Copy link
Member

Assigning P-critical as discussed as part of the Prioritization Working Group process and removing I-prioritize.

@spastorino spastorino added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. P-high High priority labels Apr 14, 2020
@oli-obk oli-obk self-assigned this Apr 14, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 25, 2020
[breaking change] Disallow statics initializing themselves

fixes rust-lang#71078

Self-initialization is unsound because it breaks privacy assumptions that unsafe code can make. In

```rust
pub mod foo {
    #[derive(Debug, Copy, Clone)]
    pub struct Foo {
        x: (),
    }
}

pub static FOO: foo::Foo = FOO;
```

unsafe could could expect that ony functions inside the `foo` module were able to create a value of type `Foo`.
@bors bors closed this as completed in b964451 Apr 26, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-visibility Area: Visibility / privacy C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants