hax icon indicating copy to clipboard operation
hax copied to clipboard

Fix `attempted to read from stolen value`

Open W95Psp opened this issue 2 years ago • 10 comments

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

  • @geonnave's bug (https://github.com/hacspec/hacspec-v2/issues/27#issuecomment-1568365024) is fixed
  • @franziskuskiefer's bug (https://github.com/hacspec/hacspec-v2/issues/27#issuecomment-1586990315) is fixed

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

W95Psp avatar Apr 12 '23 07:04 W95Psp

For the record (context from #99):

  • this issue also affects translation of the edhoc-rs crate into fstar.
  • in #99 there is a How to reproduce section, that could be validated against once this issue is addressed.

geonnave avatar May 30 '23 12:05 geonnave

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 avatar Jun 12 '23 09:06 franziskuskiefer

Another stealing bug (from issue #474):

https://github.com/hacspec/hax/blob/aa14fb139280e7fcd28acb45db27b01b53c9de93/tests/rust-ast/src/lib.rs#L14-L20

Originally posted by @franziskuskiefer in https://github.com/hacspec/hax/issues/474#issuecomment-1916302172

Open this code snippet in the playground

Status: works OK in 8ab62258df

W95Psp avatar Feb 08 '24 08:02 W95Psp

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

franziskuskiefer avatar Jul 24 '24 15:07 franziskuskiefer

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.

Nadrieril avatar Aug 08 '24 07:08 Nadrieril

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...

W95Psp avatar Aug 08 '24 08:08 W95Psp

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

Nadrieril avatar Aug 08 '24 08:08 Nadrieril

Opened https://github.com/rust-lang/rust/pull/128815

Nadrieril avatar Aug 08 '24 08:08 Nadrieril

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? :smiley: 'right click > show MIR' on a rust subexpression might be interesting to you :D)

W95Psp avatar Aug 08 '24 08:08 W95Psp

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.

Nadrieril avatar Aug 14 '24 11:08 Nadrieril

This issue has been marked as stale due to a lack of activity for 60 days. If you believe this issue is still relevant, please provide an update or comment to keep it open. Otherwise, it will be closed in 7 days.

github-actions[bot] avatar Dec 05 '24 02:12 github-actions[bot]

I can't reproduce with any of the examples in here. Unless we have a reproducer, let's close this and open something new when we hit it again.

franziskuskiefer avatar Dec 05 '24 07:12 franziskuskiefer

We were discussing that yesterday with @Nadrieril: he found a way to override queries, which basically means we can just reimplement the code that used to do the stealing! 🥳

But indeed, on the THIR side of things, same thing for me, no stealing for a long time! And it's worth noting that we had no stealing issue extracting top 10k crates.io.

W95Psp avatar Dec 05 '24 07:12 W95Psp

This is still an issue (see linked libcrux issue above).

franziskuskiefer avatar Dec 11 '24 09:12 franziskuskiefer

You could solve these issues for good by overriding queries to catch the THIR directly after it is built. See here an example in Creusot that reliably catches the mir_built in this way.

Nadrieril avatar Feb 12 '25 16:02 Nadrieril

This fix still needs to be upstreamed to hax from hax-evit, so maybe keep this open?

karthikbhargavan avatar Feb 20 '25 18:02 karthikbhargavan

Out of curiosity, how did you fix it?

Nadrieril avatar Feb 20 '25 20:02 Nadrieril