6966692: defaultReadObject can set a field multiple times

Reviewed-by: hawtin

src/share/classes/java/io/ObjectStreamClass.java

 /*
- * Copyright (c) 1996, 2008, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2010, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -1830,8 +1830,10 @@ public class ObjectStreamClass implements Serializable {
         private final ObjectStreamField[] fields;
         /** number of primitive fields */
         private final int numPrimFields;
-        /** unsafe field keys */
-        private final long[] keys;
+        /** unsafe field keys for reading fields - may contain dupes */
+        private final long[] readKeys;
+        /** unsafe fields keys for writing fields - no dupes */
+        private final long[] writeKeys;
         /** field data offsets */
         private final int[] offsets;
         /** field type codes */
@@ -1849,16 +1851,22 @@ public class ObjectStreamClass implements Serializable {
         FieldReflector(ObjectStreamField[] fields) {
             this.fields = fields;
             int nfields = fields.length;
-            keys = new long[nfields];
+            readKeys = new long[nfields];
+            writeKeys = new long[nfields];
             offsets = new int[nfields];
             typeCodes = new char[nfields];
             ArrayList<Class<?>> typeList = new ArrayList<Class<?>>();
+            Set<Long> usedKeys = new HashSet<Long>();
+
 
             for (int i = 0; i < nfields; i++) {
                 ObjectStreamField f = fields[i];
                 Field rf = f.getField();
-                keys[i] = (rf != null) ?
+                long key = (rf != null) ?
                     unsafe.objectFieldOffset(rf) : Unsafe.INVALID_FIELD_OFFSET;
+                readKeys[i] = key;
+                writeKeys[i] = usedKeys.add(key) ?
+                    key : Unsafe.INVALID_FIELD_OFFSET;
                 offsets[i] = f.getOffset();
                 typeCodes[i] = f.getTypeCode();
                 if (!f.isPrimitive()) {
@@ -1894,7 +1902,7 @@ public class ObjectStreamClass implements Serializable {
              * in array should be equal to Unsafe.INVALID_FIELD_OFFSET.
              */
             for (int i = 0; i < numPrimFields; i++) {
-                long key = keys[i];
+                long key = readKeys[i];
                 int off = offsets[i];
                 switch (typeCodes[i]) {
                     case 'Z':
@@ -1945,7 +1953,7 @@ public class ObjectStreamClass implements Serializable {
                 throw new NullPointerException();
             }
             for (int i = 0; i < numPrimFields; i++) {
-                long key = keys[i];
+                long key = writeKeys[i];
                 if (key == Unsafe.INVALID_FIELD_OFFSET) {
                     continue;           // discard value
                 }
@@ -2006,7 +2014,7 @@ public class ObjectStreamClass implements Serializable {
                 switch (typeCodes[i]) {
                     case 'L':
                     case '[':
-                        vals[offsets[i]] = unsafe.getObject(obj, keys[i]);
+                        vals[offsets[i]] = unsafe.getObject(obj, readKeys[i]);
                         break;
 
                     default:
@@ -2027,7 +2035,7 @@ public class ObjectStreamClass implements Serializable {
                 throw new NullPointerException();
             }
             for (int i = numPrimFields; i < fields.length; i++) {
-                long key = keys[i];
+                long key = writeKeys[i];
                 if (key == Unsafe.INVALID_FIELD_OFFSET) {
                     continue;           // discard value
                 }

test/java/io/Serializable/6966692/Attack.java

+/*
+ * @test
+ * @bug 6966692
+ * @summary  defaultReadObject can set a field multiple times
+ * @run shell Test6966692.sh
+*/
+
+import java.io.*;
+
+public class Attack {
+    public static void main(String[] args) throws Exception {
+        attack(setup());
+    }
+    /** Returned data has Victim with two aaaa fields. */
+    private static byte[] setup() throws Exception {
+        Victim victim = new Victim();
+        ByteArrayOutputStream byteOut = new ByteArrayOutputStream();
+        ObjectOutputStream out = new ObjectOutputStream(byteOut);
+        out.writeObject(victim);
+        out.close();
+        byte[] data = byteOut.toByteArray();
+        String str = new String(data, 0); // hibyte is 0
+        str = str.replaceAll("bbbb", "aaaa");
+        str.getBytes(0, data.length, data, 0); // ignore hibyte
+        return data;
+    }
+    private static void attack(byte[] data) throws Exception {
+        ObjectInputStream in = new ObjectInputStream(
+            new ByteArrayInputStream(data)
+        );
+        Victim victim = (Victim)in.readObject();
+        System.out.println(victim+" "+victim.aaaa);
+    }
+}

test/java/io/Serializable/6966692/README

+Testcase shows default deserialisation of the Victim having two values for the same field. 
+
+Probably requires dual core to run successfully. 
+
+Reading thread is warmed up so that it can easily win the race for the demonstration, but this means we need to make the field volatile.
+
+Typical output:
+
+Victim@1551f60 BBBB
+Victim@1551f60 AAAA
+
+The output when its fixed is, 
+Victim@1975b59 AAAA
+Victim@1975b59 AAAA  - The value is retained
+
+and when it is not fixed, it shows something like 
+Victim@173a10f AAAA
+Victim@173a10f BBBB - the value of the object gets set again and hence is different. This is a bug
+
+Look at the 
+AAAA AAAA 
+and 
+AAAA BBBB

test/java/io/Serializable/6966692/Test6966692.sh

+#!/bin/sh
+
+if [ "${TESTSRC}" = "" ]
+then TESTSRC=.
+fi
+
+if [ "${TESTJAVA}" = "" ]
+then
+  PARENT=`dirname \`which java\``
+  TESTJAVA=`dirname ${PARENT}`
+  echo "TESTJAVA not set, selecting " ${TESTJAVA}
+  echo "If this is incorrect, try setting the variable manually."
+fi
+
+if [ "${TESTCLASSES}" = "" ]
+then
+  echo "TESTCLASSES not set.  Test cannot execute.  Failed."
+  exit 1
+fi
+
+BIT_FLAG=""
+
+# set platform-dependent variables
+OS=`uname -s`
+case "$OS" in
+  SunOS | Linux )
+    NULL=/dev/null
+    PS=":"
+    FS="/"
+    ## for solaris, linux it's HOME
+    FILE_LOCATION=$HOME
+    if [ -f ${FILE_LOCATION}${FS}JDK64BIT -a ${OS} = "SunOS" ]
+    then
+        BIT_FLAG=`cat ${FILE_LOCATION}${FS}JDK64BIT | grep -v '^#'`
+    fi
+    ;;
+  Windows_* )
+    NULL=NUL
+    PS=";"
+    FS="\\"
+    ;;
+  * )
+    echo "Unrecognized system!"
+    exit 1;
+    ;;
+esac
+
+JEMMYPATH=${CPAPPEND}
+CLASSPATH=.${PS}${TESTCLASSES}${PS}${JEMMYPATH} ; export CLASSPATH
+
+THIS_DIR=`pwd`
+
+${TESTJAVA}${FS}bin${FS}java ${BIT_FLAG} -version
+
+cp ${TESTSRC}${FS}*.java .
+chmod 777 *.java
+
+${TESTJAVA}${FS}bin${FS}javac *.java
+
+${TESTJAVA}${FS}bin${FS}java ${BIT_FLAG} Attack > test.out 2>&1
+
+cat test.out
+
+STATUS=0
+
+egrep "^Victim.*BBBB.*AAAA|^Victim.*AAAA.*BBBB" test.out
+
+if [ $? = 0 ]
+then
+    STATUS=1
+else
+    egrep "^Victim.*BBBB.*BBBB|^Victim.*AAAA.*AAAA" test.out
+
+    if [ $? != 0 ]; then
+        STATUS=1
+    fi
+fi
+
+exit $STATUS

test/java/io/Serializable/6966692/Victim.java

+import java.io.*;
+
+public class Victim implements Serializable {
+    public volatile Object aaaa = "AAAA"; // must be volatile...
+    private final Object aabb = new Show(this);
+    public Object bbbb = "BBBB";
+}
+class Show implements Serializable {
+    private final Victim victim;
+    public Show(Victim victim) {
+        this.victim = victim;
+    }
+    private void readObject(java.io.ObjectInputStream in)
+     throws IOException, ClassNotFoundException
+    {
+        in.defaultReadObject();
+        Thread thread = new Thread(new Runnable() { public void run() {
+            for (;;) {
+                Object a = victim.aaaa;
+                if (a != null) {
+                    System.err.println(victim+" "+a);
+                    break;
+                }
+            }
+        }});
+        thread.start();
+
+        // Make sure we are running compiled whilst serialisation is done interpreted.
+        try {
+            Thread.sleep(1000);
+        } catch (java.lang.InterruptedException exc) {
+            Thread.currentThread().interrupt();
+        }
+    }
+}