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

Garbage value when accessing a reference into a field/element of a const value. #49955

Closed
shelvacu opened this issue Apr 13, 2018 · 6 comments
Closed
Assignees
Labels
A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@shelvacu
Copy link

shelvacu commented Apr 13, 2018

All of the below code seems to be neccesary to reproduce the bug; If ALL_THE_NUMS is accessed directly within main, the program works as expected.

Example:

fn get_element() -> &'static u32 {
    return &ALL_THE_NUMS[0];
}

const ALL_THE_NUMS : [u32; 1] = [
    1
];

fn main(){
    println!("Num is {}", get_element());
}

I expected to see this happen: Num is 1 as the output, or a compiler error if I'm doing something wrong here (am I?)

Instead, this happened: Num is 32766 or some other garbage value is output

Meta

rustc --version --verbose:

rustc 1.25.0
binary: rustc
commit-hash: unknown
commit-date: unknown
host: x86_64-unknown-linux-gnu
release: 1.25.0
LLVM version: 6.0

Also tested on master-branch version: (same result)

rustc 1.27.0-dev
binary: rustc
commit-hash: unknown
commit-date: unknown
host: x86_64-unknown-linux-gnu
release: 1.27.0-dev
LLVM version: 6.0
@matthewjasper matthewjasper added the A-borrow-checker Area: The borrow checker label Apr 13, 2018
@matthewjasper
Copy link
Contributor

It appears that (AST) borrowck thinks that the constant will get promoted to a static, but it isn't. NLL and pre-1.20 borrowck correctly error with a "temporary value does not live long enough" error.

@matthewjasper matthewjasper added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Apr 13, 2018
@cuviper cuviper added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Apr 14, 2018
@kennytm kennytm added NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. labels Apr 14, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Apr 14, 2018

cc @eddyb (const promotion)

@eddyb
Copy link
Member

eddyb commented Apr 14, 2018

Uhhh this should be promoted. AFAIK, &ALL_THE_NUMS[idx] is sugar for &(&ALL_THE_NUMS)[idx] and the inner reference would get promoted, making the indexing safe too.

EDIT: looks like we generate &local_array_copy[idx] as one MIR statement, which won't be promoted because idx is runtime, but non-MIR borrowck doesn't know this detail.

cc @rust-lang/compiler

@nikomatsakis nikomatsakis added the P-low Low priority label Apr 26, 2018
@eddyb eddyb changed the title Garbage value when accessing a reference into a const array returned by a function. Garbage value when accessing a reference into a field/element of a const value. Apr 26, 2018
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 26, 2018

After some discussion with @eddyb, we realized this is kind of a more general bug than originally thought. There are also some other troublesome examples:

https://play.rust-lang.org/?gist=5d024782ad35c5e58e000ecea7158cfd&version=stable&mode=debug

Therefore, bumping to P-high. We're gonna try to fix by improving the constant promoter and how it handles projections.

@nikomatsakis nikomatsakis added P-high High priority and removed I-nominated P-low Low priority labels Apr 26, 2018
@eddyb
Copy link
Member

eddyb commented Apr 26, 2018

This is much worse (I've updated the issue description), and it happens on all projections, e.g.:

fn get_element() -> &'static u32 { &(0,).0 }

I think I messed up: instead of promoting only Ref(temp), we should promote Ref(P(temp)) by promoting Ref(temp) to p and replacing Ref(P(temp)) with Ref(P(Deref(p))).
I guess I'll try to hack something up that we can backport.

@oli-obk
Copy link
Contributor

oli-obk commented May 8, 2018

Miri "catches" this kind of UB with an ICE if you try it inside a const fn (see #50529)

bors added a commit that referenced this issue May 19, 2018
 rustc_mir: allow promotion of promotable temps indexed at runtime.

Fixes #49955.

r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants