Workaround bugs in Proxy serialization

Workaround bugs in Proxy serialization

[Andrew Haley]

Two bugs when serializing proxies.

Firstly, we were assigning the handles for the proxyClassDescInfo and
its associated classAnnotation in the wrong order.  The grammar for
newClassDesc makes it clear that the handle is allocated before the
classAnnotation is read.

The much harder problem has to do with reading a proxy class from a
stream and then writing it to another stream.  

When reading a proxy, we generate an ObjectStreamClass by calling
lookupClass(), and then we read the ObjectStreamClass for the proxy's
superclass (which is always Proxy) from the stream.  We then set the
proxy's ObjectStreamClass's superclass.  This has the effect that the
ObjectStreamClass for the proxy class is cached in the global cache.

Later, someone comes to write an instance of the same proxy class.
They find the ObjectStreamClass for that proxy in the global cache,
but the ObjectStreamClass for the proxy's superclass has only been
created by reading it from the input stream, so it has never has its
fields set.  Writing the Proxy fails because of a
NullPointerException.

Fixing this properly seems to be incredibly hard.  I've spent the last
couple of days trying all the obvious fixes, and they all cause
breakages somewhere else.  So, I've written a workaround: if we are
writing an object's fields, first make sure we've called setFields on
the ObjectStreamClass.  This is safe, because it only has any effect
in a case where we would otherwise have crashed.

If anyone wants to fix the real bug, please do, but IMO the only cure
for our serialization code is to replace it altogether.

Andrew.



 
 2007-04-25  Andrew Haley  <aph@redhat.com>

	* java/io/ObjectStreamClass.java (ensureFieldsSet): New method.
	(setFields): call ensureFieldsSet.
	(fieldsSet): New field.
	* java/io/ObjectOutputStream.java (writeFields): Call osc.ensureFieldsSet().
	* java/io/ObjectInputStream.java (parseContent): Assign the handle
	for a PROXYCLASSDESC immediately after reading the marker.

Index: ObjectInputStream.java
===================================================================
--- ObjectInputStream.java	(revision 123952)
+++ ObjectInputStream.java	(working copy)
@@ -222,7 +222,14 @@
  	
        case TC_PROXYCLASSDESC:
  	{
- 	  if(dump) dumpElementln("PROXYCLASS");
+ 	  if(dump) dumpElementln("PROXYCLASSDESC");
+
+	  // The grammar at this point is
+	  //   TC_PROXYCLASSDESC newHandle proxyClassDescInfo
+	  // i.e. we have to assign the handle immediately after
+	  // reading the marker.
+ 	  int handle = assignNewHandle("Dummy proxy");
+
  	  int n_intf = this.realInputStream.readInt();
  	  String[] intfs = new String[n_intf];
  	  for (int i = 0; i < n_intf; i++)
@@ -250,7 +257,7 @@
                     new InternalError("Object ctor missing").initCause(x);
                 }
             }
- 	  assignNewHandle(osc);
+	  rememberHandle(osc,handle);
  	  
  	  if (!is_consumed)
  	    {
@@ -1986,6 +1993,8 @@
 
   private void dumpElementln (String msg, Object obj)
   {
+    if (obj == null)
+      obj = "(null)";
     try
       {
 	System.out.print(msg);
Index: ObjectOutputStream.java
===================================================================
--- ObjectOutputStream.java	(revision 123952)
+++ ObjectOutputStream.java	(working copy)
@@ -1212,10 +1212,14 @@
 
 
   // writes out FIELDS of OBJECT for the specified ObjectStreamClass.
-  // FIELDS are already in canonical order.
+  // FIELDS are already supposed already to be in canonical order, but
+  // under some circumstances (to do with Proxies) this isn't the
+  // case, so we call ensureFieldsSet().
   private void writeFields(Object obj, ObjectStreamClass osc)
     throws IOException
   {
+    osc.ensureFieldsSet(osc.forClass());
+
     ObjectStreamField[] fields = osc.fields;
     boolean oldmode = setBlockDataMode(false);
 
@@ -1348,6 +1352,8 @@
 	  System.out.print (" ");
 	System.out.print (Thread.currentThread() + ": ");
 	System.out.print (msg);
+	if (obj == null)
+	  obj = "(null)";
 	if (java.lang.reflect.Proxy.isProxyClass(obj.getClass()))
 	  System.out.print (obj.getClass());
 	else
Index: ObjectStreamClass.java
===================================================================
--- ObjectStreamClass.java	(revision 123952)
+++ ObjectStreamClass.java	(working copy)
@@ -654,11 +654,25 @@
       flags |= ObjectStreamConstants.SC_ENUM;
   }
 
+  // FIXME: This is a workaround for a fairly obscure bug that happens
+  // when reading a Proxy and then writing it back out again.  The
+  // result is that the ObjectStreamClass doesn't have its fields set,
+  // generating a NullPointerException.  Rather than this kludge we
+  // should probably fix the real bug, but it would require a fairly
+  // radical reorganization to do so.
+  final void ensureFieldsSet(Class cl)
+  {
+    if (! fieldsSet)
+      setFields(cl);
+  }
+
 
   // Sets fields to be a sorted array of the serializable fields of
   // clazz.
   private void setFields(Class cl)
   {
+    fieldsSet = true;
+
     SetAccessibleAction setAccessible = new SetAccessibleAction();
 
     if (!isSerializable() || isExternalizable() || isEnum())
@@ -1094,6 +1108,9 @@
 
   boolean isProxyClass = false;
 
+  // True after setFields() has been called
+  private boolean fieldsSet = false;
+
   // This is probably not necessary because this class is special cased already
   // but it will avoid showing up as a discrepancy when comparing SUIDs.
   private static final long serialVersionUID = -6120832682080437368L;

Re: Workaround bugs in Proxy serialization

[Andrew Haley]

Looking at the Classpath sources for ObjectInputStream, it seems that
the area where I had so much trouble has been reworked.  I'll
investigate importing those classes into libgcj.

Andrew.

Re: Workaround bugs in Proxy serialization

[Andrew Haley]

Andrew Haley writes:
 > Looking at the Classpath sources for ObjectInputStream, it seems that
 > the area where I had so much trouble has been reworked.  I'll
 > investigate importing those classes into libgcj.

I've thought about this some more, and come to the conclusion that the
best thing is to check this workaround into libgcj as gcj local and
re-test the next time a Classpath merge is done.

Andrew.

Re: Workaround bugs in Proxy serialization

[Andrew Haley]

For those of you that may be wondering what this was all about, there
is an excellent RMI tutoral at http://java.sun.com/docs/books/tutorial/rmi/TOC.html

This example worked with gcj as long as Sun's rmiregistry was being
sued, but not with grmiregistry, which segafulted.  The changes I made
in gcj's serialization code mean that we have interoperability between
Sun's RMI and gcj's, in both client-server directions.

I haven't tried anything more elaborate than this simple test, though.

Andrew.