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

@@ -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``.

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

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<SerializerFactory>().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)
         }
     }

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 {

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)
+                }
             }
         }
     }

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<out Any>) : 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}")

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)
+            }
         }
     }
 }

node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/carpenter/SchemaFields.kt

@@ -98,8 +98,7 @@ class NullableField(field: Class<out Any?>) : 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)
         }
     }
 

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<Pair<String, MultiOperations>>) = l.map {
+            assertNotNull (EvolvabilityTests::class.java.getResource(it.first))
+            assertTrue (File(EvolvabilityTests::class.java.getResource(it.first).toURI()).exists())
+
             Pair(DeserializationInput(sf).deserialize(SerializedBytes<C>(
                     File(EvolvabilityTests::class.java.getResource(it.first).toURI()).readBytes())), it.second)
         }

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<String>) : 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)
+    }
+}

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