[Mono]: Fix generic interface non-virtual method vt_slot issue in build_imt_slots.

[lateralusX]

In case of a generic interface, build_imt_slots calculate the vt_slot to find the index in the vtable for the implementation. Currently implementation assumed interface methods are either static and/or virtual, but it's possible to have private/sealed methods that won't end up as virtual methods in vtable. This case was handled when building the vtable, but we didn't handle that scenario when building the IMT slots, ending up with wrong vtable slot, potentially outside of allocated memory.

In repro provided by #113958, the created vtable will have 5 slots, 4 from implementing Object and 1 from the implemented interface. When calculating the vt_slot in build_imt_slots we did however include the private DIM methods when calculating vtable slot, ending up with 4 + 3, slot 7, but vtable only allocated 5, so it will read outside allocated memory, and if it this turns out to be NULL, that will trigger assert, otherwise it will use random value, but since IMT slot will be patched with compiled method, the issue wouldn't manifest, at least not in the specific repro scenario.

Fix makes sure we only count virtual methods in build_imt_slots vt_slot index, similar to previous fixes for static and non-virtual methods in non-generic interfaces:

0fb7b7d
6543a04

Fixes #113958.

[Copilot]

Pull Request Overview

This PR fixes an issue in the build_imt_slots function where non-virtual methods were being mistakenly counted when determining the vt_slot for generic interfaces. The change ensures that only virtual methods are considered, thus avoiding a vtable slot out-of-bound error.

• Only virtual methods now contribute to the vt_slot calculation.
• The change prevents accessing vtable slots beyond allocated memory in scenarios with private or sealed methods.

Comments suppressed due to low confidence (1)

src/mono/mono/metadata/object.c:1591

• The added check ensures that only virtual methods are counted to compute vt_slot. Verify that m_method_is_virtual covers all necessary cases to avoid excluding methods that should be considered virtual.

if (m_method_is_virtual (method) && mono_method_get_imt_slot (method) != slot_num) {

[lateralusX]

Failures are known.

[vitek-karas]

Looks good - can we add a test? Or is this already covered by some test and it just happens to work by accident?

[lateralusX]

We have a bunch of DIM tests, haven't looked if we have exactly this test, but there is a risk that we passed it by accident or have intermittent failures if that is the case or turned of on Mono. I can add one if we miss the scenario with couple of private and then a virtual method.

[lateralusX]

@vitek-karas, added new DIM regression tests covering the scenario seen in #113958.

[lambdageek]

it's frustrating that build_mit_slots essentially duplicates the work that we do trying to assign vtable slots to methods in vtable setup. At the very least it feels like we should be able to assert that they agree. I tried to add something in e2f058c but I couldn't finish it. I wonder if the issue was precisely these private methods.

Also, i wonder if there's something we can assert into common_call_trampoline when we do an imt lookup - (for example when we get back a vtable slot from imt lookup, maybe we can range check it to make sure that it is in bounds for the current klass)

[lateralusX]

Agree, there is a lot of scenarios covered in here as well, for this scenario, the value that we read from vtable was not really used in the end, since we compiled the method based on other lookups, so it was mainly an assert that made sure that the vtable slot was not NULL that exposed this, any other value would just "work", unless we would end up reading protected memory.