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

seanmonstar · Apr 13, 2015

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 · Apr 13, 2015

r? @eddyb

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

Manishearth · Apr 14, 2015

Manishearth reviewed Apr 14, 2015

View changes

Manishearth · Apr 14, 2015

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 · Apr 14, 2015

Manishearth reviewed Apr 14, 2015

View changes

Manishearth · Apr 14, 2015

src/librustc_lint/builtin.rs

+            if def_id.krate == ast::LOCAL_CRATE {
+                match cx.tcx.map.get(def_id.node) {
+                    NodeForeignItem(ref item) if intrinsic => {
+                        token::get_ident(item.ident) ==


nit: we should probably compare the strings (there is a get() or a deref impl on Ident), not idents

Edit: as_str()

Why compare the strings? It seems token::get_ident() doesn't return an ast::Ident (the fn name is misleading), but an InternedString. Comparing an interned string should be strictly faster than pulling out the original Strings and then doing a memcmp, right?

Yeah, but you have to intern a string to get there, so you do the comparison anyway in the interning.

item.ident is an Ident and can be directly compared with "transmute" after an as_str()

Manishearth · Apr 14, 2015

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 · Apr 14, 2015

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 · Apr 14, 2015

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 · Apr 14, 2015

Manishearth reviewed Apr 14, 2015

View changes

Manishearth · Apr 14, 2015

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 · Apr 21, 2015

This would fix #13146

seanmonstar · Apr 22, 2015

@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 · Apr 22, 2015

triage: beta-nominated

wonder if that works

seanmonstar · Apr 23, 2015

Anything else required for this?

Manishearth · Apr 23, 2015

Looks good to me.

I'll let @alexcrichton r+ this though

pnkfelix · Apr 23, 2015

from nominated to (nominated, accepted)

nwin · Apr 24, 2015

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 · Apr 24, 2015

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 · Apr 24, 2015

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" notifications@github.com
Sent: ‎4/‎25/‎2015 12:24 AM
To: "rust-lang/rust" rust@noreply.github.com
Cc: "Manish Goregaokar" manishsmail@gmail.com
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 · Apr 24, 2015

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 · Apr 25, 2015

I'm open to warn by default

seanmonstar · Apr 25, 2015

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 · Apr 27, 2015

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 · Apr 27, 2015

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 · Apr 27, 2015

@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 · May 4, 2015

@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 · May 5, 2015

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

nwin · May 5, 2015

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

llogiq · May 5, 2015

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 · May 5, 2015

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 · May 5, 2015

@bors: r+ f224446

bors · May 5, 2015

⌛️ Testing commit f224446 with merge f872a82...

bors · May 5, 2015

💔 Test failed - auto-mac-64-opt

seanmonstar · May 5, 2015

@alexcrichton updated the failing run-pass tests

alexcrichton · May 5, 2015

alexcrichton reviewed May 5, 2015

View changes

alexcrichton · May 5, 2015

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 · May 5, 2015

@bors: r+ 5b0d828

bors · May 6, 2015

⌛️ Testing commit 5b0d828 with merge e609b8a...

bors · May 6, 2015

💔 Test failed - auto-mac-64-opt

seanmonstar · May 6, 2015

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 · May 6, 2015

@bors: r+ 5624cfb

bors · May 6, 2015

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

bors · May 7, 2015

☀️ 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 · May 10, 2015

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 referenced this pull request Apr 13, 2015
Closed
Pool.inner_mut() and other mut methods are UB #2

seanmonstar force-pushed the seanmonstar:lint-transmute-mut branch from 6a9a49e to 5fc357e Apr 13, 2015

seanmonstar force-pushed the seanmonstar:lint-transmute-mut branch from 5fc357e to 09047f7 Apr 22, 2015

dotdash added the beta-nominated label Apr 22, 2015

nrc assigned alexcrichton and unassigned eddyb Apr 23, 2015

pnkfelix added the beta-accepted label Apr 23, 2015

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

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

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

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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

alexcrichton referenced this pull request May 7, 2015
Merged
Backport of PRs to Beta #25192

brson removed the beta-nominated label May 8, 2015

rust-lang/rust

Join GitHub today

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

Conversation

seanmonstar commented Apr 13, 2015

rust-highfive assigned eddyb Apr 13, 2015

This comment has been minimized.

rust-highfive commented Apr 13, 2015

seanmonstar referenced this pull request Apr 13, 2015

seanmonstar force-pushed the seanmonstar:lint-transmute-mut branch from 6a9a49e to 5fc357e Apr 13, 2015

Manishearth reviewed Apr 14, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth reviewed Apr 14, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Apr 14, 2015

This comment has been minimized.

alexcrichton commented Apr 14, 2015

This comment has been minimized.

seanmonstar commented Apr 14, 2015

Manishearth reviewed Apr 14, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

steveklabnik commented Apr 21, 2015

seanmonstar force-pushed the seanmonstar:lint-transmute-mut branch from 5fc357e to 09047f7 Apr 22, 2015

This comment has been minimized.

seanmonstar commented Apr 22, 2015

This comment has been minimized.

Manishearth commented Apr 22, 2015

dotdash added the beta-nominated label Apr 22, 2015

This comment has been minimized.

seanmonstar commented Apr 23, 2015

This comment has been minimized.

Manishearth commented Apr 23, 2015

nrc assigned alexcrichton and unassigned eddyb Apr 23, 2015

pnkfelix added the beta-accepted label Apr 23, 2015

This comment has been minimized.

pnkfelix commented Apr 23, 2015

This comment has been minimized.

nwin commented Apr 24, 2015

This comment has been minimized.

alexcrichton commented Apr 24, 2015

This comment has been minimized.

Manishearth commented Apr 24, 2015

This comment has been minimized.

bstrie commented Apr 24, 2015

This comment has been minimized.

Manishearth commented Apr 25, 2015

This comment has been minimized.

seanmonstar commented Apr 25, 2015

This comment has been minimized.

alexcrichton commented Apr 27, 2015

This comment has been minimized.

Manishearth commented Apr 27, 2015

This comment has been minimized.

seanmonstar commented Apr 27, 2015

This comment has been minimized.

seanmonstar commented May 4, 2015

This comment has been minimized.

seanmonstar commented May 5, 2015

This comment has been minimized.

nwin commented May 5, 2015

This comment has been minimized.

llogiq commented May 5, 2015

This comment has been minimized.

frankmcsherry commented May 5, 2015

This comment has been minimized.

alexcrichton commented May 5, 2015

bors added a commit that referenced this pull request May 5, 2015

This comment has been minimized.

seanmonstar force-pushed the seanmonstar:lint-transmute-mut branch from `6a9a49e` to `5fc357e` Apr 13, 2015

Manishearth reviewed Apr 14, 2015

View changes

Manishearth reviewed Apr 14, 2015

View changes

Manishearth reviewed Apr 14, 2015

View changes

seanmonstar force-pushed the seanmonstar:lint-transmute-mut branch from `5fc357e` to `09047f7` Apr 22, 2015

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

alexcrichton reviewed May 5, 2015

View changes

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

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

bors merged commit `5624cfb` into rust-lang:master May 7, 2015
2 checks passed