Skip to content

TrustedRandomAccess optimization for Zip containing vec::IntoIter is unsound with how specialization currently behaves around HRTB fn pointers #85873

Closed

Description

Related to #85863, which now includes a high-level description covering both #85863 and #85873. [This issue is not a duplicate because both issues can be fixed independently in different ways.]

use std::{iter::Zip, marker::PhantomData, vec};

struct MyStruct<T>(PhantomData<T>);
impl<T> Clone for MyStruct<T> {
    fn clone(&self) -> Self {
        Self(PhantomData)
    }
}
impl Copy for MyStruct<fn(&())> {}

#[allow(clippy::type_complexity)]
fn main() {
    let i: vec::IntoIter<MyStruct<fn(&())>> = vec![MyStruct::<fn(&())>(PhantomData)].into_iter();
    let mut y = [Some(&42)];
    let mut z = i.zip(y.iter_mut());
    let r1 = z.next().unwrap().1;
    let mut z: Zip<vec::IntoIter<MyStruct<fn(&'static ())>>, _> = z;
    let r2 = z.next().unwrap().1;
    let elt = r1.as_ref().unwrap();
    dbg!(elt);
    *r2 = None;
    dbg!(elt);
}
   Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 1.10s
     Running `target/debug/playground`
[src/main.rs:20] elt = 42
[src/main.rs:22] elt = timeout: the monitored command dumped core
/playground/tools/entrypoint.sh: line 11:     8 Segmentation fault      timeout --signal=KILL ${timeout} "$@"

(playground)

@rustbot label T-libs-impl, T-compiler, A-specialization, A-iterators, A-typesystem, A-lifetimes, A-traits, regression-from-stable-to-stable
someone please add the unsound label thanks

Explanation

Zip uses an optimization if both contained iterators implement TrustedRandomAccess. There’s an impl

impl<T> TrustedRandomAccess for IntoIter<T>
where
    T: Copy, 

for both vec::IntoIter and vec_deque::IntoIter that depend on Copy. This way, unsound specialization is possible. This can be turned into actual ill-behaved programs at runtime similar as in #85863, relying on covariance of IntoIter and Zip. This way, the ZipImpl implementation is switched out and this way the Zip iterator gets into an illegal state resulting in violation of the contract of TrustedRandomAccess by calling .next() on the IterMut iterator after the first item was already read via __iterator_get_unchecked. The best immediate fix is probably to remove those two TrustedRandomAccess implementations for IntoIter; they’re an optimization only, anyway. This distinguishes this issue clearly from #85863 because for the FusedIterator trait, the specialization is quite directly part of the API, whereas this issue is only about a soundness regression from a performance optimization that can easily be reverted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    A-iteratorsArea: IteratorsA-lifetimesArea: Lifetimes / regionsA-specializationArea: Trait impl specializationA-traitsArea: Trait systemA-typesystemArea: The type systemC-bugCategory: This is a bug.I-unsoundIssue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/SoundnessP-criticalCritical priorityT-libsRelevant to the library team, which will review and decide on the PR/issue.regression-from-stable-to-stablePerformance or correctness regression from one stable version to another.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions