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

Fix attempted to read from stolen value #27

Open
W95Psp opened this issue Apr 12, 2023 · 10 comments
Open

Fix attempted to read from stolen value #27

W95Psp opened this issue Apr 12, 2023 · 10 comments
Labels
frontend Issue in the Rust to JSON translation P2 Medium priority
Milestone

Comments

@W95Psp
Copy link
Collaborator

W95Psp commented Apr 12, 2023

The frontend browses items of a crate in an order that causes this bug.
Some data in the Rust compiler is kept in a Steal, which is basically a box whose content can be stolen, that is pulled out of the memory (rustc uses arenas)

Thus, the order in which we browse the crate to export every item matters: sometimes I inspect things that happens to be stolen before because of some previous inspections.
This is particularly true for constants, it seems.

Status: mostly fixed

However, I tried Hax on core, and I get new stolen stuff. Thus, there exists some scenarios in which this still fails.

@W95Psp W95Psp added frontend Issue in the Rust to JSON translation P3 Low priority labels Apr 12, 2023
@geonnave
Copy link

For the record (context from #99):

@franziskuskiefer
Copy link
Member

Another code snippet that panics

#[repr(u16)]
pub enum ProtocolVersion {
    Mls10 = 1,
}

impl ProtocolVersion {
    #[allow(non_upper_case_globals)]
    fn tls_deserialize(bytes: &[u8]) -> core::result::Result<(Self, &[u8]), tls_codec::Error> {
        const __TLS_CODEC_Mls10: u16 = ProtocolVersion::Mls10 as u16;
        let (discriminant, remainder) =
            <u16 as tls_codec::DeserializeBytes>::tls_deserialize(bytes)?;
        match discriminant {
            __TLS_CODEC_Mls10 => {
                let result = ProtocolVersion::Mls10 {};
                Ok((result, remainder))
            }
            _ => Err(tls_codec::Error::UnknownValue(discriminant.into())),
        }
    }
}

@franziskuskiefer franziskuskiefer added P2 Medium priority and removed P3 Low priority labels Jun 13, 2023
bors bot added a commit that referenced this issue Jun 13, 2023
139: Fix steal bug r=W95Psp a=W95Psp

This is an attempt at fixing the frontend exporter bug described in #27.

The fix is really simple: it's all about processing `const`-like items first.

`@franziskuskiefer:` I tested a bit this fix on a reduced version of the example you posted in #27, but I think you've been hitting that much more, can you try this patch?

EDIT: also tried on `@geonnave's` example of #27, seems to be fine as well!

Co-authored-by: Lucas Franceschino <lucas.franceschino@inria.fr>
@W95Psp W95Psp added this to the v1 milestone Feb 7, 2024
@W95Psp
Copy link
Collaborator Author

W95Psp commented Feb 8, 2024

Another stealing bug (from issue #474):

// error: The THIR body of item DefId(0:7 ~ rust_ast[d581]::main::_::{constant#1}) was stolen.
const _MY_CONST: bool = true;
pub fn main() {
const _: [(); 1] = [(); _MY_CONST as usize];
}

Originally posted by @franziskuskiefer in #474 (comment)

Open this code snippet in the playground

Status: works OK in 8ab62258df

@franziskuskiefer
Copy link
Member

It looks like the two examples here are not triggering the stealing bug anymore.

@Nadrieril
Copy link
Collaborator

My current understanding of stealing insofar as it affects us:

  • THIR is stolen to construct the built MIR;
  • the built MIR is stolen to construct the optimized or ctfe MIRs;
  • optimized/ctfe MIR is required to evaluate constants, which happens if a constant shows up as a const generic argument, as a pattern, inside another evaluated constant, inside a MIR body that is being optimized, and likely other cases.

Hence even translating a type may steal a THIR/MIR body.

@W95Psp
Copy link
Collaborator Author

W95Psp commented Aug 8, 2024

Yes, exactly, that's what is happening :(
Thus it's not even clear we can come up with a visit order of items that would avoid stealing issues...

@Nadrieril
Copy link
Collaborator

If we could detect whether a value is already stolen, we could fallback to the optimized MIR/evaluated value for constants.

@Nadrieril
Copy link
Collaborator

Opened rust-lang/rust#128815

@W95Psp
Copy link
Collaborator Author

W95Psp commented Aug 8, 2024

reproducer for a stealing bug with cargo hax into -k mir-built: https://hax-playground.cryspen.com/#json+mir/a339b28377/gist=6630144c4732a2bd55ef8ded02b30ef6

(I haven't shown you the playground yet @Nadrieril, right? 😃 'right click > show MIR' on a rust subexpression might be interesting to you :D)

@Nadrieril
Copy link
Collaborator

You should now be able to fix all MIR stealing bugs by using the optimized_mir when tcx.mir_built().is_stolen(). For THIR bodies I don't think there's a good solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Issue in the Rust to JSON translation P2 Medium priority
Projects
No open projects
Status: Todo
Development

No branches or pull requests

4 participants