Re: [cp-patches] New serialization fix

[Guilhem Lavaux]

Jeroen Frijters wrote:
> Guilhem Lavaux wrote:
>
> > It appears that GNU Classpath's ObjectInputStream is not sufficiently 
> > strict.
>
>
> I agree.
>
>
> > Here is a patch to fix it in pure java.
>
>
> It's not a good idea to re-introduce reflection into the per-object path
> of serialization. That slows it down enormously.

Yes, I agree. I was just proposing the least modification patch. ;) 
Actually I would have done all in native but JNI is not handy to just 
check a modifier.

> I'd like to propose something like the attached patch. It removes the
> need to the constructor lookuk for every object.
>
> The patch doesn't include the native side, because I haven't written
> that (don't need it for IKVM) but I'm happy to write it if someone else
> will test it.

I've written the native part. However to restrict the use of JNI code I 
have added a new parameter which also passes the declaring class of the 
constructor to the native function. It is used to call 
CallNonvirtualMethodXX. Here is the new complete patch.

Thanks,

Regards,

Guilhem.

Index: java/io/ObjectInputStream.java
===================================================================
RCS file: /cvsroot/classpath/classpath/java/io/ObjectInputStream.java,v
retrieving revision 1.48
diff -u -r1.48 ObjectInputStream.java
--- java/io/ObjectInputStream.java      6 Nov 2004 14:58:49 -0000       1.48
+++ java/io/ObjectInputStream.java      4 Dec 2004 14:47:09 -0000
@@ -42,11 +42,14 @@
 import gnu.java.io.ObjectIdentityWrapper;
 
 import java.lang.reflect.Array;
+import java.lang.reflect.Constructor;
 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.security.AccessController;
+import java.security.PrivilegedAction;
 import java.util.Arrays;
 import java.util.Hashtable;
 import java.util.Vector;
@@ -296,7 +299,7 @@
                  break;
                } // end if (osc.realClassIsExternalizable)
 
-             Object obj = newObject(clazz, osc.firstNonSerializableParent);
+             Object obj = newObject(clazz, 
osc.firstNonSerializableParentConstructor);
              
              int handle = assignNewHandle(obj);
              Object prevObject = this.currentObject;
@@ -532,7 +535,29 @@
        || Modifier.isAbstract(first_nonserial.getModifiers()))
        first_nonserial = first_nonserial.getSuperclass();
 
-    osc.firstNonSerializableParent = first_nonserial;
+    final Class local_constructor_class = first_nonserial;
+
+    osc.firstNonSerializableParentConstructor =
+        (Constructor)AccessController.doPrivileged(new PrivilegedAction()
+          {
+            public Object run()
+            {
+              try
+                {
+                  Constructor c = local_constructor_class.
+                                    getDeclaredConstructor(new Class[0]);
+                  if (Modifier.isPrivate(c.getModifiers()))
+                    return null;
+                  return c;
+                }
+              catch (NoSuchMethodException e)
+                {
+                  // error will be reported later, in newObject()
+                  return null;
+                }
+            }
+          });
+
     osc.realClassIsSerializable = Serializable.class.isAssignableFrom(clazz);
     osc.realClassIsExternalizable = 
Externalizable.class.isAssignableFrom(clazz);
 
@@ -1753,14 +1778,14 @@
 
   // returns a new instance of REAL_CLASS that has been constructed
   // only to the level of CONSTRUCTOR_CLASS (a super class of REAL_CLASS)
-  private Object newObject (Class real_class, Class constructor_class)
-    throws ClassNotFoundException
+  private Object newObject (Class real_class, Constructor constructor)
+    throws ClassNotFoundException, IOException
   {
+    if (constructor == null)
+        throw new InvalidClassException("Missing accessible no-arg base class 
constructor for " + real_class.getName()); 
     try
       {
-       Object obj = allocateObject (real_class);
-       callConstructor (constructor_class, obj);
-       return obj;
+       return allocateObject(real_class, constructor.getDeclaringClass(), 
constructor);
       }
     catch (InstantiationException e)
       {
@@ -1828,11 +1853,9 @@
     prereadFields = null;
   }
     
-  private native Object allocateObject (Class clazz)
+  private native Object allocateObject(Class clazz, Class constr_clazz, 
Constructor constructor)
     throws InstantiationException;
 
-  private native void callConstructor (Class clazz, Object obj);
-
   private static final int BUFFER_SIZE = 1024;
 
   private DataInputStream realInputStream;
@@ -1876,7 +1899,7 @@
   {
     if (Configuration.INIT_LOAD_LIBRARY)
       {
-       System.loadLibrary ("javaio");
+       System.loadLibrary ("io");
       }
   }
 }
Index: java/io/ObjectStreamClass.java
===================================================================
RCS file: /cvsroot/classpath/classpath/java/io/ObjectStreamClass.java,v
retrieving revision 1.37
diff -u -r1.37 ObjectStreamClass.java
--- java/io/ObjectStreamClass.java      2 Dec 2004 13:17:21 -0000       1.37
+++ java/io/ObjectStreamClass.java      4 Dec 2004 14:47:09 -0000
@@ -850,11 +850,11 @@
     {
        return (Externalizable)constructor.newInstance(null);
     }
-    catch(Exception x)
+    catch(Throwable t)
     {
        throw (InvalidClassException)
            new InvalidClassException(clazz.getName(),
-                    "Unable to instantiate").initCause(x);
+                    "Unable to instantiate").initCause(t);
     }
   }
 
@@ -888,7 +888,7 @@
   boolean realClassIsSerializable;
   boolean realClassIsExternalizable;
   ObjectStreamField[] fieldMapping;
-  Class firstNonSerializableParent;
+  Constructor firstNonSerializableParentConstructor;
   private Constructor constructor;  // default constructor for Externalizable
 
   boolean isProxyClass = false;
Index: native/jni/java-io/java_io_ObjectInputStream.c
===================================================================
RCS file: 
/cvsroot/classpath/classpath/native/jni/java-io/java_io_ObjectInputStream.c,v
retrieving revision 1.9
diff -u -r1.9 java_io_ObjectInputStream.c
--- native/jni/java-io/java_io_ObjectInputStream.c      26 Oct 2004 20:26:03 
-0000      1.9
+++ native/jni/java-io/java_io_ObjectInputStream.c      4 Dec 2004 14:47:11 
-0000
@@ -78,28 +78,14 @@
 Java_java_io_ObjectInputStream_allocateObject( JNIEnv * env,
                                               jobject self
                                               __attribute__ ((__unused__)),
-                                              jclass clazz )
+                                              jclass clazz,
+                                              jclass constr_clazz,
+                                              jobject constructor)
 {
-  return (*env)->AllocObject( env, clazz );
-}
+  jobject obj = (*env)->AllocObject( env, clazz );
+  jmethodID id = (*env)->FromReflectedMethod( env, constructor);
 
+  (*env)->CallNonvirtualVoidMethod( env, obj, constr_clazz, id);
 
-/*
- * Class:     java_io_ObjectInputStream
- * Method:    callConstructor
- * Signature: (Ljava/lang/Class;Ljava/lang/Object;)V
- */
-JNIEXPORT void JNICALL 
-Java_java_io_ObjectInputStream_callConstructor( JNIEnv * env,
-                                               jclass clazz
-                                               __attribute__ ((__unused__)),
-                                               jclass constr_class,
-                                               jobject obj )
-{
-  jmethodID id = (*env)->GetMethodID( env, constr_class,
-                                     "<init>", "()V" );
-  if( id == NULL )
-    return;
-  
-  (*env)->CallNonvirtualVoidMethod( env, obj, constr_class, id );
+  return obj;
 }