Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[strict provenance] add lints for evil pointer casts #95488

Closed
Gankra opened this issue Mar 30, 2022 · 12 comments
Closed

[strict provenance] add lints for evil pointer casts #95488

Gankra opened this issue Mar 30, 2022 · 12 comments
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-strict-provenance Area: Strict provenance for raw pointers C-future-incompatibility Category: Future-incompatibility lints

Comments

@Gankra
Copy link
Contributor

Gankra commented Mar 30, 2022

This issue is part of the Strict Provenance Experiment - #95228

We should make it easier for people to detect places where they are using casts instead of the "blessed" strict_provenance APIs.

@eddyb and I prototyped this out here: 93f7f06

The patch needs some cleanups, though. Quoting from elsewhere:


All lints should be made allow by default, meaning they're opt-in.

At least in the bootstrap, the compiler will complain if you allow() a lint in your code that doesn't exist. This potentially just means:

  • We need to keep the experimental lint around forever even when the experiment is over
  • Users can only "safely" invoke it from the command line manually, which is slightly unfortunate for anything like what I did where I used it as a FIXME/WONTFIX marker for the file.

Also due to the "Opaque Function Pointers" / "Harvard Architecture" / "AVR is cursed" issue

// HACK: The intermediate cast as usize is required for AVR
// so that the address space of the source function pointer
// is preserved in the final function pointer.
//
// https://github.com/avr-rust/rust/issues/143
fmt::Pointer::fmt(&(*self as usize as *const ()), f)

I think we want the lint broken up into parts:

  • #[fuzzy_provenance_casts] - int-to-ptr, totally evil
  • #[lossy_provencance_casts] - ptr-to-int, sketchy but valid as long as you actually want .addr() semantics
  • #[oxford_casts] - casts that make harvard architectures sad -- fn<->ptr (name is a joke... unless...)

I can't justify discouraging fn <-> int, absent better ways to talk about fn ptrs properly.

@Gankra Gankra added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-future-incompatibility Category: Future-incompatibility lints A-strict-provenance Area: Strict provenance for raw pointers labels Mar 30, 2022
@jrtc27
Copy link

jrtc27 commented Mar 30, 2022

  • #[oxford_casts] - casts that make harvard architectures sad -- fn<->ptr (name is a joke... unless...)

Von Neumann would be the appropriate name if you wanted to highlight that aspect, though some Harvard architectures can handle them just fine too, depends on whether the data pointer representation is sufficient to also safely round-trip function pointers (though it may refer to something else when interpreted as a data pointer).

@beepster4096
Copy link
Contributor

Can't we just make this lint unstable like unsafe_block_in_unsafe_fn used to be?

@Gankra
Copy link
Contributor Author

Gankra commented Mar 30, 2022

Oh yes if there's properly lint stability stuff that would be Good To Use (unless that introduces "have to use nightly" and that is deemed unacceptable).

@Gankra
Copy link
Contributor Author

Gankra commented Mar 30, 2022

Oh also I have no idea what the deal is with "safe transmute" stuff, but if these lints can at all look "into" transmutes and try to catch more Secret Casts that would be Friggin' Rad.

@eternaleye
Copy link
Contributor

eternaleye commented Mar 30, 2022

  • #[oxford_casts] - casts that make harvard architectures sad -- fn<->ptr (name is a joke... unless...)

Von Neumann would be the appropriate name if you wanted to highlight that aspect, though some Harvard architectures can handle them just fine too, depends on whether the data pointer representation is sufficient to also safely round-trip function pointers (though it may refer to something else when interpreted as a data pointer).

I proposed this in side channels, with the (joking) explanation that "if you start with just a few, they'll proliferate" :P

@jswrenn
Copy link
Member

jswrenn commented Mar 31, 2022

Oh also I have no idea what the deal is with "safe transmute" stuff, but if these lints can at all look "into" transmutes and try to catch more Secret Casts that would be Friggin' Rad.

@Gankra Should be possible! The in-development safe(r) transmute API, which will be a distinct API from mem::transmute, is just going to flatly forbid pointer-to-integer and integer-to-pointer transmutations. It should be possible to use that same machinery to lint existing occurrences of mem::transmute.

@Lokathor
Copy link
Contributor

Bytemuck also removed the Pod impl for pointers a few versions back <3

@scottmcm
Copy link
Member

scottmcm commented Apr 2, 2022

Note that lang and libs both want a lint for ptr-to-int via as (https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Adding.20methods.20as.20more.20specific.20versions.20of.20.60as.60/near/238391074) so if anyone wants to pick that up it should hopefully be uncontroversial.

@niluxv
Copy link
Contributor

niluxv commented Apr 2, 2022

I would like to give it a try.
@rustbot claim

@Manishearth
Copy link
Member

@Gankra regarding "lints that don't exist", there's a lint deprecation mechanism that allows you to register names as deprecated (and delete the impl), it's used often in clippy and occasionally in rustc

@niluxv
Copy link
Contributor

niluxv commented Apr 8, 2022

#95599 contains lints for int-to-ptr and ptr-to-int as casts, and will likely be merged soon. @rustbot release-assignment

The exposing API methods introduced by #95588 might be linted against using clippy's disallowed_methods lint (although it doesn't seem possible yet to specify raw pointer methods).

Regarding transmutes it should be pretty trivial to lint against ptr-to-int and int-to-ptr transmutes, but not when the integers/pointers are hidden inside other structs. That will probably require the full safer transmute stuff.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 8, 2022
…chaelwoerister

Strict provenance lints

See rust-lang#95488.
This PR introduces two unstable (allow by default) lints to which lint on int2ptr and ptr2int casts, as the former is not possible in the strict provenance model and the latter can be written nicer using the `.addr()` API.
Based on an initial version of the lint by `@Gankra` in rust-lang#95199.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 9, 2022
…chaelwoerister

Strict provenance lints

See rust-lang#95488.
This PR introduces two unstable (allow by default) lints to which lint on int2ptr and ptr2int casts, as the former is not possible in the strict provenance model and the latter can be written nicer using the `.addr()` API.
Based on an initial version of the lint by ``@Gankra`` in rust-lang#95199.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 9, 2022
…chaelwoerister

Strict provenance lints

See rust-lang#95488.
This PR introduces two unstable (allow by default) lints to which lint on int2ptr and ptr2int casts, as the former is not possible in the strict provenance model and the latter can be written nicer using the `.addr()` API.
Based on an initial version of the lint by ```@Gankra``` in rust-lang#95199.
@RalfJung
Copy link
Member

The lints have their own dedicated feature and tracking issue now, so closing in favor of that: #130351.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-strict-provenance Area: Strict provenance for raw pointers C-future-incompatibility Category: Future-incompatibility lints
Projects
Archived in project
Development

No branches or pull requests

10 participants