CFI: Fix types that implement Fn, FnMut, or FnOnce

[rcvalle]

Fix type transformation for types that implement Fn, FnMut, or FnOnce traits (such as Box<dyn Fn()>, Box<dyn FnMut()>, etc.) by transforming self into a trait object of the trait that defines the method. Fixes #144641.

cc @1c3t3a @Jakob-Koschel

[rustbot]

r? @SparrowLii

rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[compiler-errors]

I'd like to see more explanation for the fix in this PR. Specifically, the issue and the PR don't actually describe what kind of CFI violation was being encountered here before, or how it's being fixed by this erasure?

The comment in #144641 (comment) mentions:

The only special handling in the code is for closures, coroutines, and coroutine closures, but not for arbitrary types that implement these traits.

Does this make the code that is special-cased for closure-like types redundant?

[compiler-errors]

This logic does a ton of unnecessary work.

Just compute if let Some(trait_def_id) = tcx.trait_of_assoc(instance.def_id()), use tcx.as_lang_item() to turn it into a LangItem, and match on Fn/FnMut/FnOnce.

[compiler-errors]

What kind of InstanceKind are we expecting here? This seems like it would perhaps overlap or interact with the logic above what you added, which seems problematic.

[compiler-errors]

@maurer wrote a lot of this erasure logic, I'd be interested if he has a take on this approach as well

[maurer]

My first reaction here is that I think this is a bug in implemented_method rather than something that should be fixed via an additional clause in transform_instance - implemented_method should detect the call_mut implementation as an implemented method, and so the first non-closure clause should be triggering. I'll take a more in depth look in an hour or two when I get in to work.

[maurer]

Specifically, d5bf25c seems to address this more simply - basically, it looks like I missed FnPtrShim(def_id, _). I'm still taking a look over the other instance kinds to make sure that not casing out on only Item and FnPtrShim won't accidentally apply somewhere it shouldn't, but this seems to fix it a bit more cleanly - the problem was an unaccounted-for instance kind, not an unaccounted-for trait.

Update: It looks like the adjustment I'm proposing would also cause:

• Virtual and VTableShim to get double-processed, because we've already dealt with them. The generalization is mostly a no-op, except that the InstanceKind for VTableShim will become Virtual, which is perhaps misleading but should have no effect and Virtual instances will get their method index zeroed out.
• ReifyShim would get erased into Virtual - this would be a problem for KCFI, except this code is guarded by USE_CONCRETE_SELF, which gets forcibly enabled for ReifyReason::FnPtr.
• ClosureOnceShim and ConstructCoroutineInClosureShim should trip this, and occur before the check if something is closure-like. I wasn't able to easily detect any differences in behavior here, but I didn't do deep tracing. I think handling these via the closure-like code vs via the default impl code normalizes them the same way. We still need the closure normalizing code because true closures can't be normalized this way.
• FutureDropPollShim, CloneShim, FnPtrAddrShim, and AsyncDropGlueCtorShim would all match initially, but then be discarded because the traits they implement are not object safe.

This looks OK to me, and has the added benefit of automatically picking up new shim types when added.

If we wanted to be more conservative, we could instead do 0026b9c which fixes the tests in question with a minimal change - it only re-normalizes FnPtrShim, and would require any future shims we want normalized to be explicitly added to the default_or_shim checker.

[rcvalle]

@maurer Thank you for your time looking at it! Much appreciated. I'm wondering whether we should change approach in this code and cover each case explicitly (like we do for encoding) so we could express our intent clearly and know when something is handled (or not) and the side effects of it (also clearly).

[rcvalle]

@maurer If you put a commit message on 0026b9c, I could cherry pick it into this PR, or if you have time to, feel free to send a PR for it. Thanks a lot! Much appreciated.

[maurer]

I added a commit message at https://github.com/maurer/rust/tree/cfi-fix - I hadn't bothered because I just expected you to squash it onto your commit if you agreed with the direction.

Re: being more explicit with casing out, I think maybe the way to do that (which should definitely be a future PR if at all) would be:

1. Split out InstanceKinds into
   a. Always vtable (or Virtual) (these should be unconditionally rewritten)
   b. Sometimes vtable (these should be rewritten only when the concrete flag isn't passed)
   c. Passthrough
2. Check to see if it would be possible to simplify the flow of our existing code based on that - right now, everything in the "always vtable" section still possibly falls through and gets processed by the "sometimes vtable" section when concrete isn't set, but I don't think that's load bearing, and probably just confuses things
3. Create an explicit enum that looks something like

enum RewriteKind {
  // Always rewrite
  Drop,
  Virtual(DefId),
  VTableShim(DefId),
  // Sometimes rewrite
  Default(DefId),
  Impl(DefId),
  Shim(DefId),
  Closure(InstanceKind),
  // Never rewrite
  Passthrough(InstanceKind),
}

4. We do the exhaustive careful conversion from InstanceKind to RewriteKind, and then have a separate method that takes the RewriteKind and actually does the (possible) rewrite.

I don't know how much maintenance this saves us, mostly depends on how often we're sprinkling new InstanceKinds in or whether we've missed any other InstanceKinds