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

@@ -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 <https://docs.oracle.com/javase/tutorial/java/javaOO/nested.html>`_
+  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)

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")

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,7 +57,16 @@ 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
@@ -136,7 +144,13 @@ open class ObjectSerializer(val clazz: Type, factory: SerializerFactory) : AMQPS
     fun construct(properties: List<Any?>): 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.")
     }
 }

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<Any>? {
         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<out Any?>.propertyDescriptors(): Map<String, PropertyDescriptor> {
             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 <T : Any> 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<PropertyAccessor>().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

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<AbstractParty> 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<AbstractParty> 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<AbstractParty> 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<AbstractParty> 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");
+    }
+}
+

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<AbstractParty> getParticipants() {
+            return ImmutableList.of();
+        }
+    }
+}
+
+class Wrapper {
+    private ContractState cs;
+    Wrapper(ContractState cs) { this.cs = cs; }
+}
+
+class TemplateWrapper<T> {
+    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<ContractState> (new DummyState()))).isInstanceOf(
+                NotSerializableException.class).hasMessageContaining(
+                "has synthetic fields and is likely a nested inner class");
+    }
+}

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<AbstractParty> 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());
-    }
 }

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