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

Revert #94158, "Apply noundef metadata to loads of types that do not permit raw init" #98966

Closed
wants to merge 1 commit into from

Commits on Jul 6, 2022

  1. Revert rust-lang#94158, "Apply noundef metadata to loads of types tha…

    …t do not permit raw init"
    
    In rust-lang#94158, we started emitting `noundef`, which means that functions
    returning uninitialized references emit IR with is both `ret void` and
    also `noundef`: https://godbolt.org/z/hbjsKKfc3
    More generally, this change makes `mem::uninitialized` dangerous in a
    way that it wasn't before. This `noundef` change was shipped in the
    past 2 stable releases, 1.61.0 and 1.62.0.
    
    This concerns me because in rust-lang#66151 we have thus far decided against
    issuing a panic for creating uninitialized references within arrays,
    on account of the breakage that shows up in crater. If this pattern
    is so widely-used that we're not willing to put in a runtime check
    for it, then it doesn't seem prudent to invite LLVM to exploit this
    UB.
    
    The pattern of creating uninit references in arrays shows up real code
    because the 0.11, 0.12, and 0.13 release series for `hyper` all use
    `mem::uninitialized` to construct arrays of `httparse::Header`, which
    contains a `&str` and `&[u8]`. There is no patch available within
    these release series, so a `cargo update` is not a viable way to
    escape the UB here. Also note that the 0.11 release series of `hyper`
    predates `MaybeUninit`, so any source-level patch for that will incur
    runtime overhead. Which would be unfortunate, but preferable to UB.
    
    I discussed this with @thomcc on the community Discord, and we think
    that it would be prudent to revert this introduction of `noundef` until
    we have a mitigation in place for the UB that this may unleash. We
    haven't been able to cook up any examples of surprising optimizations
    due to this pattern, but the whole point of `noundef` is to enable
    optimizations, and we know that there is code which uses it in a way
    which is definitely instant UB and which we have declined to inform users
    of.
    
    If possible, we would like to see `freeze` applied to the return value
    of `mem::uninitialized` as a mitigation for this problem. That may be
    able to keep old code functioning without introducing a performance hit.
    
    @rustbot labels add +T-compiler +I-compiler-nominated
    saethlin committed Jul 6, 2022
    Configuration menu
    Copy the full SHA
    d6f8271 View commit details
    Browse the repository at this point in the history