Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

recursive type_alias_impl_trait unsoundly allows different underlying types #113314

Closed
kwannoel opened this issue Jul 4, 2023 · 5 comments · Fixed by #113636
Closed

recursive type_alias_impl_trait unsoundly allows different underlying types #113314

kwannoel opened this issue Jul 4, 2023 · 5 comments · Fixed by #113636
Labels
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 T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@kwannoel
Copy link

kwannoel commented Jul 4, 2023

edit: minimized

#![feature(type_alias_impl_trait)]

type Op = impl std::fmt::Display;
fn foo() -> Op { &"hello world" }

fn transform<S>() -> impl std::fmt::Display {
    &0usize
}
fn bad() -> Op {
    transform::<Op>()
}

fn main() {
    let mut x = foo();
    println!("{x}");
    x = bad();
    println!("{x}");
}

see #113314 (comment) for more details


I tried this code:

#![feature(type_alias_impl_trait)]
#![feature(generators)]
#![feature(stmt_expr_attributes)]
#![feature(proc_macro_hygiene)]

use anyhow::{anyhow, Result};
use futures::stream::Stream;
use futures_async_stream::{stream, for_await};

type Op = impl Stream<Item = Result<usize>>;
async fn iter() -> Op {
    futures::stream::iter(vec![Ok(1), Err(anyhow!(""))])
}

#[stream(item = Result<usize>)]
async fn transform<S>(stream: S)
where
    S: Stream<Item = Result<usize>> + Unpin,
{
    #[for_await]
    for i in stream {
        yield i;
    }
}

async fn bad() -> Op {
    transform(Box::pin(iter().await))
}

#[allow(dead_code)]
#[stream(item = Result<usize>)]
async fn good() {
    #[for_await]
    for i in transform(Box::pin(iter().await)) {
        yield i;
    }
}

#[tokio::main]
async fn main() {
    let s = bad().await;
    // let s = good(); // may comment this out, this works instead.
    #[for_await]
    for i in s {
        println!("{:#?}", i);
    }
}

I expected to see this happen: Compilation should fail, because different generator types should be generated by calling transform, meaning bad and iter should not share the same type Op.

Instead, compilation passes, and SEGFAULT occurs during runtime:

    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running `target/debug/async-stream-min`
Ok(
    2334106421097295465,
)
Err(
zsh: segmentation fault  cargo run --bin async-stream-min

More context: https://github.com/risingwavelabs/risingwave/pull/10266/files#r1244771828

Meta

rustc --version --verbose:

rustc 1.69.0-nightly (31f858d9a 2023-02-28)
binary: rustc
commit-hash: 31f858d9a511f24fedb8ed997b28304fec809630
commit-date: 2023-02-28
host: aarch64-apple-darwin
release: 1.69.0-nightly
LLVM version: 15.0.7
No Backtrace from running it

$ RUST_BACKTRACE=1 cargo run --bin async-stream-min
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running `target/debug/async-stream-min`
Ok(
    2334106421097295465,
)
Err(
zsh: segmentation fault  RUST_BACKTRACE=1 cargo run --bin async-stream-min
noelkwan@Noels-MacBook-Pro rust-experiments % 

@kwannoel kwannoel added the C-bug Category: This is a bug. label Jul 4, 2023
@kwannoel kwannoel changed the title Segfault when using type_alias_impl_trait to unify stream generator types Segfault when using type_alias_impl_trait to unify stream generator types Jul 4, 2023
@tgross35
Copy link
Contributor

tgross35 commented Jul 4, 2023

@rustbot label +F-type_alias_impl_trait +I-unsound +T-compiler

@rustbot rustbot added 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 T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 4, 2023
@tgross35
Copy link
Contributor

tgross35 commented Jul 4, 2023

Only possible with unstable feature

@rustbot label -I-prioritize

@rustbot rustbot removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jul 4, 2023
@lcnr lcnr added the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Jul 4, 2023
@lcnr
Copy link
Contributor

lcnr commented Jul 4, 2023

it would be good to have an example which does not depend on external crates or macros here.

@lcnr
Copy link
Contributor

lcnr commented Jul 4, 2023

slightly minimized

#![feature(type_alias_impl_trait)]
#![feature(generators)]
#![feature(stmt_expr_attributes)]
#![feature(proc_macro_hygiene)]

use futures::stream::Stream;
use futures_async_stream::{stream, for_await};

type Op = impl Stream<Item = i32>;
async fn iter() -> Op {
    futures::stream::iter(vec![])
}

#[stream(item = i32)]
async fn transform<S>(_: S)
where
    S: Stream<Item = i32> + Unpin,
{}

async fn bad() -> Op {
    transform(Box::pin(iter().await))
}

#[tokio::main]
async fn main() {
    let s = bad().await;
    #[for_await]
    for i in s {
        println!("{:#?}", i);
    }
}

Cargo.toml

[package]
name = "test1"
version = "0.1.0"
edition = "2021"

[dependencies]
futures = "0.3.28"
tokio = { version = "1.29.1", features = ["rt-multi-thread", "macros"] }
futures-async-stream = "0.2.7"

completely minimized: #113278 + incorrectly ignoring recursive definitions, writing a clean MVCE rn

@lcnr
Copy link
Contributor

lcnr commented Jul 4, 2023

#![feature(type_alias_impl_trait)]

type Op = impl std::fmt::Display;
fn foo() -> Op { &"hello world" }

fn transform<S>() -> impl std::fmt::Display {
    &0usize
}
fn bad() -> Op {
    transform::<Op>()
}

fn main() {
    let mut x = foo();
    println!("{x}");
    x = bad();
    println!("{x}");
}

bad constrains Op to transform::{opaque#0}[Op], which gets ignored because of

struct RecursionChecker {
def_id: LocalDefId,
}
impl<'tcx> ty::TypeVisitor<TyCtxt<'tcx>> for RecursionChecker {
type BreakTy = ();
fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<Self::BreakTy> {
if let ty::Alias(ty::Opaque, ty::AliasTy { def_id, .. }) = *t.kind() {
if def_id == self.def_id.to_def_id() {
return ControlFlow::Break(());
}
}
t.super_visit_with(self)
}
}
if hidden_type
.visit_with(&mut RecursionChecker { def_id: opaque_type_key.def_id })
.is_break()
{
continue;
}

I am not totally sure how this all interacts, would have to look at the code, but it feels like the same root cause as #113278

@lcnr lcnr removed the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Jul 4, 2023
@lcnr lcnr changed the title Segfault when using type_alias_impl_trait to unify stream generator types recursive type_alias_impl_trait unsoundly allows different underlying types Jul 4, 2023
@bors bors closed this as completed in d351515 Jul 18, 2023
@compiler-errors compiler-errors added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants