Add lint againts invalid runtime symbol definitions

[Urgau]

View all comments

This PR adds a deny-by-default lint againts invalid runtime symbol definitions, those runtime symbols are assumed and used by core¹ and rustc with a specific definition.

We have had multiple reports of users tripping over std symbols (addressed in a future PR):

• Why does #[no_mangle] fn open() {} make cargo t hang?
• Pointer becomes misaligned in test with no_mangle

This PR is a second attempt after #146505, where T-lang had some reservations about a blanket lint that does not check the signature, which is now done with this PR, and about linting of std runtime symbols when std is not linked, which this PR omits by not including any std runtime symbols (for now).

invalid_runtime_symbol_definitions

(deny-by-default)

The invalid_runtime_symbol_definitions lint checks the signature of items whose symbol name is a runtime symbols expected by core.

Example

#[unsafe(no_mangle)]
pub fn memcmp() {} // invalid definition of the `memcmp` runtime symbol

error: invalid definition of the runtime `memcmp` symbol used by the standard library
 --> a.rs:2:1
  |
4 | fn memcmp() {}
  | ^^^^^^^^^^^
  |
  = note: expected `unsafe extern "C" fn(*const c_void, *const c_void, usize) -> i32`
          found    `fn()`
  = help: either fix the signature or remove any attributes like `#[unsafe(no_mangle)]`, `#[unsafe(export_name = "memcmp")]`, or `#[link_name = "memcmp"]`
  = note: `#[deny(invalid_runtime_symbol_definitions)]` on by default

Explanation

Up-most care is required when defining runtime symbols assumed and used by the standard library. They must follow the C specification, not use any standard-library facility or undefined behavior may occur.

The symbols currently checked are memcpy, memmove, memset, memcmp, bcmp and strlen.

@rustbot labels +I-lang-nominated +T-lang +needs-fcp +A-lints
cc @rust-lang/lang-ops
r? compiler

Footnotes

1. https://doc.rust-lang.org/core/index.html#how-to-use-the-core-library ↩

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[PatchMixolydic]

Nit:

Suggested change

	/// Up-most care is required when defining runtime symbols assumed and
	/// Utmost care is required when defining runtime symbols assumed and

View changes since the review

[traviscross]

Thanks @Urgau for adjusting based on the feedback and moving this forward.

@rfcbot fcp merge lang

[jackh726]

Should this also include rust_eh_personality?

[steffahn]

As far as I understood the lang meeting discussion, there was no concern with this PR.

But some opinion that it doesn’t fully replace the previous PR, since some warn-by-default lint more generally applicable (and to actually address the user reports, which were about open and read) might also be desirable. All of this can of course be follow-up work so it shouldn’t block us from getting the lint as in this PR; but it’s be useful to clarify that the user reports (currently mentioned in the OP of this PR) aren’t addressed yet with this PR merged, and in this regard this isn’t a full replacement of #146505.

[scottmcm]

My interpretation here: this lint is valuable because the wrong signature for something on the well-known list is clearly and always wrong, so deny makes sense. We should have this lint. (And I'm find expanding to more functions so long as they meet that "definitely incontrovertibly wrong signature for that name that rustc or one of your linked libraries uses" bar. I don't know if we include extern function declarations in rlibs, but if this expanded to a general check that you're not conflicting with other rust libraries that'd also be cool -- not this PR I assume, though.)

Another lint, for "it's very suspicious that you're defining an unmangled open" kinds of things probably also makes sense. That might not be deny though, since it's not as unquestionably wrong. It's a rare case of a rustc lint where I'm plausibly fine with the only mitigation being to allow it, since you could just crate-level-allow it for those very few crates where doing this is intentional because it's trying to implement such things. (Notably if you're trying to define a memcpy you'll need to do extra steps already anyway, IIRC.) But that wouldn't be this PR.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 110eb49 (110eb4964d2acf5f5faee793d0d7783e28daa80a, parent: 6083f7891b5088184968e050d0eaecc7bcfd2b3d)

[nbdd0121]

It's explicitly only blessed for Rust-to-Rust calls. The rules do not apply to e.g. calling libc. Furthermore, we're discussing relaxing this.

Many of the code samples in the search that I quoted https://github.com/search?q=language%3Arust+fn+memcpy&type=code are Rust-to-Rust calls as memcpy is defined in Rust.

I still don't think the u8/i8 case is something the lint should accept. Other than reference to raw pointer of the same mutability, I don't think we should accept any divergence.

Many of these programs also probably don't care about CFI (or only have direct function call emitted for these functions, which will be the case for LLVM-generated calls to those functions), and as this is a deny-by-default lint, I think rejecting these working code is excessive. I wouldn't be concerned if the lint is only warn-by-default.

[joshtriplett]

@Darksonn wrote:

I still don't think the u8/i8 case is something the lint should accept. Other than reference to raw pointer of the same mutability, I don't think we should accept any divergence.

AFAICT, the point of this lint is "you're defining a conflicting symbol that couldn't possibly work. If you're defining something that uses a pointer but it's a different signedness, or for that matter something that accepts an Option<NonNull>, then that is something that could work. I think we should be making a best-effort here, to catch cases where someone might have defined their own function without even realizing it was a conflict; we're not trying to stop people who are reasonably trying to write an interposing symbol.

Could you elaborate on why you feel it's a problem for us to suppress the lint if the only mismatch is *mut i8 vs *mut u8 or similar?

Is CFI an issue here? Would those types be considered incompatible, tripping a CFI fault if called?

[scottmcm]

From my perspective, I would absolutely expect ABI-compatible being fine here.

If we had this check on posix_memalign, for example, I'd want to be able to use mem::Alignment as the parameter type for the alignment. Insisting that it can only be a type alias for usize would make me sad, since it would keep me from encoding preconditions in the types.

[RalfJung]

Is CFI an issue here? Would those types be considered incompatible, tripping a CFI fault if called?

Yes. That's exactly the point. That's why I referenced rust-lang/unsafe-code-guidelines#489 above.

The plan to make Rust CFI-compatible is to declare some calls that we currently deem ABI-compatible to instead have EB (Erroneous Behavior), together with a promise that Rust in its default configuration will not act on this EB. And among the calls that we'll have to make EB is a *mut u8 vs *mut i8 mismatch (or really any *mut T vs *mut U with different pointee types).

[joshtriplett]

@RalfJung I see. That makes sense.

Still not certain if we should be linting such mismatches yet, given that this is a best-effort lint, but given the CFI concern, it seems reasonable to ship this lint without the mismatch handling.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Urgau]

Given the discussion above, the T-lang triage notes and the Zulip thread. I've removed the type normalization/mismatch handling and gone back to exact type equality.

@ojeda @nbdd0121 could I ask you to prepare a patch that allow the lint (#[allow(invalid_runtime_symbol_definitions)]), so I can unblock CI and land the lint.

[ojeda]

Thanks, I can do that.

[tmandry]

If we had this check on posix_memalign, for example, I'd want to be able to use mem::Alignment as the parameter type for the alignment. Insisting that it can only be a type alias for usize would make me sad, since it would keep me from encoding preconditions in the types.

There's something I don't understand here. I don't think you would use mem::Alignment in the exported extern "C" definition of posix_memalign, because you don't necessarily control all callers. Similar for NonNull. What you might want instead is a second, safe(-r) function to expose to Rust code with stronger types, and have one call the other.

---

With that said, I believe this is the first time we're going to lint over this new category of Erroneous Behavior to satisfy CFI, and CFI is rather strict, and deny-by-default is rather strong. If there is a convincing case where this lint would steer people away from using safer types, I'd want to discuss more with the team before this lands.

I'm going to renominate so this stays on our radar, but we may be able to resolve it async if we agree there are no examples of this.

@rustbot label I-lang-nominated

[ojeda]

Please cherry-pick ojeda@422215a (will conflict with #157151).

I am allowing the lint in that commit, but for the actual kernel patch I am considering just blocking the set of functions in bindgen's generation to keep the lint enabled -- so far it is just strlen and we don't call it.

[Urgau]

Thanks. I've cherry-picked the patch.

Let's verify that it works as expected.
@bors try jobs=x86_64-rust-for-linux

[rust-bors]

☀️ Try build successful (CI)
Build commit: d3bea8c (d3bea8c7f040459e99f8e5ef8ac913f53dbb3ba9, parent: ac6f3a3e778a586854bdbf8f15202e11e2348d9f)