From 91cdcc6752ca20e6ac84d3c1977a853cecec4eee Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Mon, 26 Mar 2018 19:09:03 +0100 Subject: [PATCH 1/3] CORDA-1258 - Only register custom serializers once (#2862) * CORDA-1258 - Only register custom serializers once * Review comments * Fix test --- .../core/serialization/SerializationAPI.kt | 2 +- docs/source/changelog.rst | 4 + .../amqp/AMQPSerializationScheme.kt | 15 ++- .../serialization/amqp/SerializerFactory.kt | 2 +- .../ForbiddenLambdaSerializationTests.java | 18 ++-- .../amqp/SerializationSchemaTests.kt | 98 +++++++++++++++++++ 6 files changed, 125 insertions(+), 14 deletions(-) create mode 100644 node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationSchemaTests.kt diff --git a/core/src/main/kotlin/net/corda/core/serialization/SerializationAPI.kt b/core/src/main/kotlin/net/corda/core/serialization/SerializationAPI.kt index b8e90fc8c2..dd94c74898 100644 --- a/core/src/main/kotlin/net/corda/core/serialization/SerializationAPI.kt +++ b/core/src/main/kotlin/net/corda/core/serialization/SerializationAPI.kt @@ -182,7 +182,7 @@ interface SerializationContext { /** * The use case that we are serializing for, since it influences the implementations chosen. */ - enum class UseCase { P2P, RPCServer, RPCClient, Storage, Checkpoint } + enum class UseCase { P2P, RPCServer, RPCClient, Storage, Checkpoint, Testing } } /** diff --git a/docs/source/changelog.rst b/docs/source/changelog.rst index 6ba192aa06..e8641a5c49 100644 --- a/docs/source/changelog.rst +++ b/docs/source/changelog.rst @@ -7,6 +7,10 @@ Unreleased Here are brief summaries of what's changed between each snapshot release. This includes guidance on how to upgrade code from the previous milestone release. +* Fix CORDA-1258. Custom serializers were spuriously registered every time a serialization factory was fetched from the cache rather than + only once when it was created. Whilst registering serializers that already exist is essentially a no-op, it's a performance overhead for + a very frequent operation that hits a synchronisation point (and is thus flagged as contended by our perfomance suite) + * Update the fast-classpath-scanner dependent library version from 2.0.21 to 2.12.3 .. note:: Whilst this is not the latest version of this library, that being 2.18.1 at time of writing, versions later diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPSerializationScheme.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPSerializationScheme.kt index 96d01fa287..4d0ca7b924 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPSerializationScheme.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPSerializationScheme.kt @@ -29,8 +29,15 @@ fun SerializerFactory.addToWhitelist(vararg types: Class<*>) { } } -abstract class AbstractAMQPSerializationScheme(val cordappLoader: List) : SerializationScheme { +open class SerializerFactoryFactory { + open fun make(context: SerializationContext) = + SerializerFactory(context.whitelist, context.deserializationClassLoader) +} +abstract class AbstractAMQPSerializationScheme( + val cordappLoader: List, + val sff : SerializerFactoryFactory = SerializerFactoryFactory() +) : SerializationScheme { // TODO: This method of initialisation for the Whitelist and plugin serializers will have to change // when we have per-cordapp contexts and dynamic app reloading but for now it's the easiest way companion object { @@ -116,9 +123,11 @@ abstract class AbstractAMQPSerializationScheme(val cordappLoader: List) rpcClientSerializerFactory(context) SerializationContext.UseCase.RPCServer -> rpcServerSerializerFactory(context) - else -> SerializerFactory(context.whitelist, context.deserializationClassLoader) + else -> sff.make(context) + }.also { + registerCustomSerializers(it) } - }.also { registerCustomSerializers(it) } + } } override fun deserialize(byteSequence: ByteSequence, clazz: Class, context: SerializationContext): T { diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt index 94f319f697..9041062452 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt @@ -198,7 +198,7 @@ open class SerializerFactory( * Register a custom serializer for any type that cannot be serialized or deserialized by the default serializer * that expects to find getters and a constructor with a parameter for each property. */ - fun register(customSerializer: CustomSerializer) { + open fun register(customSerializer: CustomSerializer) { if (!serializersByDescriptor.containsKey(customSerializer.typeDescriptor)) { customSerializers += customSerializer serializersByDescriptor[customSerializer.typeDescriptor] = customSerializer diff --git a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/ForbiddenLambdaSerializationTests.java b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/ForbiddenLambdaSerializationTests.java index 123bf60e0f..c798548987 100644 --- a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/ForbiddenLambdaSerializationTests.java +++ b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/ForbiddenLambdaSerializationTests.java @@ -4,9 +4,9 @@ import com.google.common.collect.Maps; import net.corda.core.serialization.SerializationContext; import net.corda.core.serialization.SerializationFactory; import net.corda.core.serialization.SerializedBytes; -import net.corda.testing.core.SerializationEnvironmentRule; import net.corda.nodeapi.internal.serialization.kryo.CordaClosureBlacklistSerializer; import net.corda.nodeapi.internal.serialization.kryo.KryoSerializationSchemeKt; +import net.corda.testing.core.SerializationEnvironmentRule; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -19,6 +19,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.ThrowableAssert.catchThrowable; public final class ForbiddenLambdaSerializationTests { + private EnumSet contexts = EnumSet.complementOf( + EnumSet.of(SerializationContext.UseCase.Checkpoint, SerializationContext.UseCase.Testing)); @Rule public final SerializationEnvironmentRule testSerialization = new SerializationEnvironmentRule(); private SerializationFactory factory; @@ -29,11 +31,10 @@ public final class ForbiddenLambdaSerializationTests { } @Test - public final void serialization_fails_for_serializable_java_lambdas() throws Exception { - EnumSet contexts = EnumSet.complementOf(EnumSet.of(SerializationContext.UseCase.Checkpoint)); - + public final void serialization_fails_for_serializable_java_lambdas() { contexts.forEach(ctx -> { - SerializationContext context = new SerializationContextImpl(KryoSerializationSchemeKt.getKryoMagic(), this.getClass().getClassLoader(), AllWhitelist.INSTANCE, Maps.newHashMap(), true, ctx, null); + SerializationContext context = new SerializationContextImpl(KryoSerializationSchemeKt.getKryoMagic(), + this.getClass().getClassLoader(), AllWhitelist.INSTANCE, Maps.newHashMap(), true, ctx, null); String value = "Hey"; Callable target = (Callable & Serializable) () -> value; @@ -51,11 +52,10 @@ public final class ForbiddenLambdaSerializationTests { @Test @SuppressWarnings("unchecked") - public final void serialization_fails_for_not_serializable_java_lambdas() throws Exception { - EnumSet contexts = EnumSet.complementOf(EnumSet.of(SerializationContext.UseCase.Checkpoint)); - + public final void serialization_fails_for_not_serializable_java_lambdas() { contexts.forEach(ctx -> { - SerializationContext context = new SerializationContextImpl(KryoSerializationSchemeKt.getKryoMagic(), this.getClass().getClassLoader(), AllWhitelist.INSTANCE, Maps.newHashMap(), true, ctx, null); + SerializationContext context = new SerializationContextImpl(KryoSerializationSchemeKt.getKryoMagic(), + this.getClass().getClassLoader(), AllWhitelist.INSTANCE, Maps.newHashMap(), true, ctx, null); String value = "Hey"; Callable target = () -> value; diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationSchemaTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationSchemaTests.kt new file mode 100644 index 0000000000..066278b2c7 --- /dev/null +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationSchemaTests.kt @@ -0,0 +1,98 @@ +package net.corda.nodeapi.internal.serialization.amqp + +import net.corda.core.serialization.* +import net.corda.core.utilities.ByteSequence +import net.corda.nodeapi.internal.serialization.* +import org.junit.Test +import kotlin.test.assertEquals + +// Make sure all serialization calls in this test don't get stomped on by anything else +val TESTING_CONTEXT = SerializationContextImpl(amqpMagic, + SerializationDefaults.javaClass.classLoader, + GlobalTransientClassWhiteList(BuiltInExceptionsWhitelist()), + emptyMap(), + true, + SerializationContext.UseCase.Testing, + null) + +// Test factory that lets us count the number of serializer registration attempts +class TestSerializerFactory( + wl : ClassWhitelist, + cl : ClassLoader +) : SerializerFactory(wl, cl) { + var registerCount = 0 + + override fun register(customSerializer: CustomSerializer) { + ++registerCount + return super.register(customSerializer) + } +} + +// Instance of our test factory counting registration attempts. Sucks its global, but for testing purposes this +// is the easiest way of getting access to the object. +val testFactory = TestSerializerFactory(TESTING_CONTEXT.whitelist, TESTING_CONTEXT.deserializationClassLoader) + +// Serializer factory factory, plugs into the SerializationScheme and controls which factory type +// we make for each use case. For our tests we need to make sure if its the Testing use case we return +// the global factory object created above that counts registrations. +class TestSerializerFactoryFactory : SerializerFactoryFactory() { + override fun make(context: SerializationContext) = + when (context.useCase) { + SerializationContext.UseCase.Testing -> testFactory + else -> super.make(context) + } +} + +class AMQPTestSerializationScheme : AbstractAMQPSerializationScheme(emptyList(), TestSerializerFactoryFactory()) { + override fun rpcClientSerializerFactory(context: SerializationContext): SerializerFactory { + throw UnsupportedOperationException() + } + + override fun rpcServerSerializerFactory(context: SerializationContext): SerializerFactory { + throw UnsupportedOperationException() + } + + override fun canDeserializeVersion(magic: CordaSerializationMagic, target: SerializationContext.UseCase) = true +} + +// Test SerializationFactory that wraps a serialization scheme that just allows us to call .serialize. +// Returns the testing scheme we created above that wraps the testing factory. +class TestSerializationFactory : SerializationFactory() { + private val scheme = AMQPTestSerializationScheme() + + override fun deserialize( + byteSequence: ByteSequence, + clazz: Class, context: + SerializationContext + ): T { + throw UnsupportedOperationException() + } + + override fun deserializeWithCompatibleContext( + byteSequence: ByteSequence, + clazz: Class, + context: SerializationContext + ): ObjectWithCompatibleContext { + throw UnsupportedOperationException() + } + + override fun serialize(obj: T, context: SerializationContext) = scheme.serialize(obj, context) +} + +// The actual test +class SerializationSchemaTests { + @Test + fun onlyRegisterCustomSerializersOnce() { + @CordaSerializable data class C(val a: Int) + + val c = C(1) + val testSerializationFactory = TestSerializationFactory() + val expectedCustomSerializerCount = 40 + + assertEquals (0, testFactory.registerCount) + c.serialize (testSerializationFactory, TESTING_CONTEXT) + assertEquals (expectedCustomSerializerCount, testFactory.registerCount) + c.serialize (testSerializationFactory, TESTING_CONTEXT) + assertEquals (expectedCustomSerializerCount, testFactory.registerCount) + } +} \ No newline at end of file From 0f99efa768aaa6180bc24f0511e2c050dbceeb34 Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Tue, 27 Mar 2018 10:06:46 +0100 Subject: [PATCH 2/3] CORDA-1213 - Explicitly disallow serialization of non static nested classes (#2824) * CORDA-1213 - Explicitly disallow serialization of non static nested classes WIP * Review comments --- docs/source/changelog.rst | 6 + .../serialization/amqp/AMQPExceptions.kt | 7 + .../serialization/amqp/ObjectSerializer.kt | 34 ++- .../serialization/amqp/SerializationHelper.kt | 25 +- .../amqp/JavaNestedClassesTests.java | 224 ++++++++++++++++++ .../amqp/JavaNestedInheritenceTests.java | 70 ++++++ .../amqp/JavaSerializationOutputTests.java | 23 +- .../amqp/SerializationOutputTests.kt | 81 ++++++- 8 files changed, 431 insertions(+), 39 deletions(-) create mode 100644 node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPExceptions.kt create mode 100644 node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaNestedClassesTests.java create mode 100644 node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaNestedInheritenceTests.java diff --git a/docs/source/changelog.rst b/docs/source/changelog.rst index e8641a5c49..06c57db11f 100644 --- a/docs/source/changelog.rst +++ b/docs/source/changelog.rst @@ -7,6 +7,12 @@ Unreleased Here are brief summaries of what's changed between each snapshot release. This includes guidance on how to upgrade code from the previous milestone release. +* Serializing an inner class (non-static nested class in Java, inner class in Kotlin) will be rejected explicitly by the serialization + framework. Prior to this change it didn't work, but the error thrown was opaque (complaining about too few arguments + to a constructor). Whilst this was possible in the older Kryo implementation (Kryo passing null as the synthesised + reference to the outer class) as per the Java documentation `here `_ + we are disallowing this as the paradigm in general makes little sense for Contract States + * Fix CORDA-1258. Custom serializers were spuriously registered every time a serialization factory was fetched from the cache rather than only once when it was created. Whilst registering serializers that already exist is essentially a no-op, it's a performance overhead for a very frequent operation that hits a synchronisation point (and is thus flagged as contended by our perfomance suite) diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPExceptions.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPExceptions.kt new file mode 100644 index 0000000000..0f93f0947e --- /dev/null +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/AMQPExceptions.kt @@ -0,0 +1,7 @@ +package net.corda.nodeapi.internal.serialization.amqp + +import java.io.NotSerializableException +import java.lang.reflect.Type + +class SyntheticParameterException(val type: Type) : NotSerializableException("Type '${type.typeName} has synthetic " + + "fields and is likely a nested inner class. This is not support by the Corda AMQP serialization framework") \ No newline at end of file diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt index e4dddec1da..2403d825c8 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/ObjectSerializer.kt @@ -1,7 +1,6 @@ package net.corda.nodeapi.internal.serialization.amqp import net.corda.core.utilities.contextLogger -import net.corda.core.utilities.debug import net.corda.core.utilities.trace import net.corda.nodeapi.internal.serialization.amqp.SerializerFactory.Companion.nameForType import org.apache.qpid.proton.amqp.Symbol @@ -58,13 +57,22 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS data: Data, type: Type, output: SerializationOutput, - debugIndent: Int) = ifThrowsAppend({ clazz.typeName }) { + debugIndent: Int) = ifThrowsAppend({ clazz.typeName } + ) { + if (propertySerializers.size != javaConstructor?.parameterCount && + javaConstructor?.parameterCount ?: 0 > 0 + ) { + throw NotSerializableException("Serialization constructor for class $type expects " + + "${javaConstructor?.parameterCount} parameters but we have ${propertySerializers.size} " + + "properties to serialize.") + } + // Write described data.withDescribed(typeNotation.descriptor) { // Write list withList { propertySerializers.serializationOrder.forEach { property -> - property.getter.writeProperty(obj, this, output, debugIndent+1) + property.getter.writeProperty(obj, this, output, debugIndent + 1) } } } @@ -92,23 +100,23 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS private fun readObjectBuildViaConstructor( obj: List<*>, schemas: SerializationSchemas, - input: DeserializationInput) : Any = ifThrowsAppend({ clazz.typeName }){ + input: DeserializationInput): Any = ifThrowsAppend({ clazz.typeName }) { logger.trace { "Calling construction based construction for ${clazz.typeName}" } - return construct (propertySerializers.serializationOrder + return construct(propertySerializers.serializationOrder .zip(obj) .map { Pair(it.first.initialPosition, it.first.getter.readProperty(it.second, schemas, input)) } - .sortedWith(compareBy({it.first})) + .sortedWith(compareBy({ it.first })) .map { it.second }) } private fun readObjectBuildViaSetters( obj: List<*>, schemas: SerializationSchemas, - input: DeserializationInput) : Any = ifThrowsAppend({ clazz.typeName }){ + input: DeserializationInput): Any = ifThrowsAppend({ clazz.typeName }) { logger.trace { "Calling setter based construction for ${clazz.typeName}" } - val instance : Any = javaConstructor?.newInstance() ?: throw NotSerializableException ( + val instance: Any = javaConstructor?.newInstance() ?: throw NotSerializableException( "Failed to instantiate instance of object $clazz") // read the properties out of the serialised form, since we're invoking the setters the order we @@ -136,7 +144,13 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS fun construct(properties: List): Any { logger.trace { "Calling constructor: '$javaConstructor' with properties '$properties'" } - return javaConstructor?.newInstance(*properties.toTypedArray()) ?: - throw NotSerializableException("Attempt to deserialize an interface: $clazz. Serialized form is invalid.") + if (properties.size != javaConstructor?.parameterCount) { + throw NotSerializableException("Serialization constructor for class $type expects " + + "${javaConstructor?.parameterCount} parameters but we have ${properties.size} " + + "serialized properties.") + } + + return javaConstructor?.newInstance(*properties.toTypedArray()) + ?: throw NotSerializableException("Attempt to deserialize an interface: $clazz. Serialized form is invalid.") } } \ No newline at end of file diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationHelper.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationHelper.kt index cda4b76ac2..f9d9efad63 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationHelper.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationHelper.kt @@ -17,6 +17,7 @@ import kotlin.reflect.KParameter import kotlin.reflect.full.findAnnotation import kotlin.reflect.full.primaryConstructor import kotlin.reflect.jvm.isAccessible +import kotlin.reflect.jvm.javaConstructor import kotlin.reflect.jvm.javaType /** @@ -33,6 +34,7 @@ internal fun constructorForDeserialization(type: Type): KFunction? { var annotatedCount = 0 val kotlinConstructors = clazz.kotlin.constructors val hasDefault = kotlinConstructors.any { it.parameters.isEmpty() } + for (kotlinConstructor in kotlinConstructors) { if (preferredCandidate == null && kotlinConstructors.size == 1) { preferredCandidate = kotlinConstructor @@ -158,7 +160,7 @@ fun Class.propertyDescriptors(): Map { PropertyDescriptorsRegex.re.find(func.name)?.apply { // matching means we have an func getX where the property could be x or X // so having pre-loaded all of the properties we try to match to either case. If that - // fails the getter doesn't refer to a property directly, but may to a cosntructor + // fails the getter doesn't refer to a property directly, but may refer to a constructor // parameter that shadows a property val properties = classProperties[groups[2]!!.value] ?: @@ -219,19 +221,26 @@ internal fun propertiesForSerializationFromConstructor( val classProperties = clazz.propertyDescriptors() + // Annoyingly there isn't a better way to ascertain that the constructor for the class + // has a synthetic parameter inserted to capture the reference to the outer class. You'd + // think you could inspect the parameter and check the isSynthetic flag but that is always + // false so given the naming convention is specified by the standard we can just check for + // this + if (kotlinConstructor.javaConstructor?.parameterCount ?: 0 > 0 && + kotlinConstructor.javaConstructor?.parameters?.get(0)?.name == "this$0" + ) { + throw SyntheticParameterException(type) + } + if (classProperties.isNotEmpty() && kotlinConstructor.parameters.isEmpty()) { return propertiesForSerializationFromSetters(classProperties, type, factory) } return mutableListOf().apply { kotlinConstructor.parameters.withIndex().forEach { param -> - // If a parameter doesn't have a name *at all* then chances are it's a synthesised - // one. A good example of this is non static nested classes in Java where instances - // of the nested class require access to the outer class without breaking - // encapsulation. Thus a parameter is inserted into the constructor that passes a - // reference to the enclosing class. In this case we can't do anything with - // it so just ignore it as it'll be supplied at runtime anyway on invocation - val name = param.value.name ?: return@forEach + // name cannot be null, if it is then this is a synthetic field and we will have bailed + // out prior to this + val name = param.value.name!! // We will already have disambiguated getA for property A or a but we still need to cope // with the case we don't know the case of A when the parameter doesn't match a property diff --git a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaNestedClassesTests.java b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaNestedClassesTests.java new file mode 100644 index 0000000000..66cd995220 --- /dev/null +++ b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaNestedClassesTests.java @@ -0,0 +1,224 @@ +package net.corda.nodeapi.internal.serialization.amqp; + +import com.google.common.collect.ImmutableList; +import kotlin.Suppress; +import net.corda.core.contracts.ContractState; +import net.corda.core.identity.AbstractParty; +import net.corda.core.serialization.SerializedBytes; +import net.corda.nodeapi.internal.serialization.AllWhitelist; +import org.assertj.core.api.Assertions; +import org.jetbrains.annotations.NotNull; +import org.junit.Test; + +import java.io.NotSerializableException; +import java.util.List; + +class OuterClass1 { + protected SerializationOutput ser; + DeserializationInput desExisting; + DeserializationInput desRegen; + + OuterClass1() { + SerializerFactory factory1 = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader(), + new EvolutionSerializerGetter(), + new SerializerFingerPrinter()); + + SerializerFactory factory2 = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader(), + new EvolutionSerializerGetter(), + new SerializerFingerPrinter()); + + this.ser = new SerializationOutput(factory1); + this.desExisting = new DeserializationInput(factory1); + this.desRegen = new DeserializationInput(factory2); + } + + class DummyState implements ContractState { + @Override @NotNull public List getParticipants() { + return ImmutableList.of(); + } + } + + public void run() throws NotSerializableException { + SerializedBytes b = ser.serialize(new DummyState()); + desExisting.deserialize(b, DummyState.class); + desRegen.deserialize(b, DummyState.class); + } +} + +class Inherator1 extends OuterClass1 { + public void iRun() throws NotSerializableException { + SerializedBytes b = ser.serialize(new DummyState()); + desExisting.deserialize(b, DummyState.class); + desRegen.deserialize(b, DummyState.class); + } +} + +class OuterClass2 { + protected SerializationOutput ser; + DeserializationInput desExisting; + DeserializationInput desRegen; + + OuterClass2() { + SerializerFactory factory1 = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader(), + new EvolutionSerializerGetter(), + new SerializerFingerPrinter()); + + SerializerFactory factory2 = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader(), + new EvolutionSerializerGetter(), + new SerializerFingerPrinter()); + + this.ser = new SerializationOutput(factory1); + this.desExisting = new DeserializationInput(factory1); + this.desRegen = new DeserializationInput(factory2); + } + + protected class DummyState implements ContractState { + private Integer count; + + DummyState(Integer count) { + this.count = count; + } + + @Override @NotNull public List getParticipants() { + return ImmutableList.of(); + } + } + + public void run() throws NotSerializableException { + SerializedBytes b = ser.serialize(new DummyState(12)); + desExisting.deserialize(b, DummyState.class); + desRegen.deserialize(b, DummyState.class); + } +} + +class Inherator2 extends OuterClass2 { + public void iRun() throws NotSerializableException { + SerializedBytes b = ser.serialize(new DummyState(12)); + desExisting.deserialize(b, DummyState.class); + desRegen.deserialize(b, DummyState.class); + } +} + +// Make the base class abstract +abstract class AbstractClass2 { + protected SerializationOutput ser; + + AbstractClass2() { + SerializerFactory factory = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader(), + new EvolutionSerializerGetter(), + new SerializerFingerPrinter()); + + this.ser = new SerializationOutput(factory); + } + + protected class DummyState implements ContractState { + @Override @NotNull public List getParticipants() { + return ImmutableList.of(); + } + } +} + +class Inherator4 extends AbstractClass2 { + public void run() throws NotSerializableException { + ser.serialize(new DummyState()); + } +} + +abstract class AbstractClass3 { + protected class DummyState implements ContractState { + @Override @NotNull public List getParticipants() { + return ImmutableList.of(); + } + } +} + +class Inherator5 extends AbstractClass3 { + public void run() throws NotSerializableException { + SerializerFactory factory = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader(), + new EvolutionSerializerGetter(), + new SerializerFingerPrinter()); + + SerializationOutput ser = new SerializationOutput(factory); + ser.serialize(new DummyState()); + } +} + +class Inherator6 extends AbstractClass3 { + public class Wrapper { + //@Suppress("UnusedDeclaration"]) + private ContractState cState; + + Wrapper(ContractState cState) { + this.cState = cState; + } + } + + public void run() throws NotSerializableException { + SerializerFactory factory = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader(), + new EvolutionSerializerGetter(), + new SerializerFingerPrinter()); + + SerializationOutput ser = new SerializationOutput(factory); + ser.serialize(new Wrapper(new DummyState())); + } +} + +public class JavaNestedClassesTests { + @Test + public void publicNested() { + Assertions.assertThatThrownBy(() -> new OuterClass1().run()).isInstanceOf( + NotSerializableException.class).hasMessageContaining( + "has synthetic fields and is likely a nested inner class"); + } + + @Test + public void privateNested() { + Assertions.assertThatThrownBy(() -> new OuterClass2().run()).isInstanceOf( + NotSerializableException.class).hasMessageContaining( + "has synthetic fields and is likely a nested inner class"); + } + + @Test + public void publicNestedInherited() { + Assertions.assertThatThrownBy(() -> new Inherator1().run()).isInstanceOf( + NotSerializableException.class).hasMessageContaining( + "has synthetic fields and is likely a nested inner class"); + + Assertions.assertThatThrownBy(() -> new Inherator1().iRun()).isInstanceOf( + NotSerializableException.class).hasMessageContaining( + "has synthetic fields and is likely a nested inner class"); + } + + @Test + public void protectedNestedInherited() { + Assertions.assertThatThrownBy(() -> new Inherator2().run()).isInstanceOf( + NotSerializableException.class).hasMessageContaining( + "has synthetic fields and is likely a nested inner class"); + + Assertions.assertThatThrownBy(() -> new Inherator2().iRun()).isInstanceOf( + NotSerializableException.class).hasMessageContaining( + "has synthetic fields and is likely a nested inner class"); + } + + @Test + public void abstractNested() { + Assertions.assertThatThrownBy(() -> new Inherator4().run()).isInstanceOf( + NotSerializableException.class).hasMessageContaining( + "has synthetic fields and is likely a nested inner class"); + } + + @Test + public void abstractNestedFactoryOnNested() { + Assertions.assertThatThrownBy(() -> new Inherator5().run()).isInstanceOf( + NotSerializableException.class).hasMessageContaining( + "has synthetic fields and is likely a nested inner class"); + } + + @Test + public void abstractNestedFactoryOnNestedInWrapper() { + Assertions.assertThatThrownBy(() -> new Inherator6().run()).isInstanceOf( + NotSerializableException.class).hasMessageContaining( + "has synthetic fields and is likely a nested inner class"); + } +} + diff --git a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaNestedInheritenceTests.java b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaNestedInheritenceTests.java new file mode 100644 index 0000000000..548f3625a3 --- /dev/null +++ b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaNestedInheritenceTests.java @@ -0,0 +1,70 @@ +package net.corda.nodeapi.internal.serialization.amqp; + +import com.google.common.collect.ImmutableList; +import net.corda.core.contracts.ContractState; +import net.corda.core.identity.AbstractParty; +import net.corda.nodeapi.internal.serialization.AllWhitelist; +import org.assertj.core.api.Assertions; +import org.jetbrains.annotations.NotNull; +import org.junit.Test; + +import java.io.NotSerializableException; +import java.util.List; + +abstract class JavaNestedInheritenceTestsBase { + class DummyState implements ContractState { + @Override @NotNull public List getParticipants() { + return ImmutableList.of(); + } + } +} + +class Wrapper { + private ContractState cs; + Wrapper(ContractState cs) { this.cs = cs; } +} + +class TemplateWrapper { + public T obj; + TemplateWrapper(T obj) { this.obj = obj; } +} + +public class JavaNestedInheritenceTests extends JavaNestedInheritenceTestsBase { + @Test + public void serializeIt() { + SerializerFactory factory = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader(), + new EvolutionSerializerGetter(), + new SerializerFingerPrinter()); + + SerializationOutput ser = new SerializationOutput(factory); + + Assertions.assertThatThrownBy(() -> ser.serialize(new DummyState())).isInstanceOf( + NotSerializableException.class).hasMessageContaining( + "has synthetic fields and is likely a nested inner class"); + } + + @Test + public void serializeIt2() { + SerializerFactory factory = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader(), + new EvolutionSerializerGetter(), + new SerializerFingerPrinter()); + + SerializationOutput ser = new SerializationOutput(factory); + Assertions.assertThatThrownBy(() -> ser.serialize(new Wrapper (new DummyState()))).isInstanceOf( + NotSerializableException.class).hasMessageContaining( + "has synthetic fields and is likely a nested inner class"); + } + + @Test + public void serializeIt3() throws NotSerializableException { + SerializerFactory factory1 = new SerializerFactory(AllWhitelist.INSTANCE, ClassLoader.getSystemClassLoader(), + new EvolutionSerializerGetter(), + new SerializerFingerPrinter()); + + SerializationOutput ser = new SerializationOutput(factory1); + + Assertions.assertThatThrownBy(() -> ser.serialize(new TemplateWrapper (new DummyState()))).isInstanceOf( + NotSerializableException.class).hasMessageContaining( + "has synthetic fields and is likely a nested inner class"); + } +} diff --git a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaSerializationOutputTests.java b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaSerializationOutputTests.java index b6f55d3e73..e62775218b 100644 --- a/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaSerializationOutputTests.java +++ b/node-api/src/test/java/net/corda/nodeapi/internal/serialization/amqp/JavaSerializationOutputTests.java @@ -8,6 +8,7 @@ import net.corda.nodeapi.internal.serialization.AllWhitelist; import net.corda.core.serialization.SerializedBytes; import org.apache.qpid.proton.codec.DecoderImpl; import org.apache.qpid.proton.codec.EncoderImpl; +import org.jetbrains.annotations.NotNull; import org.junit.Test; import javax.annotation.Nonnull; @@ -111,10 +112,12 @@ public class JavaSerializationOutputTests { this.count = count; } + @SuppressWarnings("unused") public String getFred() { return fred; } + @SuppressWarnings("unused") public Integer getCount() { return count; } @@ -242,24 +245,4 @@ public class JavaSerializationOutputTests { BoxedFooNotNull obj = new BoxedFooNotNull("Hello World!", 123); serdes(obj); } - - protected class DummyState implements ContractState { - @Override - public List getParticipants() { - return ImmutableList.of(); - } - } - - @Test - public void dummyStateSerialize() throws NotSerializableException { - SerializerFactory factory1 = new SerializerFactory( - AllWhitelist.INSTANCE, - ClassLoader.getSystemClassLoader(), - new EvolutionSerializerGetter(), - new SerializerFingerPrinter()); - - SerializationOutput serializer = new SerializationOutput(factory1); - - serializer.serialize(new DummyState()); - } } diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutputTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutputTests.kt index 9f2921e8f3..822bf86396 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutputTests.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializationOutputTests.kt @@ -1231,7 +1231,86 @@ class SerializationOutputTests(private val compression: CordaSerializationEncodi val testExcp = TestException("hello", Throwable().apply { stackTrace = Thread.currentThread().stackTrace }) val factory = testDefaultFactoryNoEvolution() SerializationOutput(factory).serialize(testExcp) - } + + @Test + fun nestedInner() { + class C(val a: Int) { + inner class D(val b: Int) + + fun serialize() { + val factory = testDefaultFactoryNoEvolution() + SerializationOutput(factory).serialize(D(4)) + } + } + + // By the time we escape the serializer we should just have a general + // NotSerializable Exception + assertThatExceptionOfType(NotSerializableException::class.java).isThrownBy { + C(12).serialize() + }.withMessageContaining("has synthetic fields and is likely a nested inner class") + } + + @Test + fun nestedNestedInner() { + class C(val a: Int) { + inner class D(val b: Int) { + inner class E(val c: Int) + + fun serialize() { + val factory = testDefaultFactoryNoEvolution() + SerializationOutput(factory).serialize(E(4)) + } + } + + fun serializeD() { + val factory = testDefaultFactoryNoEvolution() + SerializationOutput(factory).serialize(D(4)) + } + + fun serializeE() { + D(1).serialize() + } + } + + // By the time we escape the serializer we should just have a general + // NotSerializable Exception + assertThatExceptionOfType(NotSerializableException::class.java).isThrownBy { + C(12).serializeD() + }.withMessageContaining("has synthetic fields and is likely a nested inner class") + + assertThatExceptionOfType(NotSerializableException::class.java).isThrownBy { + C(12).serializeE() + }.withMessageContaining("has synthetic fields and is likely a nested inner class") + } + + @Test + fun multiNestedInner() { + class C(val a: Int) { + inner class D(val b: Int) + inner class E(val c: Int) + + fun serializeD() { + val factory = testDefaultFactoryNoEvolution() + SerializationOutput(factory).serialize(D(4)) + } + + fun serializeE() { + val factory = testDefaultFactoryNoEvolution() + SerializationOutput(factory).serialize(E(4)) + } + } + + // By the time we escape the serializer we should just have a general + // NotSerializable Exception + assertThatExceptionOfType(NotSerializableException::class.java).isThrownBy { + C(12).serializeD() + }.withMessageContaining("has synthetic fields and is likely a nested inner class") + + assertThatExceptionOfType(NotSerializableException::class.java).isThrownBy { + C(12).serializeE() + }.withMessageContaining("has synthetic fields and is likely a nested inner class") + } + } From e43b12c203730cff9f5a46eebeef80977122824f Mon Sep 17 00:00:00 2001 From: Katelyn Baker Date: Tue, 27 Mar 2018 10:11:39 +0100 Subject: [PATCH 3/3] CORDA-1236 - Don't let Carpenter exceptions escape the serializer (#2852) * CORDA-1236 - Don't let Carpenter exceptions escape the serializer * Review comments * Merge branch 'kat/bug/master/nestedCArpenterException' of https://github.com/corda/corda into kat/bug/master/nestedCArpenterException --- docs/source/changelog.rst | 4 + .../amqp/DeserializationInput.kt | 5 +- .../serialization/amqp/SerializerFactory.kt | 21 +++- .../carpenter/AMQPSchemaExtensions.kt | 11 ++- .../serialization/carpenter/ClassCarpenter.kt | 22 +++-- .../serialization/carpenter/Exceptions.kt | 32 +++++-- .../serialization/carpenter/MetaCarpenter.kt | 14 ++- .../serialization/carpenter/SchemaFields.kt | 3 +- .../serialization/amqp/EnumEvolveTests.kt | 6 ++ .../carpenter/CarpenterExceptionTests.kt | 96 +++++++++++++++++++ .../serialization/{ => kryo}/KryoTests.kt | 4 +- 11 files changed, 184 insertions(+), 34 deletions(-) create mode 100644 node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/carpenter/CarpenterExceptionTests.kt rename node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/{ => kryo}/KryoTests.kt (99%) diff --git a/docs/source/changelog.rst b/docs/source/changelog.rst index 06c57db11f..4d704bd66c 100644 --- a/docs/source/changelog.rst +++ b/docs/source/changelog.rst @@ -24,6 +24,10 @@ from the previous milestone release. * Node can be shut down abruptly by ``shutdown`` function in `CordaRPCOps` or gracefully (draining flows first) through ``gracefulShutdown`` command from shell. +* Carpenter Exceptions will be caught internally by the Serializer and rethrown as a ``NotSerializableException`` + + * Specific details of the error encountered are logged to the node's log file. More information can be enabled by setting the debug level to ``trace`` ; this will cause the full stack trace of the error to be dumped into the log. + * Parsing of ``NodeConfiguration`` will now fail if unknown configuration keys are found. * The web server now has its own ``web-server.conf`` file, separate from ``node.conf``. diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializationInput.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializationInput.kt index cdde047ef8..04c85e4926 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializationInput.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/DeserializationInput.kt @@ -153,9 +153,12 @@ class DeserializationInput @JvmOverloads constructor(private val serializerFacto is DescribedType -> { // Look up serializer in factory by descriptor val serializer = serializerFactory.get(obj.descriptor, schemas) - if (SerializerFactory.AnyType != type && serializer.type != type && with(serializer.type) { !isSubClassOf(type) && !materiallyEquivalentTo(type) }) + if (SerializerFactory.AnyType != type && serializer.type != type && with(serializer.type) { + !isSubClassOf(type) && !materiallyEquivalentTo(type) + }) { throw NotSerializableException("Described type with descriptor ${obj.descriptor} was " + "expected to be of type $type but was ${serializer.type}") + } serializer.readObject(obj.described, schemas, this) } is Binary -> obj.array diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt index 9041062452..b0f3d4746b 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/amqp/SerializerFactory.kt @@ -2,11 +2,12 @@ package net.corda.nodeapi.internal.serialization.amqp import com.google.common.primitives.Primitives import com.google.common.reflect.TypeResolver +import net.corda.core.internal.getStackTraceAsString import net.corda.core.internal.uncheckedCast import net.corda.core.serialization.ClassWhitelist -import net.corda.nodeapi.internal.serialization.carpenter.CarpenterMetaSchema -import net.corda.nodeapi.internal.serialization.carpenter.ClassCarpenter -import net.corda.nodeapi.internal.serialization.carpenter.MetaCarpenter +import net.corda.core.utilities.contextLogger +import net.corda.core.utilities.loggerFor +import net.corda.nodeapi.internal.serialization.carpenter.* import org.apache.qpid.proton.amqp.* import java.io.NotSerializableException import java.lang.reflect.* @@ -238,7 +239,19 @@ open class SerializerFactory( if (metaSchema.isNotEmpty()) { val mc = MetaCarpenter(metaSchema, classCarpenter) - mc.build() + try { + mc.build() + } catch (e: MetaCarpenterException) { + // preserve the actual message locally + loggerFor().apply { + error ("${e.message} [hint: enable trace debugging for the stack trace]") + trace (e.getStackTraceAsString()) + } + + // prevent carpenter exceptions escaping into the world, convert things into a nice + // NotSerializableException for when this escapes over the wire + throw NotSerializableException(e.name) + } processSchema(schemaAndDescriptor, true) } } diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/AMQPSchemaExtensions.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/AMQPSchemaExtensions.kt index 2c7dea9605..c400f94ff0 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/AMQPSchemaExtensions.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/AMQPSchemaExtensions.kt @@ -133,11 +133,12 @@ fun AMQPField.getTypeAsClass(classloader: ClassLoader) = typeStrToType[Pair(type else -> classloader.loadClass(type.stripGenerics()) } -fun AMQPField.validateType(classloader: ClassLoader) = when (type) { - "byte", "int", "string", "short", "long", "char", "boolean", "double", "float" -> true - "*" -> classloader.exists(requires[0]) - else -> classloader.exists(type) -} +fun AMQPField.validateType(classloader: ClassLoader) = + when (type) { + "byte", "int", "string", "short", "long", "char", "boolean", "double", "float" -> true + "*" -> classloader.exists(requires[0]) + else -> classloader.exists(type) + } private fun ClassLoader.exists(clazz: String) = run { try { diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenter.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenter.kt index 2c98ebbc07..070116c2d4 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenter.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/ClassCarpenter.kt @@ -8,6 +8,7 @@ import org.objectweb.asm.Opcodes.* import org.objectweb.asm.Type import java.lang.Character.isJavaIdentifierPart import java.lang.Character.isJavaIdentifierStart +import java.lang.reflect.Method import java.util.* /** @@ -25,6 +26,12 @@ class CarpenterClassLoader(parentClassLoader: ClassLoader = Thread.currentThread fun load(name: String, bytes: ByteArray) = defineClass(name, bytes, 0, bytes.size) } +class InterfaceMismatchNonGetterException (val clazz: Class<*>, val method: Method) : InterfaceMismatchException( + "Requested interfaces must consist only of methods that start with 'get': ${clazz.name}.${method.name}") + +class InterfaceMismatchMissingAMQPFieldException (val clazz: Class<*>, val field: String) : InterfaceMismatchException( + "Interface ${clazz.name} requires a field named $field but that isn't found in the schema or any superclass schemas") + /** * Which version of the java runtime are we constructing objects against */ @@ -405,7 +412,7 @@ class ClassCarpenter(cl: ClassLoader = Thread.currentThread().contextClassLoader * whitelisted classes */ private fun validateSchema(schema: Schema) { - if (schema.name in _loaded) throw DuplicateNameException() + if (schema.name in _loaded) throw DuplicateNameException(schema.name) fun isJavaName(n: String) = n.isNotBlank() && isJavaIdentifierStart(n.first()) && n.all(::isJavaIdentifierPart) require(isJavaName(schema.name.split(".").last())) { "Not a valid Java name: ${schema.name}" } schema.fields.forEach { @@ -420,20 +427,17 @@ class ClassCarpenter(cl: ClassLoader = Thread.currentThread().contextClassLoader itf.methods.forEach { val fieldNameFromItf = when { it.name.startsWith("get") -> it.name.substring(3).decapitalize() - else -> throw InterfaceMismatchException( - "Requested interfaces must consist only of methods that start " - + "with 'get': ${itf.name}.${it.name}") + else -> throw InterfaceMismatchNonGetterException(itf, it) } - // If we're trying to carpent a class that prior to serialisation / deserialisation + // If we're trying to carpent a class that prior to serialisation / deserialization // was made by a carpenter then we can ignore this (it will implement a plain get // method from SimpleFieldAccess). if (fieldNameFromItf.isEmpty() && SimpleFieldAccess::class.java in schema.interfaces) return@forEach - if ((schema is ClassSchema) and (fieldNameFromItf !in allFields)) - throw InterfaceMismatchException( - "Interface ${itf.name} requires a field named $fieldNameFromItf but that " - + "isn't found in the schema or any superclass schemas") + if ((schema is ClassSchema) and (fieldNameFromItf !in allFields)) { + throw InterfaceMismatchMissingAMQPFieldException(itf, fieldNameFromItf) + } } } } diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/Exceptions.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/Exceptions.kt index a260d82272..c7c2830c62 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/Exceptions.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/Exceptions.kt @@ -1,14 +1,34 @@ package net.corda.nodeapi.internal.serialization.carpenter -import net.corda.core.CordaException import net.corda.core.CordaRuntimeException +import org.objectweb.asm.Type -class DuplicateNameException : CordaRuntimeException( - "An attempt was made to register two classes with the same name within the same ClassCarpenter namespace.") +/** + * The general exception type thrown by the [ClassCarpenter] + */ +abstract class ClassCarpenterException(msg: String) : CordaRuntimeException(msg) -class InterfaceMismatchException(msg: String) : CordaRuntimeException(msg) +/** + * Thrown by the [ClassCarpenter] when trying to build + */ +abstract class InterfaceMismatchException(msg: String) : ClassCarpenterException(msg) -class NullablePrimitiveException(msg: String) : CordaRuntimeException(msg) +class DuplicateNameException(val name : String) : ClassCarpenterException ( + "An attempt was made to register two classes with the name '$name' within the same ClassCarpenter namespace.") + +class NullablePrimitiveException(val name: String, val field: Class) : ClassCarpenterException( + "Field $name is primitive type ${Type.getDescriptor(field)} and thus cannot be nullable") class UncarpentableException(name: String, field: String, type: String) : - CordaException("Class $name is loadable yet contains field $field of unknown type $type") + ClassCarpenterException("Class $name is loadable yet contains field $field of unknown type $type") + +/** + * A meta exception used by the [MetaCarpenter] to wrap any exceptions generated during the build + * process and associate those with the current schema being processed. This makes for cleaner external + * error hand + * + * @property name The name of the schema, and thus the class being created, when the error was occured + * @property e The [ClassCarpenterException] this is wrapping + */ +class MetaCarpenterException(val name: String, val e: ClassCarpenterException) : CordaRuntimeException( + "Whilst processing class '$name' - ${e.message}") diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/MetaCarpenter.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/MetaCarpenter.kt index ae1f7f0e7d..819b8a79da 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/MetaCarpenter.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/MetaCarpenter.kt @@ -5,13 +5,13 @@ import net.corda.nodeapi.internal.serialization.amqp.RestrictedType import net.corda.nodeapi.internal.serialization.amqp.TypeNotation /** - * Generated from an AMQP schema this class represents the classes unknown to the deserialiser and that thusly - * require carpenting up in bytecode form. This is a multi step process as carpenting one object may be depedent + * Generated from an AMQP schema this class represents the classes unknown to the deserializer and that thusly + * require carpenting up in bytecode form. This is a multi step process as carpenting one object may be dependent * upon the creation of others, this information is tracked in the dependency tree represented by * [dependencies] and [dependsOn]. Creatable classes are stored in [carpenterSchemas]. * * The state of this class after initial generation is expected to mutate as classes are built by the carpenter - * enablaing the resolution of dependencies and thus new carpenter schemas added whilst those already + * enabling the resolution of dependencies and thus new carpenter schemas added whilst those already * carpented schemas are removed. * * @property carpenterSchemas The list of carpentable classes @@ -55,7 +55,7 @@ data class CarpenterMetaSchema( /** * Take a dependency tree of [CarpenterMetaSchema] and reduce it to zero by carpenting those classes that - * require it. As classes are carpented check for depdency resolution, if now free generate a [Schema] for + * require it. As classes are carpented check for dependency resolution, if now free generate a [Schema] for * that class and add it to the list of classes ([CarpenterMetaSchema.carpenterSchemas]) that require * carpenting * @@ -95,7 +95,11 @@ class MetaCarpenter(schemas: CarpenterMetaSchema, cc: ClassCarpenter) : MetaCarp override fun build() { while (schemas.carpenterSchemas.isNotEmpty()) { val newObject = schemas.carpenterSchemas.removeAt(0) - step(newObject) + try { + step(newObject) + } catch (e: ClassCarpenterException) { + throw MetaCarpenterException(newObject.name, e) + } } } } diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/SchemaFields.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/SchemaFields.kt index effb5d40c4..0ce9a8f3d1 100644 --- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/SchemaFields.kt +++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/SchemaFields.kt @@ -98,8 +98,7 @@ class NullableField(field: Class) : ClassField(field) { init { if (field.isPrimitive) { - throw NullablePrimitiveException( - "Field $name is primitive type ${Type.getDescriptor(field)} and thus cannot be nullable") + throw NullablePrimitiveException(name, field) } } diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EnumEvolveTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EnumEvolveTests.kt index bdcde9bf64..2324930abf 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EnumEvolveTests.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/amqp/EnumEvolveTests.kt @@ -1,5 +1,6 @@ package net.corda.nodeapi.internal.serialization.amqp +import net.corda.core.internal.packageName import net.corda.core.serialization.CordaSerializationTransformEnumDefault import net.corda.core.serialization.CordaSerializationTransformEnumDefaults import net.corda.core.serialization.SerializedBytes @@ -9,6 +10,8 @@ import org.junit.Test import java.io.File import java.io.NotSerializableException import kotlin.test.assertEquals +import kotlin.test.assertNotNull +import kotlin.test.assertTrue // NOTE: To recreate the test files used by these tests uncomment the original test classes and comment // the new ones out, then change each test to write out the serialized bytes rather than read @@ -346,6 +349,9 @@ class EnumEvolveTests { Pair("$resource.5.G", MultiOperations.C)) fun load(l: List>) = l.map { + assertNotNull (EvolvabilityTests::class.java.getResource(it.first)) + assertTrue (File(EvolvabilityTests::class.java.getResource(it.first).toURI()).exists()) + Pair(DeserializationInput(sf).deserialize(SerializedBytes( File(EvolvabilityTests::class.java.getResource(it.first).toURI()).readBytes())), it.second) } diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/carpenter/CarpenterExceptionTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/carpenter/CarpenterExceptionTests.kt new file mode 100644 index 0000000000..c3ce2469a5 --- /dev/null +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/carpenter/CarpenterExceptionTests.kt @@ -0,0 +1,96 @@ +package net.corda.nodeapi.internal.serialization.carpenter + +import com.google.common.reflect.TypeToken +import junit.framework.Assert.assertTrue +import net.corda.nodeapi.internal.serialization.AllWhitelist +import net.corda.nodeapi.internal.serialization.amqp.* +import org.assertj.core.api.Assertions +import org.junit.Test +import java.io.NotSerializableException +import java.lang.reflect.Type +import java.net.URL +import kotlin.reflect.jvm.jvmName +import kotlin.test.assertEquals + +// Simple way to ensure we end up trying to carpent a class, "remove" it from the class loader (if only +// actually doing that was simple) +class TestClassLoader (private var exclude: List) : ClassLoader() { + override fun loadClass(p0: String?, p1: Boolean): Class<*> { + if (p0 in exclude) { + throw ClassNotFoundException("Pretending we can't find class $p0") + } + + return super.loadClass(p0, p1) + } +} + +interface TestInterface { + fun runThing() : Int +} + +// Create a custom serialization factory where we need to be able to both specify a carpenter +// but also have the class loader used by the carpenter be substantially different from the +// one used by the factory so as to ensure we can control their behaviour independently. +class TestFactory(override val classCarpenter: ClassCarpenter, cl: ClassLoader) + : SerializerFactory (classCarpenter.whitelist, cl) + +class CarpenterExceptionTests { + companion object { + val VERBOSE: Boolean get() = false + } + + @Test + fun checkClassComparison() { + class clA : ClassLoader() { + override fun loadClass(name: String?, resolve: Boolean): Class<*> { + return super.loadClass(name, resolve) + } + } + + class clB : ClassLoader() { + override fun loadClass(name: String?, resolve: Boolean): Class<*> { + return super.loadClass(name, resolve) + } + } + + data class A(val a: Int, val b: Int) + + val a3 = ClassLoader.getSystemClassLoader().loadClass(A::class.java.name) + val a1 = clA().loadClass(A::class.java.name) + val a2 = clB().loadClass(A::class.java.name) + + assertTrue (TypeToken.of(a1).isSubtypeOf(a2)) + assertTrue (a1 is Type) + assertTrue (a2 is Type) + assertTrue (a3 is Type) + assertEquals(a1, a2) + assertEquals(a1, a3) + assertEquals(a2, a3) + } + + @Test + fun carpenterExceptionRethrownAsNotSerializableException() { + data class C2 (val i: Int) : TestInterface { + override fun runThing() = 1 + } + + data class C1 (val i: Int, val c: C2) + + // We need two factories to ensure when we deserialize the blob we don't just use the serializer + // we built to serialise things + val ser = TestSerializationOutput(VERBOSE, testDefaultFactory()).serialize(C1(1, C2(2))) + + // Our second factory is "special" + // The class loader given to the factory rejects the outer class, this will trigger an attempt to + // carpent that class up. However, when looking at the fields specified as properties of that class + // we set the class loader of the ClassCarpenter to reject one of them, resulting in a CarpentryError + // which we then want the code to wrap in a NotSerializeableException + val cc = ClassCarpenter(TestClassLoader(listOf(C2::class.jvmName)), AllWhitelist) + val factory = TestFactory(cc, TestClassLoader(listOf(C1::class.jvmName))) + + Assertions.assertThatThrownBy { + DeserializationInput(factory).deserialize(ser) + }.isInstanceOf(NotSerializableException::class.java) + .hasMessageContaining(C2::class.java.name) + } +} \ No newline at end of file diff --git a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/KryoTests.kt b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/kryo/KryoTests.kt similarity index 99% rename from node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/KryoTests.kt rename to node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/kryo/KryoTests.kt index 7150e8c566..da045ce633 100644 --- a/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/KryoTests.kt +++ b/node-api/src/test/kotlin/net/corda/nodeapi/internal/serialization/kryo/KryoTests.kt @@ -1,4 +1,4 @@ -package net.corda.nodeapi.internal.serialization +package net.corda.nodeapi.internal.serialization.kryo import com.esotericsoftware.kryo.Kryo import com.esotericsoftware.kryo.KryoException @@ -16,7 +16,7 @@ import net.corda.core.utilities.ProgressTracker import net.corda.core.utilities.sequence import net.corda.node.serialization.KryoServerSerializationScheme import net.corda.node.services.persistence.NodeAttachmentService -import net.corda.nodeapi.internal.serialization.kryo.kryoMagic +import net.corda.nodeapi.internal.serialization.* import net.corda.testing.core.ALICE_NAME import net.corda.testing.core.TestIdentity import net.corda.testing.internal.rigorousMock