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

Tracking Issue for strict_provenance_lints #130351

Open
RalfJung opened this issue Sep 14, 2024 · 8 comments
Open

Tracking Issue for strict_provenance_lints #130351

RalfJung opened this issue Sep 14, 2024 · 8 comments
Labels
A-strict-provenance Area: Strict provenance for raw pointers C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Sep 14, 2024

This tracks the two lints associated with the strict provenance feature:

  • fuzzy_provenance_casts: detects an as cast from an integer to a pointer. It is better to use with_exposed_provenance instead to make explicit what happens.
  • lossy_provenance_casts: detects an as cast from a pointer to an integer. It is better to use expose_provenance instead to make explicit what happens.

I am not sure if having two lints here is really justified, IMO they could be merged into one -- not sure what that one should be called, though. Other than that, this seems like a useful lint to ensure the codebase follows strict provenance (or opts-out explicitly, via the methods mentioned above).

I am also not sure if this shouldn't maybe be a clippy lint instead of a rustc lint?

Cc @rust-lang/opsem

@RalfJung RalfJung added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. A-strict-provenance Area: Strict provenance for raw pointers labels Sep 14, 2024
@RalfJung
Copy link
Member Author

@scottmcm wrote

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.

However, that discussion was about ptr-to-u8 casts (and in general, non-usize). I am not sure what the libs and lang stance is on linting against the usize version of these casts.

Nominating for t-lang to get a "temperature".

@RalfJung RalfJung added the I-lang-nominated Nominated for discussion during a lang team meeting. label Sep 14, 2024
@jwnrt
Copy link

jwnrt commented Sep 15, 2024

I am also not sure if this shouldn't maybe be a clippy lint instead of a rustc lint?

Would hosting this in rustc allow for making the breaking change to the usize definition in the future by making this lint forbid? I realise that usize proposal is only a pre-RFC at the moment.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 15, 2024

Code that wants to be portable for CHERI likely has to set these lints to forbid (or at least the int-to-ptr one -- but without int-to-ptr casts, there's no point in having any ptr-to-int casts). That doesn't mean the lint has to be in rustc, though.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Sep 18, 2024

We discussed this in (excessive, as is our wont) length at the @rust-lang/lang meeting. The conclusions were:

  1. We think 1 lint suffices -- 2 lints makes sense if there's a compelling reason to allow one and deny the other, which doesn't seem true here.
  2. We are aligned directionally on enabling this lint and migrating people to more explicit provenance APIs. This is a logical next step in the "journey to provenance" that we approved in RFC 3559.
  3. We are cognizant that this will have broad impact, and so we don't just want to turn it to "warn". We think it should have a cargo fix, a stable alternative, and we should consider other avenues to mitigate impact. We'd like to review that plan/PR before we "go live".

@saethlin
Copy link
Member

We think it should have a cargo fix

Did anyone explain why? Suggesting people automatically migrate to the strict provenance conversions will type-check but probably add UB that is highly non-theoretical since #121282. Suggesting expose/from_exposed will work, but I generally recognize those names as indicating that the author thought about their code and decided they need expose semantics.

@RalfJung
Copy link
Member Author

but I generally recognize those names as indicating that the author thought about their code and decided they need expose semantics.

I'm afraid that signal will anyway diminish fairly quickly, as we have seen e.g. with assume_init -- initially it was a signal, but then people just blindly replaced mem::uninitialized() with MaybeUninit::uninit().assume_init().

@saethlin
Copy link
Member

This issue is tracking the factors that will cause the signal to diminish. If we care about the signal, we should be mindful to preserve it instead of creating more problems like was done with MaybeUninit. uninit().assume_init() should have been banned from the start via lint, but instead the documentation suggested using it for arrays.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 18, 2024

The documentation suggested it for arrays of MaybeUninit<_>. (IIRC we tried having a macro for this but t-libs-api wasn't convinced of that approach.)

But anyway, that was just an example, we don't have to litigate its details. The point is, I am not convinced we are fully in control of how that signal will diminish. Also, not having any as casts any more is an intended end state (IMO), so at some point there will definitely be no signal any more -- at best, we can get "one pass" over the current ecosystem, but it's not a long-term thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-strict-provenance Area: Strict provenance for raw pointers C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants