MINA
  1. MINA
  2. DIRMINA-822

Deserialising classes that do not implement Serializable fails

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.2
    • Fix Version/s: 2.0.3
    • Component/s: Core
    • Labels:
      None

      Description

      AbstractIoBuffer uses ObjectStreamClass.lookup(Class) which returns null for classes that do not implement Serializable. This in turn leads to a NullPointerException a few lines below in resolveClass(ObjectStreamClass) where the ObjectStreamClass-parameter is null.
      Deserialising a non-serialisable class is completely legal, instances of such a class will already fail to serialise so they will never get to deserialisation.

      Using ObjectStreamClass.lookupAny(Class) will solve this issue, but it is not available before Java 6.

        Issue Links

          Activity

          Hide
          Emmanuel Lecharny added a comment -

          Yeah, lookupAny is a Java 6 method, and the lookup( Class, boolean ) is package protected... Seems like a dead end here.

          What I don't get is that if the class is not Serializable, it would not be part of the received message, isn't it ?

          I have modified the current code this way :

          case 1: // Non-primitive types
          String className = readUTF();
          Class<?> clazz = Class.forName(className, true,
          classLoader);
          ObjectStreamClass osClass = ObjectStreamClass.lookup(clazz);

          if (osClass == null)

          { throw new ClassNotFoundException("The '" + className + "' class " + "can't be read, it's not implementing the Serializable interface"); }

          return osClass;

          The idea is to throw an exception instead of simply return 'null', helping the user to understand why he has an issue. Is that enough ?

          Show
          Emmanuel Lecharny added a comment - Yeah, lookupAny is a Java 6 method, and the lookup( Class, boolean ) is package protected... Seems like a dead end here. What I don't get is that if the class is not Serializable, it would not be part of the received message, isn't it ? I have modified the current code this way : case 1: // Non-primitive types String className = readUTF(); Class<?> clazz = Class.forName(className, true, classLoader); ObjectStreamClass osClass = ObjectStreamClass.lookup(clazz); if (osClass == null) { throw new ClassNotFoundException("The '" + className + "' class " + "can't be read, it's not implementing the Serializable interface"); } return osClass; The idea is to throw an exception instead of simply return 'null', helping the user to understand why he has an issue. Is that enough ?
          Hide
          Ulrich Kreher added a comment -

          I am afraid, that is not enough. However, it is much better than getting an NPE in resolveClass.

          My first example was a bit too simple. In our scenario we have our own remote method invocation mechanism. This allows us to very easily change the used transport and various parameters (TCP, SSL, XML-encoded, HTTP or even using RMI itself internally). We have a class similar to this:

          public class RemoteMethodInv
          {
          static interface NonSerialisable
          { }

          String methodName;
          Class[] paramTypes;
          Object[] paramValues;
          }

          Now if we want to call a method remotely which has a parameter of type NonSerialisabe, the invocation will always fail with Mina 2. But such a remote call is completely legal, as long as the corresponding parameter value is serialisable. Even using null as value fails, and null is very well serialisable.

          I am not sure why Mina 2 uses an own (anonymous) ObjectInputStream at all, while Mina 1.1.7 does not. Maybe one can adapt the anonymous input stream to use more of its superclass. The default java.io.ObjectInputStream does not have this problem, which is no surprise since it may call lookup(Class, boolean).

          Show
          Ulrich Kreher added a comment - I am afraid, that is not enough. However, it is much better than getting an NPE in resolveClass. My first example was a bit too simple. In our scenario we have our own remote method invocation mechanism. This allows us to very easily change the used transport and various parameters (TCP, SSL, XML-encoded, HTTP or even using RMI itself internally). We have a class similar to this: public class RemoteMethodInv { static interface NonSerialisable { } String methodName; Class[] paramTypes; Object[] paramValues; } Now if we want to call a method remotely which has a parameter of type NonSerialisabe, the invocation will always fail with Mina 2. But such a remote call is completely legal, as long as the corresponding parameter value is serialisable. Even using null as value fails, and null is very well serialisable. I am not sure why Mina 2 uses an own (anonymous) ObjectInputStream at all, while Mina 1.1.7 does not. Maybe one can adapt the anonymous input stream to use more of its superclass. The default java.io.ObjectInputStream does not have this problem, which is no surprise since it may call lookup(Class, boolean).
          Hide
          Emmanuel Lecharny added a comment -

          Mina 1.1.7 uses the very same construction : http://svn.apache.org/viewvc/mina/tags/1.1.7/core/src/main/java/org/apache/mina/common/ByteBuffer.java?view=markup

          i'm still trying to get a clue about this part of the code and the rational behind it...

          Show
          Emmanuel Lecharny added a comment - Mina 1.1.7 uses the very same construction : http://svn.apache.org/viewvc/mina/tags/1.1.7/core/src/main/java/org/apache/mina/common/ByteBuffer.java?view=markup i'm still trying to get a clue about this part of the code and the rational behind it...
          Hide
          Ulrich Kreher added a comment -

          You are right, Mina 1.1.7 uses the same construction. I have to confess that we patched this class quite some time ago by using ObjectInputStream and ObjectOutputStream directly. For us this works so good, that we forgot about the patch. I will try this with Mina 2 now and report any problem. However, this might not be the final solution.

          Show
          Ulrich Kreher added a comment - You are right, Mina 1.1.7 uses the same construction. I have to confess that we patched this class quite some time ago by using ObjectInputStream and ObjectOutputStream directly. For us this works so good, that we forgot about the patch. I will try this with Mina 2 now and report any problem. However, this might not be the final solution.
          Hide
          Emmanuel Lecharny added a comment -

          Any patch will be very welcomed. We can even build a version you can test if you like.

          Show
          Emmanuel Lecharny added a comment - Any patch will be very welcomed. We can even build a version you can test if you like.
          Hide
          Ulrich Kreher added a comment -

          A rather simple patch but it works at least for our scenario.

          Show
          Ulrich Kreher added a comment - A rather simple patch but it works at least for our scenario.
          Hide
          Rob Eden added a comment -

          I agree with Ulrich's patch. There's no explanation of why OOS/OIS are being overridden the way they are (original SVN change is 597545: http://svn.apache.org/viewvc/mina/trunk/mina-core/src/main/java/org/apache/mina/core/buffer/AbstractIoBuffer.java?r1=594412&r2=597545) and from all I can see they're just creating issues.

          If backwards compatibility of the protocol is desired, it can be changed to always write "0" and then leave the reading portion as-is.

          Show
          Rob Eden added a comment - I agree with Ulrich's patch. There's no explanation of why OOS/OIS are being overridden the way they are (original SVN change is 597545: http://svn.apache.org/viewvc/mina/trunk/mina-core/src/main/java/org/apache/mina/core/buffer/AbstractIoBuffer.java?r1=594412&r2=597545 ) and from all I can see they're just creating issues. If backwards compatibility of the protocol is desired, it can be changed to always write "0" and then leave the reading portion as-is.
          Hide
          Emmanuel Lecharny added a comment -

          Seems that this is not the first time this issue has been encountered. Probably time to fix it now...

          Show
          Emmanuel Lecharny added a comment - Seems that this is not the first time this issue has been encountered. Probably time to fix it now...
          Hide
          Emmanuel Lecharny added a comment -

          Ok, someone already proposed a patch 3 years ago, but it was for MINA 1.1.7, which is considered as dead wood, thus it was never fixed in MINA 2 too.

          I have applied the patch provided in DIRMINA-627 : http://svn.apache.org/viewvc?rev=1082736&view=rev

          I'll build some jars and put them on my home page for those who want to test it.

          Show
          Emmanuel Lecharny added a comment - Ok, someone already proposed a patch 3 years ago, but it was for MINA 1.1.7, which is considered as dead wood, thus it was never fixed in MINA 2 too. I have applied the patch provided in DIRMINA-627 : http://svn.apache.org/viewvc?rev=1082736&view=rev I'll build some jars and put them on my home page for those who want to test it.
          Hide
          Emmanuel Lecharny added a comment -

          Available here for testing : http://people.apache.org/~elecharny/

          Please tell me if it's ok, so that I can close the issue and start a vote for a new release asap.

          Thanks !

          Show
          Emmanuel Lecharny added a comment - Available here for testing : http://people.apache.org/~elecharny/ Please tell me if it's ok, so that I can close the issue and start a vote for a new release asap. Thanks !
          Hide
          Ulrich Kreher added a comment -

          I did not try your new patched version yet. But as stated above, we use the very same patch in Mina 1.1.7 in our product. We applied the patch in november 2008 and did not have any problems with it yet. Sorry for the late reporting.

          Show
          Ulrich Kreher added a comment - I did not try your new patched version yet. But as stated above, we use the very same patch in Mina 1.1.7 in our product. We applied the patch in november 2008 and did not have any problems with it yet. Sorry for the late reporting.
          Hide
          Emmanuel Lecharny added a comment -

          Patch applied

          Show
          Emmanuel Lecharny added a comment - Patch applied

            People

            • Assignee:
              Unassigned
              Reporter:
              Ulrich Kreher
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development