# HG changeset patch # User Doug Simon # Date 1490999313 -7200 # Node ID 8cf4cf3f9f2a7021072b7cc34fdb9d06b11be41f # Parent c0b9eb2b67153f79d193e3ecfe0710ff724caf22 missing checks in HotSpotMemoryAccessProviderImpl can cause VM assertions to fail - part 3 (JDK-8177673) diff -r c0b9eb2b6715 -r 8cf4cf3f9f2a jvmci/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotConstantReflectionProvider.java --- a/jvmci/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotConstantReflectionProvider.java Thu Mar 30 23:54:54 2017 +0200 +++ b/jvmci/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotConstantReflectionProvider.java Sat Apr 01 00:28:33 2017 +0200 @@ -197,11 +197,14 @@ if (hotspotField.isStatic()) { HotSpotResolvedJavaType holder = (HotSpotResolvedJavaType) hotspotField.getDeclaringClass(); if (holder.isInitialized()) { - return memoryAccess.readUnsafeConstant(hotspotField.getJavaKind(), HotSpotObjectConstantImpl.forObject(holder.mirror()), hotspotField.offset()); + return memoryAccess.readFieldValue(hotspotField, holder.mirror()); } } else { - if (receiver.isNonNull() && hotspotField.isInObject(((HotSpotObjectConstantImpl) receiver).object())) { - return memoryAccess.readUnsafeConstant(hotspotField.getJavaKind(), receiver, hotspotField.offset()); + if (receiver.isNonNull()) { + Object object = ((HotSpotObjectConstantImpl) receiver).object(); + if (hotspotField.isInObject(object)) { + return memoryAccess.readFieldValue(hotspotField, object); + } } } return null; diff -r c0b9eb2b6715 -r 8cf4cf3f9f2a jvmci/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotMemoryAccessProviderImpl.java --- a/jvmci/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotMemoryAccessProviderImpl.java Thu Mar 30 23:54:54 2017 +0200 +++ b/jvmci/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotMemoryAccessProviderImpl.java Sat Apr 01 00:28:33 2017 +0200 @@ -55,46 +55,68 @@ * @param base constant value containing the base address for a pending read * @return {@code null} if {@code base} does not box an object otherwise the object boxed in * {@code base} - * @throws IllegalArgumentException if {@code base} boxes an object and the read address does - * not correspond to a field or element within the object */ - private static Object asObject(MetaAccessProvider metaAccess, Constant base, JavaKind kind, long displacement) { + private static Object asObject(HotSpotJVMCIRuntimeProvider runtime, Constant base, JavaKind kind, long displacement) { if (base instanceof HotSpotObjectConstantImpl) { HotSpotObjectConstantImpl constant = (HotSpotObjectConstantImpl) base; - HotSpotResolvedJavaType type = (HotSpotResolvedJavaType) constant.getType(); + HotSpotResolvedObjectType type = constant.getType(); Object object = constant.object(); - if (type.isArray()) { - ResolvedJavaType componentType = type.getComponentType(); - JavaKind componentKind = componentType.getJavaKind(); - final int headerSize = getArrayBaseOffset(componentKind); - int sizeOfElement = getArrayIndexScale(componentKind); - int length = Array.getLength(object); - int index = (int) ((displacement - headerSize) / sizeOfElement); - if (displacement < headerSize || index >= length || ((displacement - headerSize) % sizeOfElement) != 0) { - throw new IllegalArgumentException("Unsafe array access: reading element of kind " + kind + - " at offset " + displacement + " (index ~ " + index + ") in " + - type.toJavaName() + " object of length " + length); - } + if (object instanceof Class) { + // Cannot check bounds when reading from a java.lang.Class as + // we don't have the bounds for the variable length part of + // the object tail containing the static fields of the + // represented class. } else { - ResolvedJavaField field = type.findInstanceFieldWithOffset(displacement, kind); - if (field == null && object instanceof Class) { - HotSpotResolvedObjectTypeImpl staticFieldsHolder = (HotSpotResolvedObjectTypeImpl) metaAccess.lookupJavaType((Class) object); - field = staticFieldsHolder.findStaticFieldWithOffset(displacement, kind); - } - if (field == null) { - throw new IllegalArgumentException("Unsafe object access: field not found for read of kind " + kind + - " at offset " + displacement + " in " + - type.toJavaName() + " object"); - } - if (field.getJavaKind() != kind) { - throw new IllegalArgumentException("Unsafe object access: field " + field.format("%H.%n:%T") + " not of expected kind " + kind + - " at offset " + displacement + " in " + - type.toJavaName() + " object"); - } + checkRead(runtime, kind, displacement, type, object); } return object; } return null; + + } + + private static void checkRead(HotSpotJVMCIRuntimeProvider runtime, JavaKind kind, long displacement, HotSpotResolvedObjectType type, Object object) { + if (type.isArray()) { + ResolvedJavaType componentType = type.getComponentType(); + JavaKind componentKind = componentType.getJavaKind(); + final int headerSize = getArrayBaseOffset(componentKind); + int sizeOfElement = getArrayIndexScale(componentKind); + int length = Array.getLength(object); + long arrayEnd = headerSize + (sizeOfElement * length); + boolean aligned = ((displacement - headerSize) % sizeOfElement) == 0; + if (displacement >= (arrayEnd - sizeOfElement) || (kind == JavaKind.Object && !aligned)) { + int index = (int) ((displacement - headerSize) / sizeOfElement); + throw new AssertionError("Unsafe array access: reading element of kind " + kind + + " at offset " + displacement + " (index ~ " + index + ") in " + + type.toJavaName() + " object of length " + length); + } + } else if (kind != JavaKind.Object) { + // In non-product builds, unsafe.cpp asserts that an access based on a non-null + // object is within bounds so do a similar check here to prevent a VM halt. + long size = Math.abs(type.instanceSize()); + int bytesToRead = kind.getByteCount(); + if (displacement + bytesToRead > size || displacement < 0) { + throw new IllegalArgumentException("Unsafe access: reading " + bytesToRead + " bytes at offset " + displacement + " in " + + type.toJavaName() + " object of size " + size); + } + } else { + ResolvedJavaField field = type.findInstanceFieldWithOffset(displacement, JavaKind.Object); + if (field == null && object instanceof Class) { + // Read of a static field + MetaAccessProvider metaAccess = runtime.getHostJVMCIBackend().getMetaAccess(); + HotSpotResolvedObjectTypeImpl staticFieldsHolder = (HotSpotResolvedObjectTypeImpl) metaAccess.lookupJavaType((Class) object); + field = staticFieldsHolder.findStaticFieldWithOffset(displacement, JavaKind.Object); + } + if (field == null) { + throw new IllegalArgumentException("Unsafe object access: field not found for read of kind Object" + + " at offset " + displacement + " in " + type.toJavaName() + " object"); + } + if (field.getJavaKind() != JavaKind.Object) { + throw new IllegalArgumentException("Unsafe object access: field " + field.format("%H.%n:%T") + " not of expected kind Object" + + " at offset " + displacement + " in " + + type.toJavaName() + " object"); + } + } } private boolean isValidObjectFieldDisplacement(Constant base, long displacement) { @@ -104,9 +126,6 @@ if (displacement == runtime.getConfig().classMirrorOffset) { // Klass::_java_mirror is valid for all Klass* values return true; - } else if (displacement == runtime.getConfig().arrayKlassComponentMirrorOffset) { - // ArrayKlass::_component_mirror is only valid for all ArrayKlass* values - return ((HotSpotResolvedObjectTypeImpl) metaspaceObject).mirror().isArray(); } } else { throw new IllegalArgumentException(String.valueOf(metaspaceObject)); @@ -128,8 +147,8 @@ throw new IllegalArgumentException(String.valueOf(base)); } - private static long readRawValue(MetaAccessProvider metaAccess, Constant baseConstant, long displacement, JavaKind kind, int bits) { - Object base = asObject(metaAccess, baseConstant, kind, displacement); + private static long readRawValue(HotSpotJVMCIRuntimeProvider runtime, Constant baseConstant, long displacement, JavaKind kind, int bits) { + Object base = asObject(runtime, baseConstant, kind, displacement); if (base != null) { switch (bits) { case Byte.SIZE: @@ -166,8 +185,6 @@ if (metaspaceObject instanceof HotSpotResolvedObjectTypeImpl) { if (displacement == runtime.getConfig().classMirrorOffset) { assert expected == ((HotSpotResolvedObjectTypeImpl) metaspaceObject).mirror(); - } else if (displacement == runtime.getConfig().arrayKlassComponentMirrorOffset) { - assert expected == ((HotSpotResolvedObjectTypeImpl) metaspaceObject).mirror().getComponentType(); } } } @@ -177,7 +194,7 @@ private Object readRawObject(Constant baseConstant, long initialDisplacement, boolean compressed) { long displacement = initialDisplacement; Object ret; - Object base = asObject(runtime.getHostJVMCIBackend().getMetaAccess(), baseConstant, JavaKind.Object, displacement); + Object base = asObject(runtime, baseConstant, JavaKind.Object, displacement); if (base == null) { assert !compressed; displacement += asRawPointer(baseConstant); @@ -190,36 +207,42 @@ return ret; } - /** - * Reads a value of this kind using a base address and a displacement. No bounds checking or - * type checking is performed. Returns {@code null} if the value is not available at this point. - * - * @param baseConstant the base address from which the value is read. - * @param displacement the displacement within the object in bytes - * @return the read value encapsulated in a {@link JavaConstant} object, or {@code null} if the - * value cannot be read. - * @throws IllegalArgumentException if {@code kind} is {@code null}, {@link JavaKind#Void}, not - * {@link JavaKind#Object} or not {@linkplain JavaKind#isPrimitive() primitive} kind - * or if {@code baseConstant} is a boxed object and the read address does not denote - * a field or element within the object - */ - JavaConstant readUnsafeConstant(JavaKind kind, JavaConstant baseConstant, long displacement) { - if (kind == null) { - throw new IllegalArgumentException("null JavaKind"); - } - if (kind == JavaKind.Object) { - Object o = readRawObject(baseConstant, displacement, runtime.getConfig().useCompressedOops); + JavaConstant readFieldValue(HotSpotResolvedJavaField field, Object obj) { + assert obj != null; + assert !field.isStatic() || obj instanceof Class; + long displacement = field.offset(); + if (field.getJavaKind() == JavaKind.Object) { + Object o = UNSAFE.getObject(obj, displacement); return HotSpotObjectConstantImpl.forObject(o); } else { - int bits = kind.getByteCount() * Byte.SIZE; - return readPrimitiveConstant(kind, baseConstant, displacement, bits); + JavaKind kind = field.getJavaKind(); + switch (kind) { + case Boolean: + return JavaConstant.forBoolean(UNSAFE.getBoolean(obj, displacement)); + case Byte: + return JavaConstant.forByte(UNSAFE.getByte(obj, displacement)); + case Char: + return JavaConstant.forChar(UNSAFE.getChar(obj, displacement)); + case Short: + return JavaConstant.forShort(UNSAFE.getShort(obj, displacement)); + case Int: + return JavaConstant.forInt(UNSAFE.getInt(obj, displacement)); + case Long: + return JavaConstant.forLong(UNSAFE.getLong(obj, displacement)); + case Float: + return JavaConstant.forFloat(UNSAFE.getFloat(obj, displacement)); + case Double: + return JavaConstant.forDouble(UNSAFE.getDouble(obj, displacement)); + default: + throw new IllegalArgumentException("Unsupported kind: " + kind); + } } } @Override public JavaConstant readPrimitiveConstant(JavaKind kind, Constant baseConstant, long initialDisplacement, int bits) { try { - long rawValue = readRawValue(runtime.getHostJVMCIBackend().getMetaAccess(), baseConstant, initialDisplacement, kind, bits); + long rawValue = readRawValue(runtime, baseConstant, initialDisplacement, kind, bits); switch (kind) { case Boolean: return JavaConstant.forBoolean(rawValue != 0); @@ -240,7 +263,7 @@ default: throw new IllegalArgumentException("Unsupported kind: " + kind); } - } catch (IllegalArgumentException | NullPointerException e) { + } catch (NullPointerException e) { return null; } } diff -r c0b9eb2b6715 -r 8cf4cf3f9f2a jvmci/jdk.vm.ci.meta/src/jdk/vm/ci/meta/MemoryAccessProvider.java --- a/jvmci/jdk.vm.ci.meta/src/jdk/vm/ci/meta/MemoryAccessProvider.java Thu Mar 30 23:54:54 2017 +0200 +++ b/jvmci/jdk.vm.ci.meta/src/jdk/vm/ci/meta/MemoryAccessProvider.java Sat Apr 01 00:28:33 2017 +0200 @@ -35,11 +35,9 @@ * @param displacement the displacement within the object in bytes * @param bits the number of bits to read from memory * @return the read value encapsulated in a {@link JavaConstant} object of {@link JavaKind} kind - * or {@code null} if {@code base} is a boxed object and the read address does not - * denote a location holding an {@code kind} value - * @throws IllegalArgumentException if {@code kind} is {@link JavaKind#Void} or not - * {@linkplain JavaKind#isPrimitive() primitive} kind or {@code bits} is not 8, 16, - * 32 or 64 + * @throws IllegalArgumentException if the read is out of bounds of the object or {@code kind} + * is {@link JavaKind#Void} or not {@linkplain JavaKind#isPrimitive() primitive} + * kind or {@code bits} is not 8, 16, 32 or 64 */ JavaConstant readPrimitiveConstant(JavaKind kind, Constant base, long displacement, int bits) throws IllegalArgumentException; @@ -48,9 +46,9 @@ * * @param base the base address from which the value is read * @param displacement the displacement within the object in bytes - * @return the read value encapsulated in a {@link Constant} object or {@code null} if the - * address computed from {@code base} and {@code displacement} does not denote a - * location holding an {@code Object} value + * @return the read value encapsulated in a {@link Constant} object + * @throws IllegalArgumentException if the address computed from {@code base} and + * {@code displacement} does not denote a location holding an {@code Object} value */ JavaConstant readObjectConstant(Constant base, long displacement); }