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

Polonius (but not ordinary NLL) accepts code which segfaults #53142

Closed
KiChjang opened this issue Aug 7, 2018 · 11 comments
Closed

Polonius (but not ordinary NLL) accepts code which segfaults #53142

KiChjang opened this issue Aug 7, 2018 · 11 comments
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness NLL-polonius Issues related for using Polonius in the borrow checker requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@KiChjang
Copy link
Member

KiChjang commented Aug 7, 2018

Based on this tweet https://twitter.com/bhuztez/status/1026061344980828160.

Commit that worked around the segfault: https://github.com/bhuztez/porus/commit/d28ebdac301c0d81284bbfb93e0eb0c2d48ddb5b

@bhuztez 不知道你能不能看英语,能否提供有关这个bug的详细资料吗?

@KiChjang KiChjang added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness and removed I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness labels Aug 7, 2018
@KiChjang
Copy link
Member Author

KiChjang commented Aug 7, 2018

Removing the soundness label until further details are provided.

@upsuper
Copy link
Contributor

upsuper commented Aug 7, 2018

I suggest we close this bug. The problem basically roots in this file: https://github.com/bhuztez/porus/blob/5a733ac564146f20bb1b2a65ff98d3ca9234a1ab/src/ptr.rs

The codebase exposes unsafe pointer operations as safe functions without any soundness check, and those "safe" operations are used to build some basic data structures, which are then used in the code in question. There is nothing Rust compiler can promise with that on soundness, because the code basically bypasses any soundness check.

@kennytm
Copy link
Member

kennytm commented Aug 7, 2018

This may be a Polonius bug. Note that the project invokes rustc with -Z borrowck=mir -Z polonius.

Compiling these files directly on commit af6ecbc without NLL gives the following errors:

ALDS1_6_B.rs
error[E0597]: borrowed value does not live long enough
  --> ALDS1_6_B.rs:14:5
   |
14 | /     writelnf!(
15 | |         "{}[{:d}]{}",
16 | |         join(f!(""), list::iter(slice!(a, [, pivot])).map(|e| f!("{e:d} "))),
17 | |         a[pivot],
18 | |         join(f!(""), list::iter(slice!(a, [pivot+1, ])).map(|e| f!(" {e:d}"))));
   | |                                                                                ^
   | |                                                                                |
   | |________________________________________________________________________________temporary value dropped here while still borrowed
   |                                                                                  temporary value does not live long enough
   |
   = note: values in a scope are dropped in the opposite order they are created
   = note: consider using a `let` binding to increase its lifetime
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error[E0597]: borrowed value does not live long enough
  --> ALDS1_6_B.rs:14:5
   |
14 | /     writelnf!(
15 | |         "{}[{:d}]{}",
16 | |         join(f!(""), list::iter(slice!(a, [, pivot])).map(|e| f!("{e:d} "))),
17 | |         a[pivot],
18 | |         join(f!(""), list::iter(slice!(a, [pivot+1, ])).map(|e| f!(" {e:d}"))));
   | |                                                                                ^
   | |                                                                                |
   | |________________________________________________________________________________temporary value dropped here while still borrowed
   |                                                                                  temporary value does not live long enough
   |
   = note: values in a scope are dropped in the opposite order they are created
   = note: consider using a `let` binding to increase its lifetime
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error[E0499]: cannot borrow `*porus_sink` as mutable more than once at a time
  --> ALDS1_6_B.rs:14:5
   |
14 | /     writelnf!(
15 | |         "{}[{:d}]{}",
16 | |         join(f!(""), list::iter(slice!(a, [, pivot])).map(|e| f!("{e:d} "))),
17 | |         a[pivot],
18 | |         join(f!(""), list::iter(slice!(a, [pivot+1, ])).map(|e| f!(" {e:d}"))));
   | |                                                                                ^
   | |                                                                                |
   | |________________________________________________________________________________mutable borrow ends here
   |                                                                                  mutable borrow starts here in previous iteration of loop

error: aborting due to 3 previous errors

Some errors occurred: E0499, E0597.
For more information about an error, try `rustc --explain E0499`.
ITP1_6_A.rs
error[E0597]: borrowed value does not live long enough
  --> ITP1_6_A.rs:11:5
   |
11 |     writelnf!("{}", join(f!(" "), list::iter(slice!(a, [,,-1])).map(|e| f!("{e:d}"))));
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
   |     |                                                                                 |
   |     |                                                                                 temporary value dropped here while still borrowed
   |     temporary value does not live long enough
   |
   = note: values in a scope are dropped in the opposite order they are created
   = note: consider using a `let` binding to increase its lifetime
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

(Note: I haven't checked the source in detail whether the segfault is caused by the invalid "safe" code in #53142 (comment))

@KiChjang
Copy link
Member Author

KiChjang commented Aug 7, 2018

cc @rust-lang/wg-compiler-nll

@KiChjang KiChjang added A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll labels Aug 7, 2018
@nikomatsakis
Copy link
Contributor

It sounds like it would be helpful if someone (@bhuztez, perhaps?) could try to "narrow this down" to an isolated example. I'm going to mark it as NLL-deferred for now, since Polonius is not high priority just now.

@nikomatsakis nikomatsakis changed the title "Rust code passed compilation checks, but still segfaults" Polonius (but not ordinary NLL) accepts code which segfaults Aug 7, 2018
@matthewjasper
Copy link
Contributor

-Z polonius never reports conflict errors.

@bhuztez
Copy link

bhuztez commented Aug 7, 2018

compile the following code with rustc -Z borrowck=mir -Z polonius -C opt-level=2 and run. It does not print 300

struct X<'a, T: 'a> {
    a: &'a T,
    b: isize,
    c: isize,
}

struct Y<'a, T: 'a> {
    a: &'a T,
    b: usize,
}

fn new_y<'a, T: 'a>(x: &'a T) -> Y<'a, T> {
    Y {
        a: x,
        b: 100,
    }
}

fn main() {
    let a = &200isize;
    let y = new_y(&X{a: a, b: 300, c:400});
    let f = || { println!("{}", y.a.b); };
    (|| { f() })();
}

@kennytm kennytm added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness C-bug Category: This is a bug. labels Aug 7, 2018
@nikomatsakis
Copy link
Contributor

I think that — quite likely — the optimization work has broken polonius mode. We might consider removing it for now.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Aug 8, 2018

I know some of the changes I am doing are likely to break it. =)

bors added a commit that referenced this issue Sep 27, 2018
[NLL] Get Polonius borrow check to work in simple cases

* Restores the generation of outlives facts from subtyping.
* Restore liveness facts.
* Generate invalidates facts at the start point of each location,
  where we check for errors.
* Add a small test for simple cases (previously these cases have compiled, and more recently ICEd).

Closes #54212
cc #53142 (will need test)

### Known limitations

* Two phase borrows aren't implemented for Polonius yet
* Invalidation facts haven't been updated for some of the recent changes to make `Drop` terminators access fewer things.
* Fact generation is not as optimized as it could be.
* Around 30 tests fail in compare mode, often tests that are ignored in nll compare mode

r? @nikomatsakis
@matthewjasper matthewjasper added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Oct 14, 2018
@matthewjasper matthewjasper added NLL-polonius Issues related for using Polonius in the borrow checker and removed NLL-deferred labels Dec 1, 2018
@Centril Centril added the requires-nightly This issue requires a nightly compiler in some way. label Jul 28, 2019
@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 15, 2019
@bjorn3
Copy link
Member

bjorn3 commented Sep 23, 2019

Is this still an issue?

@Mark-Simulacrum
Copy link
Member

I'm going to go ahead and close this as it seems likely that polonius has since changed and in any case it doesn't seem helpful to keep a bug open about it :)

If anything it could be moved to the polonius repo :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness NLL-polonius Issues related for using Polonius in the borrow checker requires-nightly This issue requires a nightly compiler in some way. 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

10 participants