Skip to content

opaque type wf check anchor usage is unsound #113278

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
lcnr opened this issue Jul 3, 2023 · 2 comments · Fixed by #113661
Closed

opaque type wf check anchor usage is unsound #113278

lcnr opened this issue Jul 3, 2023 · 2 comments · Fixed by #113661
Labels
A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. C-bug Category: This is a bug. F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness requires-nightly This issue requires a nightly compiler in some way. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented Jul 3, 2023

see the second comment for an explanation: #113278 (comment)

#![feature(trivial_bounds, type_alias_impl_trait)]
mod sus {
    pub type Sep = impl Sized;
    pub fn define_sep() -> Sep {
        String::new()
    }

    pub type Tait = impl Mk;

    pub trait Mk: Proj {
        fn mk() -> <Self as Proj>::Assoc;
    }

    impl<T> Mk for T
    where
        T: Proj<Assoc = ()>,
    {
        fn mk() -> <T as Proj>::Assoc {
            ()
        }
    }

    pub trait Proj {
        type Assoc;
    }
    impl Proj for () {
        type Assoc = Sep;
    }

    fn define() -> Tait
    where
        (): Proj<Assoc = ()>,
    {
    }
}

fn main() {
    let _ = <sus::Tait as sus::Mk>::mk();
}

results in

error: internal compiler error: compiler/rustc_middle/src/ty/instance.rs:413:18: failed to resolve instance for <() as Mk>::mk

thread 'rustc' panicked at 'Box<dyn Any>', /rustc/37998ab508d5d9fa0d465d7b535dc673087dda8f/compiler/rustc_errors/src/lib.rs:1650:9
stack backtrace:
    [..]
query stack during panic:
#0 [collect_and_partition_mono_items] collect_and_partition_mono_items

going to minimize this further and explain why it is broken

@lcnr lcnr added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. C-bug Category: This is a bug. F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` requires-nightly This issue requires a nightly compiler in some way. labels Jul 3, 2023
@lcnr
Copy link
Contributor Author

lcnr commented Jul 3, 2023

#![feature(trivial_bounds, type_alias_impl_trait)]

mod sus {
    use super::*;
    pub type Sep = impl Sized + std::fmt::Display;
    pub fn mk_sep() -> Sep {
        String::from("hello")
    }

    pub trait Proj {
        type Assoc;
    }
    impl Proj for () {
        type Assoc = sus::Sep;
    }
    
    pub struct Bar<T: Proj> {
        pub inner: <T as Proj>::Assoc,
        pub _marker: T,
    }
    impl<T: Proj> Clone for Bar<T> {
        fn clone(&self) -> Self {
            todo!()
        }
    }
    impl<T: Proj<Assoc = i32> + Copy> Copy for Bar<T> {}
    pub type Tait = impl Copy + From<Bar<()>> + Into<Bar<()>>;
    pub fn define_tait() -> Tait
    where
        (): Proj<Assoc = i32>,
    {
        Bar {
            inner: 1i32,
            _marker: (),
        }
    }
}

fn copy_tait(x: sus::Tait) -> (sus::Tait, sus::Tait) {
    (x, x)
}

fn main() {
    let bar = sus::Bar {
        inner: sus::mk_sep(),
        _marker: (),
    };
    let (y, z) = copy_tait(bar.into()); // copy a string
    drop(y.into()); // drop one instance
    println!("{}", z.into().inner); // print the other
}
O��X
free(): double free detected in tcache 2
Aborted (core dumped)

The bug is as follows:

  • define_tait is allowed to have stronger where-bounds than Tait, we use an additional (): Proj<Assoc = i32> to correctly get Bar<()>: Copy inside of define_tait. This causes define_tait to be uncallable as the where-bound can never be true, but that doesn't matter as Tait implements From<Bar<()>>.
  • for that to be sound, we then check that Tait is well formed in its own where-bounds, these do not include (): Proj<Assoc = i32> so this should fail
  • HOWEVER, because we allow defining opaque types in project.rs this WF check defines sus::Tait to be i32 for the where-bound of the Copy impl. This defining use differs from the actual concrete type and is simply discarded

tl;dr: checking that opaque types are well-formed can define more opaques than its defining scope. These defining-uses are simply ignored however.

sources

equating the expected term of a Projection obligation can constrain opaque types:

// Need to define opaque types to support nested opaque types like `impl Fn() -> impl Trait`
match infcx.at(&obligation.cause, obligation.param_env).eq(
DefineOpaqueTypes::Yes,
normalized,
actual,
) {

we allow defining uses when checking wf and discard them in check_opaque_type_well_formed

.with_opaque_type_inference(if next_trait_solver {
DefiningAnchor::Bind(def_id)
} else {
DefiningAnchor::Bubble
})

// This is still required for many(half of the tests in ui/type-alias-impl-trait)
// tests to pass
let _ = infcx.take_opaque_types();

and the same in check_opaque_meets_bounds

.with_opaque_type_inference(DefiningAnchor::Bind(defining_use_anchor))

let _ = infcx.take_opaque_types();

@oli-obk
Copy link
Contributor

oli-obk commented Jul 3, 2023

oh, you found a way to do this! I tried and failed. I thought I had built an assertion against that, but maybe I removed it at some point. Should have left comments for myself.

Originally we had a PredicateKind::Opaque that was produced by all opaque type <-> other type comparisons. I guess this is where the new solver has landed now... I wonder if we can fix this reasonably in the old solver.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. C-bug Category: This is a bug. F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness requires-nightly This issue requires a nightly compiler in some way. T-types Relevant to the types team, which will review and decide on the PR/issue.
Development

Successfully merging a pull request may close this issue.

3 participants