Add additional field checks for deserialization.

nfuller · andi34 · commit 95620483ee2f · 2016-06-07T20:52:22.000+02:00
Check that a field is not static when deserializing. Contains some additional tests to confirm and document behavior and prevent regressions for field deserialization. (cherry picked from commit f4d72bc) Bug: 17202597 Change-Id: I72456a8b45ca0de1d3dd2b0f9515548b02e0a7be
diff --git a/luni/src/main/java/java/io/ObjectInputStream.java b/luni/src/main/java/java/io/ObjectInputStream.java
@@ -23,7 +23,6 @@
 import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
 import java.lang.reflect.Proxy;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -1063,12 +1062,10 @@ private void readFieldValues(Object obj, ObjectStreamClass classDesc) throws Opt
         }
 
         for (ObjectStreamField fieldDesc : fields) {
-            Field field = classDesc.getReflectionField(fieldDesc);
-            if (field != null && Modifier.isTransient(field.getModifiers())) {
-                field = null; // No setting transient fields! (http://b/4471249)
-            }
-            // We may not have been able to find the field, or it may be transient, but we still
-            // need to read the value and do the other checking...
+            // checkAndGetReflectionField() can return null if it was not able to find the field or
+            // if it is transient or static. We still need to read the data and do the other
+            // checking...
+            Field field = classDesc.checkAndGetReflectionField(fieldDesc);
             try {
                 Class<?> type = fieldDesc.getTypeInternal();
                 if (type == byte.class) {
diff --git a/luni/src/main/java/java/io/ObjectOutputStream.java b/luni/src/main/java/java/io/ObjectOutputStream.java
@@ -950,9 +950,11 @@ private void writeFieldValues(Object obj, ObjectStreamClass classDesc) throws IO
         for (ObjectStreamField fieldDesc : classDesc.fields()) {
             try {
                 Class<?> type = fieldDesc.getTypeInternal();
-                Field field = classDesc.getReflectionField(fieldDesc);
+                Field field = classDesc.checkAndGetReflectionField(fieldDesc);
                 if (field == null) {
-                    throw new InvalidClassException(classDesc.getName() + " doesn't have a field " + fieldDesc.getName() + " of type " + type);
+                    throw new InvalidClassException(classDesc.getName()
+                            + " doesn't have a serializable field " + fieldDesc.getName()
+                            + " of type " + type);
                 }
                 if (type == byte.class) {
                     output.writeByte(field.getByte(obj));
@@ -1750,7 +1752,7 @@ private int writeNewEnum(Object object, Class<?> theClass, boolean unshared) thr
         // Only write field "name" for enum class, which is the second field of
         // enum, that is fields[1]. Ignore all non-fields and fields.length < 2
         if (fields != null && fields.length > 1) {
-            Field field = classDesc.getSuperclass().getReflectionField(fields[1]);
+            Field field = classDesc.getSuperclass().checkAndGetReflectionField(fields[1]);
             if (field == null) {
                 throw new NoSuchFieldError();
             }
diff --git a/luni/src/main/java/java/io/ObjectStreamClass.java b/luni/src/main/java/java/io/ObjectStreamClass.java
@@ -185,26 +185,46 @@ long getConstructor() {
         return constructor;
     }
 
-    Field getReflectionField(ObjectStreamField osf) {
+    /**
+     * Returns the {@link Field} referred to by {@link ObjectStreamField} for the class described by
+     * this {@link ObjectStreamClass}. A {@code null} value is returned if the local definition of
+     * the field does not meet the criteria for a serializable / deserializable field, i.e. the
+     * field must be non-static and non-transient. Caching of each field lookup is performed. The
+     * first time a field is returned it is made accessible with a call to
+     * {@link Field#setAccessible(boolean)}.
+     */
+    Field checkAndGetReflectionField(ObjectStreamField osf) {
         synchronized (reflectionFields) {
             Field field = reflectionFields.get(osf);
-            if (field != null) {
+            // null might indicate a cache miss or a hit and a non-serializable field so we
+            // check for a mapping.
+            if (field != null || reflectionFields.containsKey(osf)) {
                 return field;
             }
         }
 
+        Field field;
         try {
             Class<?> declaringClass = forClass();
-            Field field = declaringClass.getDeclaredField(osf.getName());
-            field.setAccessible(true);
-            synchronized (reflectionFields) {
-                reflectionFields.put(osf, field);
+            field = declaringClass.getDeclaredField(osf.getName());
+
+            int modifiers = field.getModifiers();
+            if (Modifier.isStatic(modifiers) || Modifier.isTransient(modifiers)) {
+                // No serialization or deserialization of transient or static fields!
+                // See http://b/4471249 and http://b/17202597.
+                field = null;
+            } else {
+                field.setAccessible(true);
             }
-            return reflectionFields.get(osf);
         } catch (NoSuchFieldException ex) {
             // The caller messed up. We'll return null and won't try to resolve this again.
-            return null;
+            field = null;
+        }
+
+        synchronized (reflectionFields) {
+            reflectionFields.put(osf, field);
         }
+        return field;
     }
 
     /*
diff --git a/luni/src/test/java/libcore/java/io/SerializationTest.java b/luni/src/test/java/libcore/java/io/SerializationTest.java
@@ -18,6 +18,7 @@
 
 import java.io.IOException;
 import java.io.InvalidClassException;
+import java.io.NotSerializableException;
 import java.io.ObjectStreamClass;
 import java.io.ObjectStreamField;
 import java.io.Serializable;
@@ -49,6 +50,97 @@ static class FieldMadeTransient implements Serializable {
         private int nonTransientInt;
     }
 
+    public void testSerializeFieldMadeStatic() throws Exception {
+        // Does ObjectStreamClass have the right idea?
+        ObjectStreamClass osc = ObjectStreamClass.lookup(FieldMadeStatic.class);
+        ObjectStreamField[] fields = osc.getFields();
+        assertEquals(0, fields.length);
+
+        // This was created by serializing a FieldMadeStatic with a non-static staticInt
+        String s = "aced0005737200316c6962636f72652e6a6176612e696f2e53657269616c697a6174696f6e54657"
+                + "374244669656c644d6164655374617469630000000000000000020001490009737461746963496e7"
+                + "47870000022b8";
+        FieldMadeStatic deserialized = (FieldMadeStatic) SerializationTester.deserializeHex(s);
+        // The field data is simply ignored if it is static.
+        assertEquals(9999, deserialized.staticInt);
+    }
+
+    static class FieldMadeStatic implements Serializable {
+        private static final long serialVersionUID = 0L;
+        // private int staticInt = 8888;
+        private static int staticInt = 9999;
+    }
+
+    // We can serialize an object that has an unserializable field providing it is null.
+    public void testDeserializeNullUnserializableField() throws Exception {
+        // This was created by creating a new SerializableContainer and not setting the
+        // unserializable field. A canned serialized form is used so we can tell if the static
+        // initializers were executed during deserialization.
+        // SerializationTester.serializeHex(new SerializableContainer());
+        String s = "aced0005737200376c6962636f72652e6a6176612e696f2e53657269616c697a6174696f6e54657"
+                + "3742453657269616c697a61626c65436f6e7461696e657200000000000000000200014c000e756e7"
+                + "3657269616c697a61626c657400334c6c6962636f72652f6a6176612f696f2f53657269616c697a6"
+                + "174696f6e546573742457617353657269616c697a61626c653b787070";
+
+        serializableContainerInitializedFlag = false;
+        wasSerializableInitializedFlag = false;
+
+        SerializableContainer sc = (SerializableContainer) SerializationTester.deserializeHex(s);
+        assertNull(sc.unserializable);
+
+        // Confirm the container was initialized, but the class for the null field was not.
+        assertTrue(serializableContainerInitializedFlag);
+        assertFalse(wasSerializableInitializedFlag);
+    }
+
+    public static boolean serializableContainerInitializedFlag = false;
+
+    static class SerializableContainer implements Serializable {
+        private static final long serialVersionUID = 0L;
+        private Object unserializable = null;
+
+        static {
+            serializableContainerInitializedFlag = true;
+        }
+    }
+
+    // We must not serialize an object that has a non-null unserializable field.
+    public void testSerializeUnserializableField() throws Exception {
+        SerializableContainer sc = new SerializableContainer();
+        sc.unserializable = new WasSerializable();
+        try {
+            SerializationTester.serializeHex(sc);
+            fail();
+        } catch (NotSerializableException expected) {
+        }
+    }
+
+    // It must not be possible to deserialize an object if a field is no longer serializable.
+    public void testDeserializeUnserializableField() throws Exception {
+        // This was generated by creating a SerializableContainer and setting the unserializable
+        // field to a WasSerializable when it was still Serializable. A canned serialized form is
+        // used so we can tell if the static initializers were executed during deserialization.
+        // SerializableContainer sc = new SerializableContainer();
+        // sc.unserializable = new WasSerializable();
+        // SerializationTester.serializeHex(sc);
+        String s = "aced0005737200376c6962636f72652e6a6176612e696f2e53657269616c697a6174696f6e54657"
+                + "3742453657269616c697a61626c65436f6e7461696e657200000000000000000200014c000e756e7"
+                + "3657269616c697a61626c657400124c6a6176612f6c616e672f4f626a6563743b7870737200316c6"
+                + "962636f72652e6a6176612e696f2e53657269616c697a6174696f6e5465737424576173536572696"
+                + "16c697a61626c65000000000000000002000149000169787000000000";
+
+        serializableContainerInitializedFlag = false;
+        wasSerializableInitializedFlag = false;
+        try {
+            SerializationTester.deserializeHex(s);
+            fail();
+        } catch (InvalidClassException expected) {
+        }
+        // Confirm neither the container nor the contained class was initialized.
+        assertFalse(serializableContainerInitializedFlag);
+        assertFalse(wasSerializableInitializedFlag);
+    }
+
     public void testSerialVersionUidChange() throws Exception {
         // this was created by serializing a SerialVersionUidChanged with serialVersionUID = 0L
         String s = "aced0005737200396c6962636f72652e6a6176612e696f2e53657269616c697a6174696f6e54657"
