Add lint to deny transmuting &T to &mut T #24392

seanmonstar · 2015-04-13T22:05:48Z

The UnsafeCell documentation says it is undefined behavior, so people shouldn't do it.

This happened to catch one case in libstd that was doing this, and I switched that to use an UnsafeCell internally.

Closes #13146

rust-highfive · 2015-04-13T22:05:52Z

r? @eddyb

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

Manishearth · 2015-04-14T13:23:54Z

src/librustc_lint/builtin.rs

+            if let ast::ExprPath(..) = expr.node {
+                if let DefFn(did, _) = ty::resolve_expr(cx.tcx, expr) {
+                    if def_id_is_transmute(cx, did) {
+                        let typ = ty::node_id_to_type(cx.tcx, expr.id);


Just use expr_ty?

I actually stole most of this code from rustc::middle::instrinsicck...

I'm not sure why we need to go through resolve_expr here, actually.

We have transmute(a), where transmute(a) is an ExprCall (where the first Expr is an ExprPath) and a is some form of Expr. Just running expr_ty on both should be enough to get the types, without going through BareFnTy and whatnot.

It seems it's needed to get to the DefId, so that we can determine if this method is actually std::mem::transmute, even if it's been imported as a different name. The BareFnTy is also needed to check the ABI.

Manishearth · 2015-04-14T13:32:45Z

I'll have another look later when I can access docs, but this looks good to me code-wise. There may be a way to avoid the csearch.

I'm not certain if we want to add more lints though. @alexcrichton thoughts?

alexcrichton · 2015-04-14T16:30:26Z

I personally feel that we have far too many lints as-is, but I'm also quite biased! I don't believe we have a strict policy on what lints are included and which are not.

seanmonstar · 2015-04-14T22:12:30Z

So, this doesn't need to be a lint. It could be built in to the instrinsicck code that checks all other kinds of transmutes. However, this one doesn't violate a size difference. Either way, docs saying "don't ever do this" but a compiler who happily allows it reminds me of C.

Manishearth · 2015-04-14T22:18:16Z

src/librustc_lint/builtin.rs

+            None
+        }
+
+        fn def_id_is_transmute(cx: &Context, def_id: DefId) -> bool {


I don't think we need the csearch here. Just check that the ABI is rust-intrinsic and that the path matches transmute

I don't know enough yet, so likely ignore me. However, it looks like csearch is needed if the fn def_id is not defined in the same crate (so most cases), right?

Well, you don't need to lookup the def, the name of the function is already with you in the ExprPath -- transmute(a) is an ExprCall(ExprPath("transmute"), vec![whatever a is])

Hm, good point!

@Manishearth oh, what about use std::mem::transmute as trans? I imagine that's why the original fn is being looked up?

Ah right. Lookup is necessary in that case.

steveklabnik · 2015-04-21T13:25:34Z

This would fix #13146

seanmonstar · 2015-04-22T04:59:06Z

@Manishearth okay, using .as_str() == "transmute" instead.

Also added [breaking-change] to the commit, since it technically is. However, it's UB without it, so the breakage seems worth it.

Also, nominate for beta, since breaking at 1.1 seems impossible?

Manishearth · 2015-04-22T08:18:26Z

triage: beta-nominated

wonder if that works

seanmonstar · 2015-04-23T17:42:09Z

Anything else required for this?

Manishearth · 2015-04-23T17:58:10Z

Looks good to me.

I'll let @alexcrichton r+ this though

pnkfelix · 2015-04-23T21:40:29Z

from nominated to (nominated, accepted)

nwin · 2015-04-24T14:57:59Z

Maybe the documentation of UnsafeCell should be reworded. That transmuting & to &mut is “in general” undefined behavior does not mean that it is undefined behavior per se. The reference makes much stronger arguments.

alexcrichton · 2015-04-24T18:53:52Z

I still personally feel that lints like this are too specific to be that beneficial. For example this does not provide an iron-clad guarantee that this kind of "undefined behavior" isn't actually happening. The biggest abuser of &mut self in the standard library, channels, is not caught by this change and probably wouldn't with any form of lint. I know that lints are not meant to provide an iron-clad guarantee, but I'm just not sure how useful this will end up being (especially being deny-by-default).

Manishearth · 2015-04-24T19:06:58Z

IMO if we crack down on UB now, even with semi effective solutions like this one; it will still be a net gain in the future when there isn't any UB abuse scattered all over the place.

-----Original Message-----
From: "Alex Crichton" [email protected]
Sent: ‎4/‎25/‎2015 12:24 AM
To: "rust-lang/rust" [email protected]
Cc: "Manish Goregaokar" [email protected]
Subject: Re: [rust] Add lint to deny transmuting &T to &mut T (#24392)

I still personally feel that lints like this are too specific to be that beneficial. For example this does not provide an iron-clad guarantee that this kind of "undefined behavior" isn't actually happening. The biggest abuser of &mut self in the standard library, channels, is not caught by this change and probably wouldn't with any form of lint. I know that lints are not meant to provide an iron-clad guarantee, but I'm just not sure how useful this will end up being (especially being deny-by-default).
—
Reply to this email directly or view it on GitHub.

bstrie · 2015-04-24T23:39:03Z

I happen to believe that any lint that's supposed to be so important to be deny-by-default should be a feature of the language proper rather than being left up to a lint. I dunno, maybe the perfect is the enemy of the good...

Manishearth · 2015-04-25T09:00:42Z

I'm open to warn by default

seanmonstar · 2015-04-25T18:58:26Z

Since this is undefined behavior, I think it should be deny by default. If you somehow know better, you can add an allow.

I originally thought of adding this code to intrinsicsck, but several in #rust-internals said it seemed wrong since that's only supposed to enforce the values are the same size.

alexcrichton · 2015-04-27T17:58:54Z

I do agree with @bstrie that it seems weird to implement a check for undefined behavior as a lint instead of some sort of other error. A lint is an admission that "sure, you may want to actually do this sometimes, just not all the time", but I'm not sure that we have a set of behavior where you would indeed want this kind of a transmute.

Manishearth · 2015-04-27T18:05:19Z

I don't know, I'm wary of building in "no, you should never need this" into a language; there should always be a way to do stuff like this, just that there are more hoops to jump through.

seanmonstar · 2015-04-27T19:17:52Z

@alexcrichton I guess it's technically not undefined behavior to transmute if you already are using an UnsafeCell? Such as:

unsafe {
    let val = &*self.foo.get();
    let val_mut: &mut Foo = transmute(val);
}

Though, at that point, you should likely just do &mut *self.foo.get(). Again, if this should always be an error, not something that can be turned off, I can move it to intrinsicck, if that seems better.

seanmonstar · 2015-05-04T20:57:31Z

@alexcrichton I think that quote from nwin is regarding the repr + drop lint, not this one?

As for motivation, it actually came from seeing it appear in crates on crates.io, such as the issue I linked regarding pool, which did this transmute in several places. I likewise started with a transmute in hyper's OptCell, until I noticed that line in the UnsafeCell docs, and scrambled to fix it.

I genuinely think that more often than not, it's a mistake because people don't realize it is undefined behavior, and pointing it out and suggesting UnsafeCell is the right thing to do. Those who think they know better, can add an allow in special cases.

seanmonstar · 2015-05-05T01:53:21Z

Another instance: http://www.reddit.com/r/rust/comments/34u40n/abomonation_terrifying_serialization/cqynb6u

nwin · 2015-05-05T07:17:55Z

@seanmonstar yes it was not about this one, admittedly ambiguously formulated.

llogiq · 2015-05-05T07:18:58Z

tomaka:

But is there really a situation where someone would say: "oh, this lint detected a mistake I made, let me fix my code"?

Lints – and static code analysis in general – are regarded very positively in many language communities. E.g. many front-end devs won't allow code that doesn't pass jslint, and I (a seasoned Java dev with more than a decade of experience) have FindBugs check my code at all times. It's cheap and will sometimes catch oversights. Errors happen. Having an additional safety net cannot hurt. Rust is already very good in this regard.

So while I certainly agree that not all lints belong in rustc, I don't feel that we have too many. But I'm just a rust newbie, so feel free to ignore my opinion. 😄

frankmcsherry · 2015-05-05T07:31:45Z

I'm with @seanmonstar on this one. The issue that I had (linked above) was something that a) seems to work fine, b) I had every reason to believe would work (exclusive ownership was guaranteed elsewhere), and c) should never be allowed by anyone serious about things.

The suggestion to do the transmute was made by a seasoned (unnamed) Rust-y in IRC. Now that I've internalized some of the things on the UB link I know a bit better, but I could really use the help from lints written by smarter folks.

alexcrichton · 2015-05-05T16:58:23Z

@bors: r+ f224446

bors · 2015-05-05T17:30:22Z

⌛ Testing commit f224446 with merge f872a82...

bors · 2015-05-05T18:18:26Z

💔 Test failed - auto-mac-64-opt

seanmonstar · 2015-05-05T18:41:38Z

@alexcrichton updated the failing run-pass tests

alexcrichton · 2015-05-05T18:43:24Z

src/test/auxiliary/issue_8401.rs

@@ -22,5 +22,5 @@ fn bar<T>(_: &mut A, _: &T) {}

 fn foo<T>(t: &T) {
    let b = B;
-    bar(unsafe { mem::transmute(&b as &A) }, t)
+    bar(&mut b as &mut A, t)


Did you run this test? I think that the binding to b needs to be mut for this to work? (I'd recommend being sure to run the tests for these cases).

I pushed before running (the run caught what you noticed), since I needed to rebuild and it'd take a while. It's all done, and I'm pushing the corrections that make these 2 tests pass.

alexcrichton · 2015-05-05T22:24:15Z

@bors: r+ 5b0d828

bors · 2015-05-06T02:42:12Z

⌛ Testing commit 5b0d828 with merge e609b8a...

bors · 2015-05-06T03:33:41Z

💔 Test failed - auto-mac-64-opt

…d behavior [breaking-change] Technically breaking, since code that had been using these transmutes before will no longer compile. However, it was undefined behavior, so really, it's a good thing. Fixing your code would require some re-working to use an UnsafeCell instead. Closes rust-lang#13146

seanmonstar · 2015-05-06T05:28:51Z

Sigh, had improved the lint message from review comments, but forgot to adjust the expected error in the compile-fail. Then the subsequent check-stage1-rpass for the previous failure hadn't caught it either.

Now cfail passes as well.

alexcrichton · 2015-05-06T16:10:19Z

@bors: r+ 5624cfb

The [UnsafeCell documentation says it is undefined behavior](http://doc.rust-lang.org/nightly/std/cell/struct.UnsafeCell.html), so people shouldn't do it. This happened to catch one case in libstd that was doing this, and I switched that to use an UnsafeCell internally. Closes #13146

bors · 2015-05-06T22:27:59Z

⌛ Testing commit 5624cfb with merge 8767e97...

bors · 2015-05-07T00:20:23Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

apoelstra · 2015-05-10T18:14:50Z

Thanks a ton for merging this; to add to the support, after this lint appeared I found the bug in eventual carllerche/eventual#21 and in my own rust-bitcoin.

rust-highfive assigned eddyb Apr 13, 2015

seanmonstar mentioned this pull request Apr 13, 2015

Pool.inner_mut() and other mut methods are UB carllerche/pool#2

Closed

seanmonstar force-pushed the lint-transmute-mut branch from 6a9a49e to 5fc357e Compare April 13, 2015 23:09

Manishearth reviewed Apr 14, 2015
View reviewed changes

seanmonstar force-pushed the lint-transmute-mut branch from 5fc357e to 09047f7 Compare April 22, 2015 04:57

dotdash added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 22, 2015

nrc assigned alexcrichton and unassigned eddyb Apr 23, 2015

pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 23, 2015

seanmonstar force-pushed the lint-transmute-mut branch from f224446 to 50d406b Compare May 5, 2015 18:41

alexcrichton reviewed May 5, 2015
View reviewed changes

seanmonstar force-pushed the lint-transmute-mut branch from 50d406b to 5b0d828 Compare May 5, 2015 19:16

seanmonstar added 3 commits May 5, 2015 22:26

std: update select internals to not use mutable transmuting

e136f93

test: update run-pass tests to not use mutable transmuting

5624cfb

seanmonstar force-pushed the lint-transmute-mut branch from 5b0d828 to 5624cfb Compare May 6, 2015 05:26

bors merged commit 5624cfb into rust-lang:master May 7, 2015

alexcrichton mentioned this pull request May 7, 2015

Backport of PRs to Beta #25192

Merged

brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 8, 2015

Add lint to deny transmuting &T to &mut T #24392

Add lint to deny transmuting &T to &mut T #24392

Conversation

seanmonstar commented Apr 13, 2015

rust-highfive commented Apr 13, 2015

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

Manishearth commented Apr 14, 2015

alexcrichton commented Apr 14, 2015

seanmonstar commented Apr 14, 2015

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

steveklabnik commented Apr 21, 2015

seanmonstar commented Apr 22, 2015

Manishearth commented Apr 22, 2015

seanmonstar commented Apr 23, 2015

Manishearth commented Apr 23, 2015

pnkfelix commented Apr 23, 2015

nwin commented Apr 24, 2015

alexcrichton commented Apr 24, 2015

Manishearth commented Apr 24, 2015

bstrie commented Apr 24, 2015

Manishearth commented Apr 25, 2015

seanmonstar commented Apr 25, 2015

alexcrichton commented Apr 27, 2015

Manishearth commented Apr 27, 2015

seanmonstar commented Apr 27, 2015

seanmonstar commented May 4, 2015

seanmonstar commented May 5, 2015

nwin commented May 5, 2015

llogiq commented May 5, 2015

frankmcsherry commented May 5, 2015

alexcrichton commented May 5, 2015

bors commented May 5, 2015

bors commented May 5, 2015

seanmonstar commented May 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented May 5, 2015

bors commented May 6, 2015

bors commented May 6, 2015

seanmonstar commented May 6, 2015

alexcrichton commented May 6, 2015

bors commented May 6, 2015

bors commented May 7, 2015

apoelstra commented May 10, 2015