Disallow mutable borrow to non-mut statics #47543

topecongiro · 2018-01-18T14:47:42Z

rust-highfive · 2018-01-18T14:47:47Z

(rust_highfive has picked a reviewer for you, use r? to override)

topecongiro · 2018-01-18T14:52:05Z

Hmm, I just realized that when nll is enabled, the error message is slightly different.
Is this acceptable?

with nll

error[E0596]: cannot borrow immutable item `*TAB[..]` as mutable
  --> test.rs:16:5
   |
16 |     TAB[0].iter_mut();
   |     ^^^^^^ cannot borrow as mutable
   |
   = note: Value not mutable causing this error: `TAB`

error: aborting due to previous error

without nll

error[E0596]: cannot borrow borrowed content of mutable binding as mutable
  --> test.rs:14:5
   |
14 |     TAB[0].iter_mut();
   |     ^^^^^^ cannot borrow as mutable

error: aborting due to previous error

eddyb · 2018-01-18T15:09:40Z

r? @nikomatsakis

nikomatsakis · 2018-01-18T22:37:12Z

@topecongiro that's acceptable, it's a separate issue

nikomatsakis · 2018-01-19T13:26:13Z

tbh, though, I find the MIR borrowck error much easier to understand =)

nikomatsakis

Thanks! =)

nikomatsakis · 2018-01-19T23:35:24Z

src/librustc_borrowck/borrowck/gather_loans/mod.rs

@@ -233,7 +233,17 @@ fn check_mutability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
                                           cmt,
                                           code: err_mutbl }))
            } else {
-                Ok(())
+                match cmt.maybe_static_mutability() {


Something about this check doesn't feel right. There is a specific check that is supposed to guarantee that the reference is not an "aliasable" location, and it seems to be going awry, but I feel like we should be fixing that check, and not adding a new one. This is the check I am talking about:

rust/src/librustc_borrowck/borrowck/gather_loans/mod.rs

Lines 164 to 203 in 5965b79

/// Implements the A-* rules in README.md.

fn check_aliasability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,

borrow_span: Span,

loan_cause: AliasableViolationKind,

cmt: mc::cmt<'tcx>,

req_kind: ty::BorrowKind)

-> Result<(),()> {

let aliasability = cmt.freely_aliasable();

debug!("check_aliasability aliasability={:?} req_kind={:?}",

aliasability, req_kind);

match (aliasability, req_kind) {

(mc::Aliasability::NonAliasable, _) => {

/* Uniquely accessible path -- OK for `&` and `&mut` */

Ok(())

}

(mc::Aliasability::FreelyAliasable(mc::AliasableStatic), ty::ImmBorrow) => {

// Borrow of an immutable static item.

Ok(())

}

(mc::Aliasability::FreelyAliasable(mc::AliasableStaticMut), _) => {

// Even touching a static mut is considered unsafe. We assume the

// user knows what they're doing in these cases.

Ok(())

}

(mc::Aliasability::FreelyAliasable(alias_cause), ty::UniqueImmBorrow) |

(mc::Aliasability::FreelyAliasable(alias_cause), ty::MutBorrow) => {

bccx.report_aliasability_violation(

borrow_span,

loan_cause,

alias_cause,

cmt);

Err(())

}

(..) => {

Ok(())

}

}

}

Can you maybe tell me what the cmt is in this case, and what the value of aliasability is?

The cmt is

cmt=cmt_ { id: NodeId(16), span: test.rs:13:5: 13:11, cat: Deref(cmt_ { id: NodeId(16), span: test.rs:13:5: 13:11, cat: Interior(cmt_ { id: NodeId(14), span: test.rs:13:5: 13:8, cat: StaticItem, mutbl: McImmutable, ty: [&'static mut [u8]; 0], note: NoteNone, }, []), mutbl: McImmutable, ty: &'static mut [u8], note: NoteNone, }, BorrowedPtr(MutBorrow, ReStatic)), mutbl: McDeclared, ty: [u8], note: NoteNone }

and the value of aliasability is

aliasability=FreelyAliasable(AliasableStatic)

Huh, so I wonder in that case why this code:

rust/src/librustc_borrowck/borrowck/gather_loans/mod.rs

Lines 190 to 198 in 5965b79

(mc::Aliasability::FreelyAliasable(alias_cause), ty::UniqueImmBorrow) |

(mc::Aliasability::FreelyAliasable(alias_cause), ty::MutBorrow) => {

bccx.report_aliasability_violation(

borrow_span,

loan_cause,

alias_cause,

cmt);

Err(())

}

doesn't execute... req_kind is ty::MutBorrow, right?

That is because check_mutability triggers an error, and check_aliasability is no called at all.

Huh. So how come I don't see an error reporting in the original issue #42344? I see the ICE -- where does that ICE occur?

Oh, were you asking about the behavior of the original rustc? I was talking about how rustc bahaves when this PR was applied. I am sorry for the confusion.

Using the master branch, yes, req_kind is ty::MutBorrow, and bccx.report_aliasability_violation is executed. The following error is reported.

error: internal compiler error: aliasability violation for static `cannot borrow data mutably` --> test.rs:3:5 | 3 | TAB[0].iter_mut(); | ^^^^^^ error: aborting due to previous error

@topecongiro ah, I see -- actually, I think that the delay_span_bug is just wrong and should be removed. The logic was that a case like static X: u32 = 2; &mut X should not reach there (and indeed I think it won't) -- but it overlooked the possibility of an &mut living in a static.

Can you update this PR to just remove the delay_span_bug call for AliasabilityReason::Static?

Sure! Thank your for your patience with my slow updates.

shepmaster · 2018-01-26T17:50:41Z

Ping from triage, @topecongiro ! Will you be able to respond to the most recent set of feedback soon?

shepmaster · 2018-01-26T17:53:28Z

Disallow mutable borrow to non-mut statics

I got excited and thought this fixed #46557 😝

topecongiro · 2018-01-26T20:37:18Z

Oops, I missed the notification of review by @nikomatsakis. Sorry!

I will work on this today at some point.

nikomatsakis · 2018-01-29T17:18:42Z

@topecongiro left another question :)

This path was considered to be unreachable. However, `&mut` could potentially live inside `static`. For example, `static TAB: [&mut [u8]; 0] = [];`.

nikomatsakis · 2018-02-05T17:17:24Z

@bors r+

Nice =)

bors · 2018-02-05T17:17:25Z

📌 Commit 3d114c7 has been approved by nikomatsakis

…akis Disallow mutable borrow to non-mut statics Closes rust-lang#42344.

Rollup of 10 pull requests - Successful merges: #46030, #47496, #47543, #47704, #47753, #47807, #47948, #47959, #48003, #48007 - Failed merges:

rust-highfive assigned eddyb Jan 18, 2018

rust-highfive assigned nikomatsakis and unassigned eddyb Jan 18, 2018

nikomatsakis requested changes Jan 19, 2018

View reviewed changes

shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 20, 2018

topecongiro force-pushed the issue-42344 branch from 9c26ca7 to ce21516 Compare February 4, 2018 08:34

Remove delay_span_bug() in check_aliasability

3d114c7

This path was considered to be unreachable. However, `&mut` could potentially live inside `static`. For example, `static TAB: [&mut [u8]; 0] = [];`.

topecongiro force-pushed the issue-42344 branch from ce21516 to 3d114c7 Compare February 4, 2018 10:15

kennytm added a commit to kennytm/rust that referenced this pull request Feb 5, 2018

Rollup merge of rust-lang#47543 - topecongiro:issue-42344, r=nikomats…

9dab737

…akis Disallow mutable borrow to non-mut statics Closes rust-lang#42344.

kennytm mentioned this pull request Feb 5, 2018

Rollup of 10 pull requests #48017

Merged

bors added a commit that referenced this pull request Feb 6, 2018

Auto merge of #48017 - kennytm:rollup, r=kennytm

b224fc8

Rollup of 10 pull requests - Successful merges: #46030, #47496, #47543, #47704, #47753, #47807, #47948, #47959, #48003, #48007 - Failed merges:

bors merged commit 3d114c7 into rust-lang:master Feb 6, 2018

topecongiro deleted the issue-42344 branch February 6, 2018 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow mutable borrow to non-mut statics #47543

Disallow mutable borrow to non-mut statics #47543

topecongiro commented Jan 18, 2018

rust-highfive commented Jan 18, 2018

topecongiro commented Jan 18, 2018

eddyb commented Jan 18, 2018

nikomatsakis commented Jan 18, 2018

nikomatsakis commented Jan 19, 2018

nikomatsakis left a comment

nikomatsakis Jan 19, 2018

topecongiro Jan 27, 2018

nikomatsakis Jan 29, 2018

topecongiro Jan 29, 2018

nikomatsakis Jan 30, 2018

topecongiro Jan 31, 2018

nikomatsakis Jan 31, 2018

nikomatsakis Jan 31, 2018

topecongiro Feb 4, 2018

shepmaster commented Jan 26, 2018

shepmaster commented Jan 26, 2018

topecongiro commented Jan 26, 2018

nikomatsakis commented Jan 29, 2018

nikomatsakis commented Feb 5, 2018

bors commented Feb 5, 2018

	/// Implements the A-* rules in README.md.
	fn check_aliasability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
	borrow_span: Span,
	loan_cause: AliasableViolationKind,
	cmt: mc::cmt<'tcx>,
	req_kind: ty::BorrowKind)
	-> Result<(),()> {

	let aliasability = cmt.freely_aliasable();
	debug!("check_aliasability aliasability={:?} req_kind={:?}",
	aliasability, req_kind);

	match (aliasability, req_kind) {
	(mc::Aliasability::NonAliasable, _) => {
	/* Uniquely accessible path -- OK for `&` and `&mut` */
	Ok(())
	}
	(mc::Aliasability::FreelyAliasable(mc::AliasableStatic), ty::ImmBorrow) => {
	// Borrow of an immutable static item.
	Ok(())
	}
	(mc::Aliasability::FreelyAliasable(mc::AliasableStaticMut), _) => {
	// Even touching a static mut is considered unsafe. We assume the
	// user knows what they're doing in these cases.
	Ok(())
	}
	(mc::Aliasability::FreelyAliasable(alias_cause), ty::UniqueImmBorrow) \|
	(mc::Aliasability::FreelyAliasable(alias_cause), ty::MutBorrow) => {
	bccx.report_aliasability_violation(
	borrow_span,
	loan_cause,
	alias_cause,
	cmt);
	Err(())
	}
	(..) => {
	Ok(())
	}
	}
	}

Disallow mutable borrow to non-mut statics #47543

Disallow mutable borrow to non-mut statics #47543

Conversation

topecongiro commented Jan 18, 2018

rust-highfive commented Jan 18, 2018

topecongiro commented Jan 18, 2018

with nll

without nll

eddyb commented Jan 18, 2018

nikomatsakis commented Jan 18, 2018

nikomatsakis commented Jan 19, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shepmaster commented Jan 26, 2018

shepmaster commented Jan 26, 2018

topecongiro commented Jan 26, 2018

nikomatsakis commented Jan 29, 2018

nikomatsakis commented Feb 5, 2018

bors commented Feb 5, 2018