Double Drop on Rust beta

@asm89

Getting double Drop/free errors on Rust. Sounds very dangerous. It started on 23 May rust nightly. Fails on beta as well, which is the next release.

A minimal repro https://bit.ly/2A52CJe
Dropping is printed twice. Flipping the order of elements in struct, makes it work and then drop is only called once.

Cc @asm89

@rustbot

@rustbot ping cleanup

@AminArria

Hey Cleanup Crew ICE-breakers! This bug has been identified as a good
"Cleanup ICE-breaking candidate". In case it's useful, here are some
instructions for tackling these sorts of bugs. Maybe take a look?
Thanks! <3

cc @AminArria @camelid @chrissimpkins @contrun @DutchGhost @elshize @ethanboxx @h-michael @HallerPatrick @hdhoang @hellow554 @imtsuki @kanru @KarlK90 @LeSeulArtichaut @MAdrianMattocks @matheus-consoli @mental32 @nmccarty @Noah-Kennedy @pard68 @PeytonT @pierreN @Redblueflame @RobbieClarken @RobertoSnap @robjtede @SarthakSingh31 @senden9 @shekohex @sinato @spastorino @turboladen @woshilapin @yerke

Possibly related to #71956 ?

Here is a simpler test case:

struct DD {
    a: String,
}

// DD is dropped twice
impl Drop for DD {
    fn drop(&mut self) {
        println!("Dropping!");
    }
}

struct Yolo {
    a: DD,
    b: Option<DD>,
}

async fn gg(x: &Yolo) {}

fn main() {
    futures::executor::block_on(async {
        let d2 = async { DD { a: "a".to_owned() } };

        // Swap the order here and it works
        let action = Yolo {
            b: None,
            a: d2.await,
        };
        gg(&action).await;
    });
}

playground

Note that the underlying problem is not a double free. We instead copy the value of a both into b and a, which changes b to Some(Yolo { .. }) which is then correctly dropped.

An example illustrating this is

struct Foo {
    a: usize,
    b: &'static u32,
}

fn main() {
    futures::executor::block_on(async {
        // Swap the order here and it works
        let action = Foo {
            b: &42,
            a: async { 0 }.await,
        };
        
        println!("{:p}", action.b);
        // ^ This prints `0x0`

        async {}.await;
    });
}

This would explain why my debugger replied with cannot access memory on action.b

@spastorino

@spastorino I find it strange that the priority of an issue can change based on whether a fix is being worked on. If (god forbid) I get hit by a bus tomorrow and no one thinks to take on the work of backporting #73153, we would still need to block the release, no?

@ecstatic-morse, maybe I didn't properly explain myself or I may be overlooking something. What I meant is that given that this problem was fixed by reverting the PR that has caused this regression, we can lower the priority of the issue now. My understanding is that after the revert the problem is fixed but we want to keep the issue open to find a better solution. Am I right? if so, this is why I consider that this is not P-critical anymore, given that the underlying problem is solved.

If nothing were done from this point forward (i.e. no one backported #73153), this miscompilation would land on the next stable release. I didn't come up with the current labelling system, but that seems critical to me.

@pnkfelix

I do think it makes some sense perhaps to keep this P-critical while the backport isn't accepted; I'm not sure if @pnkfelix hasn't had time to accept it yet or if we need to discuss next T-compiler meeting or what.

@ecstatic-morse

@ecstatic-morse the revert won't hit stable because it was beta-nominated and it was accepted for backport during today's meeting. Anyway, we have also discussed if we should change the priority or not but I don't think we have clearly concluded anything.

In my opinion, we should remove the P-critical as soon as the PR that fixes the issue lands and the PR is also beta nominated, because by doing that we have a way to track it and won't be missed. It will be accepted and it will be backported. The downsides of not doing so is having more P-critical issues in our process, we need to review them frequently, add them to the agenda, discuss them during the meeting, etc but those are unactionable and it also makes the list of P-critical issues bigger.

I don't have anything to add to my comments above besides the fact that I'm generally suspicious of open-source projects who change issue priority based on things besides severity. Ultimately it's up to y'all (meaning WG-prioritization) on how to handle release blocking issues that have been fixed on master but not beta.

Add regression test for rust-lang#73137

Fixed on beta in #73326

Look for stores between non-conflicting generator saved locals

Rollup merge of rust-lang#73244 - ecstatic-morse:validate-generator-m…

Navigation Menu

Repository navigation

Double Drop on Rust beta #73137

24 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Double Drop on Rust beta #73137

Description

Activity

jonas-schievink commented on Jun 8, 2020

rustbot commented on Jun 8, 2020

pranjalssh commented on Jun 8, 2020

Darksonn commented on Jun 8, 2020

lcnr commented on Jun 8, 2020

pranjalssh commented on Jun 8, 2020

24 remaining items

spastorino commented on Jun 11, 2020

ecstatic-morse commented on Jun 11, 2020

Mark-Simulacrum commented on Jun 11, 2020

spastorino commented on Jun 11, 2020

ecstatic-morse commented on Jun 11, 2020

ecstatic-morse commented on Jun 17, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions