RFR: 8284877: Check type compatibility before looking up method from receiver's vtable [v2]

[Daniel D. Daugherty]

On Thu, 21 Apr 2022 06:04:27 GMT, Yi Yang <yyang at openjdk.org> wrote:

>> Hi, can I have a review for this enhancement? This patch adds type compatibility check before method lookup for robustness. In some internal cases, serialization framework may improperly generate an object of wrong type, which leads JVM randomly crashes during method resolution.
>> 
>> For example:
>> 
>> invokevirtual selected method: receiver-class:java.util.ArrayList, resolved-class:com.taobao.forest.domain.util.LongMapSupportArrayList, resolved_method:com.taobao.forest.domain.util.LongMapSupportArrayList.toMap()Ljava/util/Map;, selected_method:0x458, vtable_index:56#
>> 
>> The real type of receiver is ArrayList, while the resolved method is LongMapSupportArrayList.toMap. VM attempts to select method as if looking up from receiver's vtable via vtable index of resolved method(i.e. attempts to lookup `toMap()` from 
>>  ArrayList), an invalid method or incorrect method would be selected, thus causing some strange crashes.
>> 
>> I think it's reasonable to add a type compatibility check before method lookup. If such an incompatible call is found, JVM could throw an exception instead.
>
> Yi Yang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   add jtreg test

Changes requested by dcubed (Reviewer).

src/hotspot/share/interpreter/linkResolver.cpp line 1360:

> 1358:     stringStream ss;
> 1359:     ss.print("Resolved method holder class %s", resolved_method->method_holder()->external_name());
> 1360:     ss.print(" must be linked");

Nit: missing period at the end of the sentence.

src/hotspot/share/interpreter/linkResolver.cpp line 1364:

> 1362:   }
> 1363: 
> 1364:   // Receriver should be compatible with resolved klass, i.e. it must be a type of resolved klass

Typo: s/Receriver/Receiver/

src/hotspot/share/interpreter/linkResolver.cpp line 1365:

> 1363: 
> 1364:   // Receriver should be compatible with resolved klass, i.e. it must be a type of resolved klass
> 1365:   // or a subtype of resolved klass(receiver instanceof resolved_klass)

Nit: missing period at the end of the sentence.

src/hotspot/share/interpreter/linkResolver.cpp line 1370:

> 1368:     stringStream ss;
> 1369:     ss.print("The type of receiver %s", recv_klass->external_name());
> 1370:     ss.print(" should be or subtype of %s", resolved_klass->external_name());

Nit: missing period at the end of the sentence.

test/hotspot/jtreg/runtime/linkResolver/TestErrorReceiverType.java line 2:

> 1: /*
> 2:  * Copyright (c) 2022, Alibaba Group Holding Limited. All Rights Reserved.

Update copyright year to 2023.

test/hotspot/jtreg/runtime/linkResolver/TestErrorReceiverType.java line 31:

> 29:  * @modules java.base/jdk.internal.misc:+open
> 30:  * @library /test/lib
> 31:  * @run main/othervm TestErrorReceiverType

Will likely need `-Xcheck:jni` if this fix goes that route.

-------------

PR Review: https://git.openjdk.org/jdk/pull/8241#pullrequestreview-1481959458
PR Review Comment: https://git.openjdk.org/jdk/pull/8241#discussion_r1231243625
PR Review Comment: https://git.openjdk.org/jdk/pull/8241#discussion_r1231241229
PR Review Comment: https://git.openjdk.org/jdk/pull/8241#discussion_r1231242775
PR Review Comment: https://git.openjdk.org/jdk/pull/8241#discussion_r1231244033
PR Review Comment: https://git.openjdk.org/jdk/pull/8241#discussion_r1231245215
PR Review Comment: https://git.openjdk.org/jdk/pull/8241#discussion_r1231246297