Stream: t-lang/wg-unsafe-code-guidelines

Topic: triggering UB with pinned generators


Jake Goulding (Jun 19 2019 at 16:35, on Zulip):

Does anyone have an easy example of a generator that will trigger Miri due to an improper usage of Pin?

I think I have some code that I think is using Pin::new_unchecked incorrectly, but Miri reports it as clean. I think that if I can get a generator that definitely uses self-references it will though.

nagisa (Jun 19 2019 at 16:47, on Zulip):

Any generator which maintains internal references during execution?

nagisa (Jun 19 2019 at 16:47, on Zulip):

between yield points?

RalfJ (Jun 19 2019 at 17:01, on Zulip):

yeah you have to actually get the code to access bad memory in the wrong way

Jake Goulding (Jun 19 2019 at 20:14, on Zulip):

Trying the simple thing gives a compiler error; do I actually need to use raw pointers?

    move || {
        let mut num = 0;
        let num = &mut num;

        yield *num;
        *num += 1;

        yield *num;
        *num += 1;

        yield *num;
        *num += 1;
    }
error[E0626]: borrow may still be in use when generator yields
  --> src/main.rs:11:19
   |
11 |         let num = &mut num;
   |                   ^^^^^^^^
12 |
13 |         yield *num;
   |         ---------- possible yield occurs here
RalfJ (Jun 19 2019 at 20:46, on Zulip):

probably there's some unstable feature flag to enable this?

Jake Goulding (Jun 20 2019 at 02:58, on Zulip):

That would be cool, but finding a feature flag can be annoying. I like it when the compiler tells me :-)

RalfJ (Jun 20 2019 at 07:25, on Zulip):

or maybe it only works for futures? that seems weird though.
Cc @Taylor Cramer @tmandry

tmandry (Jun 21 2019 at 02:17, on Zulip):

I think you would need to make an immovable generator (i.e. static move || { ... }), resume it with Pin::new_unchecked, move it, and then resume it again

RalfJ (Jun 21 2019 at 07:11, on Zulip):

static move || is the key to make an immovable generator closure?

RalfJ (Jun 21 2019 at 07:11, on Zulip):

is there something similar for top-level functions?

Jake Goulding (Jun 21 2019 at 19:15, on Zulip):

@tmandry perfect, thank you! this now trips up Miri correctly

Jake Goulding (Jun 21 2019 at 19:17, on Zulip):

TL;DR: I wrote some incorrect unsafe code for generators years ago. I moved it to Pin and that made me realize that it was broken; I just wanted to "prove" it via Miri

Jake Goulding (Jun 21 2019 at 19:18, on Zulip):

I guess there's not an easy way to adapt a generator to an iterator without something like Box::pin because there's no way to prevent moving the iterator

RalfJ (Jun 21 2019 at 19:18, on Zulip):

@Jake Goulding thanks for that testcase :D

Jake Goulding (Jun 21 2019 at 19:19, on Zulip):

@RalfJ NP. I'm sure it can be made smaller as I wasn't going for minimal

RalfJ (Jun 21 2019 at 19:19, on Zulip):

shrug fine for me

RalfJ (Jun 21 2019 at 19:33, on Zulip):

hm, unfortunately your generator test errors even if you dont break the pinning^^

RalfJ (Jun 21 2019 at 19:34, on Zulip):

looks like a stacked borrows violation somewhere in the genrator stuff...

RalfJ (Jun 21 2019 at 19:35, on Zulip):

ah I guess it's not too surprising, you are creating a new mutable ref each time

RalfJ (Jun 21 2019 at 19:37, on Zulip):

@Jake Goulding here's a better example: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=7c961f8815370ae5aa4d2674d5b99def

Jake Goulding (Jun 21 2019 at 19:48, on Zulip):

@RalfJ I’m not following what you mean by “creating a new mutable ref each time”. You mean via let me = unsafe { Pin::new_unchecked(&mut self.0) }; ? Other than breaking the guarantee of Pin, how is that bad?

RalfJ (Jun 21 2019 at 19:50, on Zulip):

it breaks (one version of) the notion that a mutable reference is unique

RalfJ (Jun 21 2019 at 19:50, on Zulip):

so it's basically the same as

let mut local = 0;
let raw = &mut local as *mut _;
let mut_ref = &mut local;
let _val = *raw; // UB! You violated aliasing rules.
RalfJ (Jun 21 2019 at 19:51, on Zulip):

also see https://github.com/rust-lang/unsafe-code-guidelines/issues/148

Jake Goulding (Jun 21 2019 at 19:56, on Zulip):

I’m gonna think on it a bit, but until I have my “aha!” moment…

This is how I would have “fixed” it, but Miri still complains; is this the point you are talking about?

RalfJ (Jun 21 2019 at 19:56, on Zulip):

yes

RalfJ (Jun 21 2019 at 19:57, on Zulip):

my "better example" shows an error that does not come from stacked borrows. dangling pointer is more fundamental and there's nothing to discuss there.

RalfJ (Jun 21 2019 at 19:57, on Zulip):

so that better demonstrates that your original code was wrong

RalfJ (Jun 21 2019 at 19:58, on Zulip):

but even correct self-referential generators currently fail to run in Miri (without -Zmiri-disable-validation), and that's a deep-rooted incompatibility between (the current approach to) self-referential generators and (the current version of) Stacked Borrows

Jake Goulding (Jun 21 2019 at 20:00, on Zulip):

Oh. Then this version with no unsafe code that triggers Miri might actually be OK…?

RalfJ (Jun 21 2019 at 20:21, on Zulip):

it better be, with no unsafe code :P

Jake Goulding (Jun 21 2019 at 20:23, on Zulip):

@RalfJ I know I’m not telling you anything new, but false positives for a tool like Miri are really bad for trustworthiness for such a tool.

Jake Goulding (Jun 21 2019 at 20:23, on Zulip):

If possible, it’d be wonderful to say “this error may not be valid due to …”

RalfJ (Jun 21 2019 at 20:24, on Zulip):

well we are in the exploratory phase of both Miri and Stacked Borrows. not sure what to do about that.^^

RalfJ (Jun 21 2019 at 20:25, on Zulip):

but I'll add to the "redo our error messages" task that it would be a good idea to distinguish "certain" from "less certain" UB errors

RalfJ (Jun 21 2019 at 20:26, on Zulip):

that will not be 100% reliable but it might help in situations like this

Jake Goulding (Jun 21 2019 at 20:26, on Zulip):

My worry is that during that phase, people might try to run Miri, see an error, then be told “oh that’s not a real error”. They internalize that and then in 3 years when we are past that stage they still think “Miri isn’t always right”

Jake Goulding (Jun 21 2019 at 20:27, on Zulip):

const generics is a recent example of an “easy” way out of this:

warning: the feature const_generics is incomplete and may cause the compiler to crash

Could Miri say something equivalent?

RalfJ (Jun 21 2019 at 20:27, on Zulip):

it says that in its README^^

RalfJ (Jun 21 2019 at 20:28, on Zulip):

I know that false positives are a problem. Not long ago you also warned me about false negatives. I'm afraid avoiding both is not going to be possible until we have a precise and complete spec, and part of the job of Miri is to help figuring out that spec. ;)

RalfJ (Jun 21 2019 at 20:31, on Zulip):

I share your worry though

RalfJ (Jun 21 2019 at 20:33, on Zulip):

I planned anyway to print next to the error some text saying whether this is UB or an operation not supported by Miri. So now I think it should say "definite UB" or "might be UB, this is experimental" or "unsupported"

Jake Goulding (Jun 21 2019 at 20:35, on Zulip):

I mean, the compiler doesn’t always avoid all false {negatives,positives} so you don’t have to be perfect.

README

(a) Who reads that :upside_down: (b) My main usage of Miri is via the playground’s button (and I just assume that’s true for everyone) so I was thinking in the output would be more obvious

So now I think it should say

That would be wonderful

RalfJ (Jun 21 2019 at 20:36, on Zulip):

great, now I just need to find that time machine that lets me actually implement this :D

RalfJ (Jun 21 2019 at 20:37, on Zulip):

currently just keeping Miri working (plus reviews etc) sucks up all my Rust time -- but that's also because of an approaching paper deadline

Jake Goulding (Jun 22 2019 at 13:23, on Zulip):

I think that cloning yourself is probably the more realistic solution :-)

Last update: Nov 19 2019 at 17:35UTC