Compiler accepts return mut ref to local var on no longer valid stackframe

[CasualX]

Spotted on reddit: https://www.reddit.com/r/rust/comments/7i64sy/safe_function_returns_static_mut/
credit to jDomantas

I tried this code: playground

fn gimme_static_mut() -> &'static mut u32 {
    let ref mut x = 1234543;
    x
}

fn main() {
    let x = gimme_static_mut();
    let y = gimme_static_mut();
    *y = 42;
    let a = *x;
    println!("a: {}", a);
}

This is accepted by the compiler and prints 42.

I expected to see this happen: The program should be rejected by the compiler with a lifetime error

Instead this happened: The program was accepted and miscompiled badly

The assembly code clearly demonstrates the problem, a local variable is created for the literal, its address is taken and returned:

playground::gimme_static_mut:
.Lfunc_begin2:
	push	rbp
.Lcfi6:
.Lcfi7:
	mov	rbp, rsp
.Lcfi8:
	sub	rsp, 16
	lea	rax, [rbp - 4]
.Ltmp6:
	mov	dword ptr [rbp - 4], 1234543
	mov	qword ptr [rbp - 16], rax
	mov	rax, qword ptr [rbp - 16]
.Ltmp7:
	add	rsp, 16
	pop	rbp
	ret
.Ltmp8:
.Lfunc_end2:

Meta

Latest version on playground

[collares]

This program is rejected by rustc 1.20.0, but is accepted by rustc 1.21.0. Bisecting now.

[skade]

@mauricioc This is due to #38865. It wasn't a feature in 1.20. 1.20 also doesn't compile the immutable (safe) variant:

fn gimme_static() -> &'static u32 {
    let ref x = 1234543;
    x
}

[CasualX]

An alternative way to define gimme_static_mut:

fn gimme_static_mut() -> &'static mut u32 {
    let x = &mut 1234543;
    x
}

Gives the expected compiler error:

   Compiling playground v0.0.1 (file:///playground)
error[E0597]: borrowed value does not live long enough
 --> src/main.rs:2:18
  |
2 |     let x = &mut 1234543;
  |                  ^^^^^^^ does not live long enough
3 |     x
4 | }
  | - temporary value only lives until here
  |
  = note: borrowed value must be valid for the static lifetime...

error: aborting due to previous error

error: Could not compile `playground`.

To learn more, run the command again with --verbose.

Looks like an oversight with the ref mut pattern match syntax to capture the literal by reference.

[nikomatsakis]

Fixed by MIR borrowck.

[shepmaster]

Fixed by MIR borrowck.

Which isn't enabled in Rust 1.23 beta, so we still need to fix this in some other fashion for the next release, correct?

[bstrie]

I see that Niko has commented though I don't see a priority assigned yet, so nominating for completeness.

[shepmaster]

This issue continues to ship in stable Rust 1.23

[nikomatsakis]

My expectation is that this will be fixed (along with a number of other bugs) when we transition to the MIR-based borrow checker (and NLL). I guess it's worth discussing whether we feel we ought to try to fix faster than that.

[bstrie]

Well before we can discuss whether we ought to get a fix in "faster", we'd need to have an official estimate of when MIR borrowck/NLL is expected to land in stable. :) I've gotten the impression that people have been deliberately noncommittal towards a timeframe up til now, but is it safe to assume that the answer is "hopefully stable sometime in 2018"?

[nikomatsakis]

@bstrie definitely -- first half of 2018 is my general plan, hopefully towards the beginning of that.

[nikomatsakis]

triage: P-medium

Plan is to fix this as part of the transition to NLL. I'll be adding a test case (marked with #![feature(nll)] soon.)

[SimonSapin]

Should this be close while #![feature(nll)] is not yet the default?

[aidanhs]

I think it's reasonable to say this is a P-medium issue until the fix is in master and enabled by default (I'm open to disagreement though!).

[nikomatsakis]

Well, it was intentional to close the issues now that there is a test, so as to keep the issue tracked (and in particular the WG-compielr-nll) more oriented at "actionable" issues. In other words, this is basically -- at this point -- a dup of the NLL tracking issue. I'm still inclined to close, I don't know what value there is from keeping it open. But I too am willing to give away if others feel really strongly.

We could perhaps compromise by removing WG-compiler-nll or something, so that I don't have to keep looking at the same issues over and over. But it's kind of annoying.

[nikomatsakis]

More specifically, the idea was to mark this as a "duplicate" of #47366, which admittedly wasn't quite clear.

[nikomatsakis]

@aidanhs and I discussed. Since there is a test and this is fixed in MIR borrow check, closing as duplicate of #47366.

[nikomatsakis]

Er, perhaps that was premature.

[nikomatsakis]

Decided to just remove WG-compiler-nll for such issues, so we can keep them open but they can stop annoying me.

[nikomatsakis]

I took a peek at this. The problem seems to be that we decide that the expression is promotable — which is true, but we ought not to be promoting a ref mut (let ref x = ... -- which also type checks -- is just fine).

[nikomatsakis]

Ah, I suspect I know the problem. Note that this variant is properly rejected:

fn gimme_static_mut() -> &'static mut u32 {
    match 123443 {
       ref mut x => x,
    }
}

fn main() {
    let x = gimme_static_mut();
    let y = gimme_static_mut();
    *y = 42;
    let a = *x;
    println!("a: {}", a);
}

I think the error is that we specifically mark things that are ref mut matched as non-promotable for matches:

rust/src/librustc_passes/rvalue_promotion.rs

Lines 227 to 239 in 577a5b2

	if let hir::ExprMatch(ref discr, ref arms, _) = ex.node {
	// Compute the most demanding borrow from all the arms'
	// patterns and set that on the discriminator.
	let mut mut_borrow = false;
	for pat in arms.iter().flat_map(|arm| &arm.pats) {
	if self.mut_rvalue_borrows.remove(&pat.id) {
	mut_borrow = true;
	}
	}
	if mut_borrow {
	self.mut_rvalue_borrows.insert(discr.id);
	}
	}

but we don't do anything similar for let initializers.

cc @eddyb — sound plausible?

[nikomatsakis]

I imagine then that we have to add some similar code here:

rust/src/librustc_passes/rvalue_promotion.rs

Lines 204 to 206 in 577a5b2

	hir::DeclLocal(_) => {
	self.promotable = false;
	}

basically, checking if the variable being assigned has a ref mut binding somewhere, and then — if so — adding the "initializer.id" into mut_rvalue_borrows.

[nikomatsakis]

Here is a bonus bug. This compiles:

fn gimme_static_mut() -> &'static mut u32 {
    match (123443,) {
       (ref mut x,) => x,
    }
}

[nikomatsakis]

Fix in #51274

[pnkfelix]

since this was fixed for AST borrow-check, it no longer should have the NLL-fixed-by-NLL label.