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

[Yi Yang]

On Tue, 19 Apr 2022 12:14:08 GMT, David Holmes <dholmes 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.
>
> Hi,
> 
> This really needs a test case so we can understand exactly what is going on. Receiver type checking happens in a number of places so we need to understand how the problem arises and exactly what should be happening in that case per the JVMS. If this is a corrupt serialization stream then it may indicate a bug in the JDK deserialization code - again important to know.
> 
> Thanks,
> David

Hi @dholmes-ora, I believe this is not a problem of JDK serialization because it uses a third-party library, this library deserializes an ill-typed object via Unsafe, which leads to some strange crashes, so this patch is not a Fix,  but an Enhancement. This problem happens in production and I'm trying to reproduce it locally, but it's still not working, I'll keep trying.

We preserved many debug information, it should be enough to tell what was going on at that time. The crash stacks are as follows:


#1
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x48ddc5]  CompilationPolicy::can_be_compiled(methodHandle, int)+0x155
V  [libjvm.so+0x490c81]  CompilationPolicy::must_be_compiled(methodHandle, int)+0x91
V  [libjvm.so+0x7e1d50]  CallInfo::set_common(KlassHandle, KlassHandle, methodHandle, methodHandle, CallInfo::CallKind, int, Thread*)+0xa0
V  [libjvm.so+0x7e2082]  CallInfo::set_virtual(KlassHandle, KlassHandle, methodHandle, methodHandle, int, Thread*)+0xf2
V  [libjvm.so+0x7e2627]  LinkResolver::runtime_resolve_virtual_method(CallInfo&, methodHandle, KlassHandle, Handle, KlassHandle, bool, Thread*)+0x1e7
V  [libjvm.so+0x7e2cf2]  LinkResolver::resolve_virtual_call(CallInfo&, Handle, KlassHandle, KlassHandle, Symbol*, Symbol*, KlassHandle, bool, bool, Thread*)+0x152
V  [libjvm.so+0x7e2f96]  LinkResolver::resolve_invokevirtual(CallInfo&, Handle, constantPoolHandle, int, Thread*)+0x276
V  [libjvm.so+0x7e760b]  LinkResolver::resolve_invoke(CallInfo&, Handle, constantPoolHandle, int, Bytecodes::Code, Thread*)+0x20b
V  [libjvm.so+0x69777d]  InterpreterRuntime::resolve_invoke(JavaThread*, Bytecodes::Code)+0x1bd
j  com.taobao.forest.domain.dataobject.std.impl.DefaultStdCategoryPropertyDO.getValuesMap()Ljava/util/Map;+4
v  ~StubRoutines::call_stub
...
#2 
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x7e25c0]  LinkResolver::runtime_resolve_virtual_method(CallInfo&, methodHandle, KlassHandle, Handle, KlassHandle, bool, Thread*)+0x180
V  [libjvm.so+0x7e2cf2]  LinkResolver::resolve_virtual_call(CallInfo&, Handle, KlassHandle, KlassHandle, Symbol*, Symbol*, KlassHandle, bool, bool, Thread*)+0x152
V  [libjvm.so+0x7e2f96]  LinkResolver::resolve_invokevirtual(CallInfo&, Handle, constantPoolHandle, int, Thread*)+0x276
V  [libjvm.so+0x7e760b]  LinkResolver::resolve_invoke(CallInfo&, Handle, constantPoolHandle, int, Bytecodes::Code, Thread*)+0x20b
V  [libjvm.so+0x69777d]  InterpreterRuntime::resolve_invoke(JavaThread*, Bytecodes::Code)+0x1bd
j  com.taobao.forest.domain.dataobject.std.impl.DefaultStdCategoryPropertyDO.getValuesMap()Ljava/util/Map;+4
v  ~StubRoutines::call_stub


Some method resolution logs are as folows:

# linktime_resolve_virtual_method
invokevirtual resolved method: caller-class:com.taobao.forest.domain.dataobject.std.impl.DefaultStdCategoryPropertyDO, compile-time-class:com.taobao.forest.domain.util.LongMapSupportArrayList, method:com.taobao.forest.domain.util.LongMapSupportArrayList.toMap()Ljava/util/Map;, method_holder:com.taobao.forest.domain.util.MapSupportArrayList, access_flags: public 

# runtime_resolve_virtual_method
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#


I want to clarify it again. During linktime_resolve_virtual_method, JVM finds that the method to be called is LongMapSupportArrayList.toMap, and the method holder class is `DefaultStdCategoryPropertyDO`. It looks like:


public class DefaultStdCategoryPropertyDO extends DefaultFeatureSupportDO implements StdCategoryPropertyDO {
    ...
    private LongMapSupportArrayList<StdCategoryPropertyValueDO> longValues = new LongMapSupportArrayList("valueId");
    ...
    public Map<Long, ? extends StdCategoryPropertyValueDO> getValuesMap() {
        return this.longValues.toMap();
    }
}

Then at runtime_resolve_virtual_method, the real type of the receiver is ArrayList. Since there is no receiver type check, the JVM tries to find toMap from ArrayList's vtables (through the virtual table index), which may eventually find an invalid method (selected_method: 0x458), or incorrect method (crash at CallInfo::set_virtual)

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

PR: https://git.openjdk.java.net/jdk/pull/8241