JRuby (please use github issues at http://bugs.jruby.org)
  1. JRuby (please use github issues at http://bugs.jruby.org)
  2. JRUBY-5479

Socket#pack_sockaddr_in fails for port numbers greater than 32383

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: JRuby 1.6RC2
    • Fix Version/s: JRuby 1.6RC3
    • Component/s: None
    • Labels:
      None
    • Environment:
      OSX
    • Number of attachments :
      0

      Description

      If you pack a sockaddr involving a port higher than 32383, it will create an invalid sockaddr with a port probably outside the legal range.

      irb(main):021:0> Socket.unpack_sockaddr_in( Socket.pack_sockaddr_in( 32383, 'localhost' ) ).inspect
      => "[32383, \"127.0.0.1\"]"
      irb(main):022:0> Socket.unpack_sockaddr_in( Socket.pack_sockaddr_in( 32384, 'localhost' ) ).inspect
      => "[97789, \"127.0.0.1\"]"
      

        Issue Links

          Activity

          Hide
          Hiroshi Nakamura added a comment -

          This patch should fix. I don't have enough time to run spec/test now...

          diff --git a/src/org/jruby/ext/socket/RubySocket.java b/src/org/jruby/ext/socket/RubySocket.jav
          a
          index 0c11d8c..2c92790 100644
          --- a/src/org/jruby/ext/socket/RubySocket.java
          +++ b/src/org/jruby/ext/socket/RubySocket.java
          @@ -557,7 +557,7 @@ public class RubySocket extends RubyBasicSocket {
                       throw context.getRuntime().newArgumentError("can't resolve socket address of wrong
           type");
                   }
           
          -        int port = (val.charAt(2) << 8) + (val.charAt(3));
          +        int port = (val.charAt(2) << 8) | (val.charAt(3));
                   StringBuilder sb = new StringBuilder();
                   sb.append((int)val.charAt(4));
                   sb.append(".");
          
          Show
          Hiroshi Nakamura added a comment - This patch should fix. I don't have enough time to run spec/test now... diff --git a/src/org/jruby/ext/socket/RubySocket.java b/src/org/jruby/ext/socket/RubySocket.jav a index 0c11d8c..2c92790 100644 --- a/src/org/jruby/ext/socket/RubySocket.java +++ b/src/org/jruby/ext/socket/RubySocket.java @@ -557,7 +557,7 @@ public class RubySocket extends RubyBasicSocket { throw context.getRuntime().newArgumentError("can't resolve socket address of wrong type"); } - int port = (val.charAt(2) << 8) + (val.charAt(3)); + int port = (val.charAt(2) << 8) | (val.charAt(3)); StringBuilder sb = new StringBuilder(); sb.append((int)val.charAt(4)); sb.append(".");
          Hide
          Hiro Asari added a comment - - edited

          val.charAt(3) is a byte, so that it fits in 8 bits; + and | are equivalent here.

          This turns out to be a regression introduced by:

          $ git bisect bad
          028579935f48d49f7e60c28d37422832ed4ef34f is the first bad commit
          commit 028579935f48d49f7e60c28d37422832ed4ef34f
          Author: Charles Oliver Nutter <headius@headius.com>
          Date:   Wed Jan 26 23:02:44 2011 -0600
          
              Fix JRUBY-5388: Requiring a filename with accented characters fails
              
              * When converting a require/load name to a Java String, be sure to decode using the encoding
          
          :040000 040000 0408ddaa95d42e0e20fc3363cc78a37e0e333d56 7c2023753927785d8331a8387f2b81f52c5bb414 M	src
          

          And it only affects 1.8 mode.

          $ jruby --1.9 -v -rsocket -e "p Socket.unpack_sockaddr_in( Socket.pack_sockaddr_in( 32385, 'localhost' ) ).inspect"
          jruby 1.6.0.RC2 (ruby 1.9.2 patchlevel 136) (2011-02-09 5434c72) (Java HotSpot(TM) 64-Bit Server VM 1.6.0_22) [darwin-x86_64-java]
          "[32385, \"127.0.0.1\"]"
          

          Looks like we are decoding the bytes incorrectly.

          Show
          Hiro Asari added a comment - - edited val.charAt(3) is a byte, so that it fits in 8 bits; + and | are equivalent here. This turns out to be a regression introduced by: $ git bisect bad 028579935f48d49f7e60c28d37422832ed4ef34f is the first bad commit commit 028579935f48d49f7e60c28d37422832ed4ef34f Author: Charles Oliver Nutter <headius@headius.com> Date: Wed Jan 26 23:02:44 2011 -0600 Fix JRUBY-5388: Requiring a filename with accented characters fails * When converting a require/load name to a Java String, be sure to decode using the encoding :040000 040000 0408ddaa95d42e0e20fc3363cc78a37e0e333d56 7c2023753927785d8331a8387f2b81f52c5bb414 M src And it only affects 1.8 mode. $ jruby --1.9 -v -rsocket -e "p Socket.unpack_sockaddr_in( Socket.pack_sockaddr_in( 32385, 'localhost' ) ).inspect" jruby 1.6.0.RC2 (ruby 1.9.2 patchlevel 136) (2011-02-09 5434c72) (Java HotSpot(TM) 64-Bit Server VM 1.6.0_22) [darwin-x86_64-java] "[32385, \"127.0.0.1\"]" Looks like we are decoding the bytes incorrectly.
          Hide
          Hiroshi Nakamura added a comment -

          Hiro: You're right. I wrote too soon. And it looks we don't support IPv6...

          Is there anyone who wants to tackle this?

          Show
          Hiroshi Nakamura added a comment - Hiro: You're right. I wrote too soon. And it looks we don't support IPv6... Is there anyone who wants to tackle this?
          Hide
          Hiroshi Nakamura added a comment -

          2nd try;

          diff --git a/src/org/jruby/ext/socket/RubySocket.java b/src/org/jruby/ext/socket/RubySocket.jav
          a
          index 0c11d8c..7e164e0 100644
          --- a/src/org/jruby/ext/socket/RubySocket.java
          +++ b/src/org/jruby/ext/socket/RubySocket.java
          @@ -552,20 +552,24 @@ public class RubySocket extends RubyBasicSocket {
               }
               @JRubyMethod(meta = true)
               public static IRubyObject unpack_sockaddr_in(ThreadContext context, IRubyObject recv, IRub
          yObject addr) {
          -        String val = addr.convertToString().toString();
          -        if((Platform.IS_BSD && val.charAt(0) != 16 && val.charAt(1) != 2) || (!Platform.IS_BSD
           && val.charAt(0) != 2)) {
          +        byte[] val = addr.convertToString().getBytes();
          +        // TODO: IPv6 support.
          +        if (val.length < 8) {
          +            throw context.getRuntime().newArgumentError("too short sockaddr");
          +        }
          +        if((Platform.IS_BSD && val[0] != 16 && val[1] != 2) || (!Platform.IS_BSD && val[0] != 
          2)) {
                       throw context.getRuntime().newArgumentError("can't resolve socket address of wrong
           type");
                   }
           
          -        int port = (val.charAt(2) << 8) + (val.charAt(3));
          +        int port = (val[2] & 0xff) << 8 | (val[3] & 0xff);
                   StringBuilder sb = new StringBuilder();
          -        sb.append((int)val.charAt(4));
          +        sb.append((int)val[4]);
                   sb.append(".");
          -        sb.append((int)val.charAt(5));
          +        sb.append((int)val[5]);
                   sb.append(".");
          -        sb.append((int)val.charAt(6));
          +        sb.append((int)val[6]);
                   sb.append(".");
          -        sb.append((int)val.charAt(7));
          +        sb.append((int)val[7]);
           
                   IRubyObject[] result = new IRubyObject[]{
                           context.getRuntime().newFixnum(port),
          
          Show
          Hiroshi Nakamura added a comment - 2nd try; diff --git a/src/org/jruby/ext/socket/RubySocket.java b/src/org/jruby/ext/socket/RubySocket.jav a index 0c11d8c..7e164e0 100644 --- a/src/org/jruby/ext/socket/RubySocket.java +++ b/src/org/jruby/ext/socket/RubySocket.java @@ -552,20 +552,24 @@ public class RubySocket extends RubyBasicSocket { } @JRubyMethod(meta = true) public static IRubyObject unpack_sockaddr_in(ThreadContext context, IRubyObject recv, IRub yObject addr) { - String val = addr.convertToString().toString(); - if((Platform.IS_BSD && val.charAt(0) != 16 && val.charAt(1) != 2) || (!Platform.IS_BSD && val.charAt(0) != 2)) { + byte[] val = addr.convertToString().getBytes(); + // TODO: IPv6 support. + if (val.length < 8) { + throw context.getRuntime().newArgumentError("too short sockaddr"); + } + if((Platform.IS_BSD && val[0] != 16 && val[1] != 2) || (!Platform.IS_BSD && val[0] != 2)) { throw context.getRuntime().newArgumentError("can't resolve socket address of wrong type"); } - int port = (val.charAt(2) << 8) + (val.charAt(3)); + int port = (val[2] & 0xff) << 8 | (val[3] & 0xff); StringBuilder sb = new StringBuilder(); - sb.append((int)val.charAt(4)); + sb.append((int)val[4]); sb.append("."); - sb.append((int)val.charAt(5)); + sb.append((int)val[5]); sb.append("."); - sb.append((int)val.charAt(6)); + sb.append((int)val[6]); sb.append("."); - sb.append((int)val.charAt(7)); + sb.append((int)val[7]); IRubyObject[] result = new IRubyObject[]{ context.getRuntime().newFixnum(port),
          Hide
          Hiro Asari added a comment -

          That looks good, passes all the existing specs as well.

          Show
          Hiro Asari added a comment - That looks good, passes all the existing specs as well.
          Hide
          Charles Oliver Nutter added a comment -

          I suspect this was fixed by the following commit, since it works fine now:

          commit 2eb09cdec693a70b11ea6f122e0c465bfff28300
          Author: Charles Oliver Nutter <headius@headius.com>
          Date: Sat Feb 12 03:31:12 2011 -0600

          Fix intermittent failure in JRUBY-5232 regression test.

          By passing through java.lang.String, RubyBasicSocket occasionally introduced UTF-8 character sequences that just happened to be in the sockaddr bytes as encoded characters in the resulting String. As a result, the characters were not correctly aligned with the original bytes, causing them to occasionally be shifted (since a UTF sequence might consume more than one byte).

          Show
          Charles Oliver Nutter added a comment - I suspect this was fixed by the following commit, since it works fine now: commit 2eb09cdec693a70b11ea6f122e0c465bfff28300 Author: Charles Oliver Nutter <headius@headius.com> Date: Sat Feb 12 03:31:12 2011 -0600 Fix intermittent failure in JRUBY-5232 regression test. By passing through java.lang.String, RubyBasicSocket occasionally introduced UTF-8 character sequences that just happened to be in the sockaddr bytes as encoded characters in the resulting String. As a result, the characters were not correctly aligned with the original bytes, causing them to occasionally be shifted (since a UTF sequence might consume more than one byte).
          Hide
          Charles Oliver Nutter added a comment -

          I'm not quite sure why my change fixed this, but adding the unsigned masking might have had something to do with it.

          Show
          Charles Oliver Nutter added a comment - I'm not quite sure why my change fixed this, but adding the unsigned masking might have had something to do with it.
          Hide
          Hiroshi Nakamura added a comment -

          Confirmed that the commit by Charles fixes this problem. My patch doesn't care char-to-int casting problem for each component of IPv4 addr.

          Charles: byte-to-int-cast-with-0xff is needed for unsigned masking as you wrote but the fix for this issue (JRUBY-5479) is using byte[] instead of char[]. charAt(3) looks to be 0xfffd in pack_sockaddr_in when 0x80 is encoded. I'll investigate where this invalid char is generated, as well as IPv6 support.

          Show
          Hiroshi Nakamura added a comment - Confirmed that the commit by Charles fixes this problem. My patch doesn't care char-to-int casting problem for each component of IPv4 addr. Charles: byte-to-int-cast-with-0xff is needed for unsigned masking as you wrote but the fix for this issue ( JRUBY-5479 ) is using byte[] instead of char[]. charAt(3) looks to be 0xfffd in pack_sockaddr_in when 0x80 is encoded. I'll investigate where this invalid char is generated, as well as IPv6 support.

            People

            • Assignee:
              Charles Oliver Nutter
              Reporter:
              bob mcwhirter
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: