Transmuting known null ptr to ref

[felix91gr]

Working on implementing #628

[felix91gr]

@flip1995 this is very much a WIP (the cool thing is that GH now lets me mark it as WIP ^^)

I wanted to ask for your help: why do I get the following error?

error[E0277]: the trait bound `for<'a, 'tcx> &'static rustc::lint::Lint: rustc::lint::LateLintPass<'a, 'tcx>` is not satisfied
   --> clippy_lints/src/lib.rs:574:33
    |
574 |     reg.register_late_lint_pass(box transmuting_null::TRANSMUTING_NULL);
    |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `for<'a, 'tcx> rustc::lint::LateLintPass<'a, 'tcx>` is not implemented for `&'static rustc::lint::Lint`
    |
    = note: required for the cast to the object type `dyn for<'a, 'tcx> rustc::lint::LateLintPass<'a, 'tcx> + rustc_data_structures::sync::Send + rustc_data_structures::sync::Sync`

error: aborting due to previous error

I thought I'd copied everything relevant from another Late-Pass Lint, but clearly I've missed something ^^'
I don't understand this error. Where does it stem from?

[felix91gr]

I have now started work on the lint proper.

I know how to identify a transmute call. However, I'll either need some kind of book keeping in order to know if there's a path that leads to the null pointer being cast, or something like Miri so that I can directly test if the variable being set as null still is null when entering the transmute call.

Any pointers on the book keeping idea would be appreciated. This is what I want to do:

• Check an assignment. If an assignment is made of null to a pointer, then remember that variable.
• If the variable is immutable, awesome.
• If not, check all subsequent assignments made to that variable.
• If before that variable is set to a non-null value, it is given to a transmute function, then we know this is an error.

Bonus question: if the variable is shadowed, would it matter? Would my reference to the variable being shadowed appear in subsequent checks?

Thanks in advance.

[felix91gr]

The author lint is helping me catch the one-liner offenders. The problem is the multi-liner ones. Is it possible to use the author lint to catch an entire scope/block of code?

[felix91gr]

I think I'm now catching all the one-liners. @flip1995 I now need your help on how to proceed with cases that span more than one statement :)

[felix91gr]

Also please check my UI test, maybe I'm still lacking some examples of "known nulls": https://github.com/rust-lang/rust-clippy/pull/3848/files#diff-9c393f2ec14c6de7f3cc2117ba4b7514

[flip1995]

Before implementing all the cases by hand you should take a look at clippy_lints/src/constants.rs. There should be a method which you should be able to call on the argument of the transmute call. With a little bit of luck, this already covers all/most of the cases.

Please also push a WIP *.stderr file. This makes reviewing easier since I can see what already works directly.

[felix91gr]

Please also push a WIP *.stderr file. This makes reviewing easier since I can see what already works directly.

Oh! Of course! I thought I might as well wait for completion before pushing it, given it's not ready yet. But I didn't think you might need it; it makes so much sense to me now. I'm gonna push it :)

Edit: done ^^

[felix91gr]

you should take a look at clippy_lints/src/constants.rs

You mean clippy_lints/src/consts.rs?

[felix91gr]

It would be this, right?

[felix91gr]

Okay, this is what I have for multi-statement cases. I create a ConstEvalLateContext, and ask it to find the value of the expression that's being used as the first argument to std::mem::transmute. The problem is that it gives me None (as you can see at the end, I'm debugging it by displaying it as a lint).

Maybe this particular evaluation hasn't been implemented yet? 🤔

fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
    if_chain! {
            if let ExprKind::Call(ref func, ref args) = expr.node;
            if let ExprKind::Path(ref path) = func.node;
            if match_qpath(path, &["std", "mem", "transmute"]);
            if args.len() == 1;
            
            then {

                // Catching transmute over variables or constants 
                // that resolve to `null`
                // FIXME: make it work
                let mut const_eval_context = constant_context(cx, cx.tables);
                if_chain! {
                    if let ExprKind::Path(ref qpath) = args[0].node;

                    then {
                        let x = const_eval_context.expr(&args[0]);

                        let s = format!("{:?}", x);
                        span_lint(
                            cx, 
                            TRANSMUTING_NULL, 
                            expr.span, 
                            &s)
                    }
                }
         // Omitted the rest of the function because it's not relevant for this case

[felix91gr]

I think it all amounts to the fact that in clippy_lints/src/consts.rs, the fetch_path method only has the lookup implementation for Def::Const and Def::AssociatedConst. For all other variants, the method returns None.

We're missing almost all of the cases.

[felix91gr]

I think for the purposes of this PR, we're missing an implementation for the following variants of the rustc::hir::def::Def enum, in order of importance:

• Def::Local: local variables, the (arguably) most common use-case.
• Def::Fn: for if the user makes a function that returns a null pointer and breaks their entire codebase through transmutes.
• Def::Static: it associates values with types, right? There we could have a variable that is a null pointer.
• Def::Upvar, Def::Label? What do they mean? Is Label the label for loops? What's Upvar then? Anyways, I'm including them in this list because I couldn't discard them with my current knowledge.

[felix91gr]

The miri_to_const function at consts.rs is incomplete as well. The std::mem::transmute(ZPTR); line gets recognized by the fetch_path implementation, but when it's sent to miri_to_const for its final resolution, a None is returned. So we have that as well... >.<'

[felix91gr]

I have pushed a change that lets you now evaluate a const that has a raw pointer inside of it. And the lint captures that!

[felix91gr]

Also: the only thing we need at fetch_path seems to be Def::Local: all the uses I've written in the test case that are not const being given to the transmute are of type Def::Local.

[felix91gr]

The Def::Local variant might need a hir-dification, because it still uses NodeId. Should we wait on that?

OTOH, this PR already should be able to catch:

• One-liner null transmutes
• Const-that-are-equal-to-null transmutes

If that's useful enough, we could merge what there is for now and wait for more hir-dification to happen before completing the feature.

[felix91gr]

Nevermind the Hir thing. I think the only thing we really need is a way to interpret the value of a Def::Local expression. Do you think we can?

[felix91gr]

About the error messages:

Here the first three are actual linting triggers. The next one (Some(RawPtr(1))) is a fake that's not triggering by the lint (which is good), but appears because I'm showing the Interpreted information there in order to debug.

The last three are that, as well. But those three should trigger a lint, if we manage to interpret the value of a Def::Local expression somehow.

[flip1995]

Thanks for doing all the work and the research! I'll take a closer look at this, this evening.

The stderr looks good so far. It seems that the only thing left to do is to resolve local vars.

[flip1995]

I took a closer look at this, but couldn't figure out how to handle the Local case yet. I will take another look tomorrow.

For now you just get a nice to know:

[oli-obk]

Doing this with let bindings will be night impossible to get right without making all of this a MIR lint. Even then, we'll be duplicating a lot of code from rustc's const propagation pass. I'm not sure if we should try to go beyond simple const folding in the AST.

[felix91gr]

Doing this with let bindings will be night impossible to get right without making all of this a MIR lint

What do you mean? As in, with things like check_expr which allow to only look at 1 expr at a time?

Btw regarding MIR and subsequent passes: we only intend to check things that the compiler is able to deduce are null at compile time. Isn't there an optimization pass that will ultimately change this code:

let zero : *const u64 = 0 as *const u64;
         
let zero_ts : &u64 = std::mem::transmute(zero);

Into this, using constant propagation:

let zero_ts : &u64 = std::mem::transmute(0 as *const u64);

I was thinking that if that pass exists, then maybe we can run the lint after that pass, and check directly inside the transmute call whether or not its parameter turned out to be a "known null".

Even then, we'll be duplicating a lot of code from rustc's const propagation pass.

I'd say that depending on the APIs we have access to, I'm not against reimplementing some code. But if you say this, I imagine it's probably because it must be quite some work and it's not all that feasible.

I'm not sure if we should try to go beyond simple const folding in the AST.

In this context, is const folding what we've being doing? Or is it what I asked about in the paragraph above? (what I called "constant propagation")

[oli-obk]

using constant propagation:

Yes, that's what would happen if we implemented this as a MIR lint. Basically right now we would only get std::mem::transmute(0 as *const u64), but in the future we automatically get more advanced checks when the MIR optimizations end up being turned on

[felix91gr]

Let's do that, then. What do I need to do in order to change this to a MIR lint?

[bors]

☔ The latest upstream changes (presumably #3767) made this pull request unmergeable. Please resolve the merge conflicts.

[flip1995]

What do I need to do in order to change this to a MIR lint?

Sadly there is no nice interface for MIR based lints, like there is for AST and HIR. (At least AFAIK @oli-obk ?)

For now I would suggest to just do this with the HIR information we have available.

[felix91gr]

In the last couple of days, I've been asking Oliver tons and tons of questions to understand what parts of MIR can be used here, and for what purposes :)

With what I know now, I should be able to reimplement a bit of what I did already, making it work using the MIR tooling instead of the code present in consts.rs. The parts I can't reimplement with MIR I'd leave to work as they are now.

The Local case we were talking about above seems to be out of reach for now, regardless of the approach we use (unless we implement a constant propagator inside the lint, which seems a bit crazy! ^^').

However, using the MIR tooling should leave the door open for future improvements coming for free from MIR itself to the lint. After the constant propagation optimization gets more stable there, it should even cover the Local case.

We'll need a tracking issue (I assume at rustc's repo?) for the MIR opts, so that when those are ready, we can update the tests here to cover those cases.

[felix91gr]

PS: it has been updated :)

[flip1995]

Just the 2 small points above, a CI pass and this can get merged.

[felix91gr]

Ran rustfmt over the new lint's file and test file, and over consts.rs! :D

[felix91gr]

Now that #3905 has been merged, we can re-run CI over this :)

[felix91gr]

Aw dang, it failed on CI. Now I'm out of the computer, I'll go back to this if not tonight, tomorrow :)

[felix91gr]

Yay, Travis passed! The PR is finally ready ^^

[flip1995]

rustfmt to remove trailing whitespaces (?):
https://travis-ci.com/rust-lang/rust-clippy/jobs/189561870#L1583-L1584

[felix91gr]

Done!
(I need to fix my vscode setup bc it's not running rustfmt since I started running the master toolchain 😱)

[felix91gr]

Crap, because I don't understand how to update my master branch using the Github interface, I've fell into the habit of deleting my fork and remaking it in order to make a new Pull Request.

I have deleted it. This PR seems to still work? But Github says the PR comes from "unkown repository". Please tell me if it still works, otherwise I'll make another branch and put my local copy of the PR into it 😱

Sorry for the inconvenience!! 🙇‍♂️

[phansch]

You can try git fetch upstream pull/3848/head:null_transmute on your new fork to fetch this PR. That will re-create the null_transmute branch locally with your PR. You may have to rename upstream if your git remote to this repo is called differently.

If that doesn't work for some reason, have a look at this gist, too.

[phansch]

@bors try (let's see if bors cares about the missing branch)

[bors]

⌛ Trying commit 069957a with merge ed09092...

[bors]

☀️ Try build successful - checks-travis, status-appveyor
Build commit: ed09092

[flip1995]

@bors r+

[bors]

📌 Commit 069957a has been approved by flip1995

[bors]

⌛ Testing commit 069957a with merge e91ba8a...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing e91ba8a to master...

[felix91gr]

⛵️