Harmony
  1. Harmony
  2. HARMONY-5876

[classlib][nio] NIO native corrupting Long.valueOf(0)

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 5.0M6
    • Fix Version/s: None
    • Component/s: Classlib
    • Labels:
      None
    • Environment:
      Windows x86-32 for sure, probably others.

      Description

      The following test case:

      import java.net.*;
      import java.nio.channels.*;
      public class SocketChannelTest {
      public static void main(String args[]) throws Exception

      { SocketChannel socketChannel = SocketChannel.open(); socketChannel.configureBlocking(false); InetSocketAddress addr = new InetSocketAddress("127.0.0.1", 6789); System.out.println(new Long(0)); System.out.println(Long.valueOf(0)); socketChannel.connect(addr); System.out.println(new Long(0)); System.out.println(Long.valueOf(0)); }

      }

      will print the following output on Harmony M6:
      0
      0
      0
      598759496

      On Sun JDK 1.5.0_14, it prints the expected value:
      0
      0
      0
      0

      I suspect that the problem is in the NIO native code: in particular, OSNetworkSystem.c does the following:
      void
      setConnectContext(JNIEnv *env,jobject longclass,U_8 * context){
      jclass descriptorCLS;
      jfieldID descriptorFID;
      descriptorCLS = (*env)->FindClass (env, "java/lang/Long");
      descriptorFID = (*env)->GetFieldID (env, descriptorCLS, "value","J");
      (*env)->SetLongField(env, longclass, descriptorFID,(jlong)((IDATA)context));
      };

      This will work as long as Longs aren't cached - but once Longs are cached for performance reasons (as they are in Harmony) modifying the value of a Long in a native may have unexpected repercussions.

      I'm pretty sure this code is in error - I don't know if other native code does a similar thing.

        Activity

        Hide
        Tim Ellison added a comment -

        Committed quick fix in r669129.

        Show
        Tim Ellison added a comment - Committed quick fix in r669129.
        Tim Ellison made changes -
        Assignee Tim Ellison [ tellison ]
        Hide
        Tim Ellison added a comment -

        Yeah, it shouldn't do that.

        The quick fix is to make this a unique instance of Long, but I agree that it is distasteful to modify the Long's value.

        A better solution is to either set the connectContext field to a new instance of Long with the desired value, or (if you don't want to call back) create a private inner class with a long field you can slam without affecting others.

        I'm inclined towards the latter option.

        Index: modules/nio/src/main/java/common/org/apache/harmony/nio/internal/SocketChannelImpl.java
        ===================================================================
        — modules/nio/src/main/java/common/org/apache/harmony/nio/internal/SocketChannelImpl.java (revision 664690)
        +++ modules/nio/src/main/java/common/org/apache/harmony/nio/internal/SocketChannelImpl.java (working copy)
        @@ -121,7 +121,9 @@

        // This content is a point used to set in connect_withtimeout() in pending
        // mode.

        • private Long connectContext = Long.valueOf(0L);
          + // Must be a new instance of Long (i.e. not valueOf) as it's value may
          + // be modified by native code.
          + private Long connectContext = new Long(0L);

        // Used to store the trafficClass value which is simply returned
        // as the value that was set. We also need it to pass it to methods

        Show
        Tim Ellison added a comment - Yeah, it shouldn't do that. The quick fix is to make this a unique instance of Long, but I agree that it is distasteful to modify the Long's value. A better solution is to either set the connectContext field to a new instance of Long with the desired value, or (if you don't want to call back) create a private inner class with a long field you can slam without affecting others. I'm inclined towards the latter option. Index: modules/nio/src/main/java/common/org/apache/harmony/nio/internal/SocketChannelImpl.java =================================================================== — modules/nio/src/main/java/common/org/apache/harmony/nio/internal/SocketChannelImpl.java (revision 664690) +++ modules/nio/src/main/java/common/org/apache/harmony/nio/internal/SocketChannelImpl.java (working copy) @@ -121,7 +121,9 @@ // This content is a point used to set in connect_withtimeout() in pending // mode. private Long connectContext = Long.valueOf(0L); + // Must be a new instance of Long (i.e. not valueOf) as it's value may + // be modified by native code. + private Long connectContext = new Long(0L); // Used to store the trafficClass value which is simply returned // as the value that was set. We also need it to pass it to methods
        Hide
        Nathan Beyer added a comment -

        That's definitely a bug. The RI caches Long instances as well; i believe the javadoc even says this. A Long instance is supposed to be immutable.

        Show
        Nathan Beyer added a comment - That's definitely a bug. The RI caches Long instances as well; i believe the javadoc even says this. A Long instance is supposed to be immutable.
        Andrew Cornwall made changes -
        Field Original Value New Value
        Summary [classlib][nio] NIO native corrupting Long(0) [classlib][nio] NIO native corrupting Long.valueOf(0)
        Andrew Cornwall created issue -

          People

          • Assignee:
            Tim Ellison
            Reporter:
            Andrew Cornwall
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development