Repository: netty/netty
Issue: NetUtil.bytesToIpAddress broken for IPv4
Labels: defect

For textual IPv4 representation the method incorrectly performs bit shifts on the array bytes.

4.1.5.Final


Issue comments:
@henrik-lindqvist maybe you can provide a PR ?

or provide a link to an RFC or specification which describes expected behavior and an input which doesn't produce the expected output.

NetUtil.bytesToIpAddress(new byte[] { 83, -1, -20, 16 }, 0, 4) returns "0.255.255.16" not "83.255.236.16" as expected. See initial post for cause.


Pull request review comments:
shouldnt this be 16 ?

I think 15 should be correct. `"000.000.000.000".length() == 15`

doh... right


Patch that resolved the issue:
diff --git a/codec-socks/src/main/java/io/netty/handler/codec/socksx/v5/Socks5AddressDecoder.java b/codec-socks/src/main/java/io/netty/handler/codec/socksx/v5/Socks5AddressDecoder.java
index 83b15c42f1c..bc37469edb7 100644
--- a/codec-socks/src/main/java/io/netty/handler/codec/socksx/v5/Socks5AddressDecoder.java
+++ b/codec-socks/src/main/java/io/netty/handler/codec/socksx/v5/Socks5AddressDecoder.java
@@ -52,7 +52,7 @@ public String decodeAddress(Socks5AddressType addrType, ByteBuf in) throws Excep
                 } else {
                     byte[] tmp = new byte[IPv6_LEN];
                     in.readBytes(tmp);
-                    return NetUtil.bytesToIpAddress(tmp, 0, IPv6_LEN);
+                    return NetUtil.bytesToIpAddress(tmp);
                 }
             } else {
                 throw new DecoderException("unsupported address type: " + (addrType.byteValue() & 0xFF));
diff --git a/common/src/main/java/io/netty/util/NetUtil.java b/common/src/main/java/io/netty/util/NetUtil.java
index b990d0e9889..a645cd70d63 100644
--- a/common/src/main/java/io/netty/util/NetUtil.java
+++ b/common/src/main/java/io/netty/util/NetUtil.java
@@ -16,7 +16,6 @@
 package io.netty.util;
 
 import io.netty.util.internal.PlatformDependent;
-import io.netty.util.internal.StringUtil;
 import io.netty.util.internal.logging.InternalLogger;
 import io.netty.util.internal.logging.InternalLoggerFactory;
 
@@ -506,35 +505,33 @@ public static String intToIpAddress(int i) {
      * @throws IllegalArgumentException
      *         if {@code length} is not {@code 4} nor {@code 16}
      */
-    public static String bytesToIpAddress(byte[] bytes, int offset, int length) {
-        if (length == 4) {
-            StringBuilder buf = new StringBuilder(15);
-
-            buf.append(bytes[offset ++] >> 24 & 0xff);
-            buf.append('.');
-            buf.append(bytes[offset ++] >> 16 & 0xff);
-            buf.append('.');
-            buf.append(bytes[offset ++] >> 8 & 0xff);
-            buf.append('.');
-            buf.append(bytes[offset] & 0xff);
-
-            return buf.toString();
-        }
-
-        if (length == 16) {
-            final StringBuilder sb = new StringBuilder(39);
-            final int endOffset = offset + 14;
+    public static String bytesToIpAddress(byte[] bytes) {
+        return bytesToIpAddress(bytes, 0, bytes.length);
+    }
 
-            for (; offset < endOffset; offset += 2) {
-                StringUtil.toHexString(sb, bytes, offset, 2);
-                sb.append(':');
+    /**
+     * Converts 4-byte or 16-byte data into an IPv4 or IPv6 string respectively.
+     *
+     * @throws IllegalArgumentException
+     *         if {@code length} is not {@code 4} nor {@code 16}
+     */
+    public static String bytesToIpAddress(byte[] bytes, int offset, int length) {
+        switch (length) {
+            case 4: {
+                return new StringBuilder(15)
+                        .append(bytes[offset] & 0xff)
+                        .append('.')
+                        .append(bytes[offset + 1] & 0xff)
+                        .append('.')
+                        .append(bytes[offset + 2] & 0xff)
+                        .append('.')
+                        .append(bytes[offset + 3] & 0xff).toString();
             }
-            StringUtil.toHexString(sb, bytes, offset, 2);
-
-            return sb.toString();
+            case 16:
+                return toAddressString(bytes, offset, false);
+            default:
+                throw new IllegalArgumentException("length: " + length + " (expected: 4 or 16)");
         }
-
-        throw new IllegalArgumentException("length: " + length + " (expected: 4 or 16)");
     }
 
     public static boolean isValidIpV6Address(String ipAddress) {
@@ -981,13 +978,17 @@ public static String toAddressString(InetAddress ip, boolean ipv4Mapped) {
             return ip.getHostAddress();
         }
         if (!(ip instanceof Inet6Address)) {
-            throw new IllegalArgumentException("Unhandled type: " + ip.getClass());
+            throw new IllegalArgumentException("Unhandled type: " + ip);
         }
 
-        final byte[] bytes = ip.getAddress();
+        return toAddressString(ip.getAddress(), 0, ipv4Mapped);
+    }
+
+    private static String toAddressString(byte[] bytes, int offset, boolean ipv4Mapped) {
         final int[] words = new int[IPV6_WORD_COUNT];
         int i;
-        for (i = 0; i < words.length; ++i) {
+        final int end = offset + words.length;
+        for (i = offset; i < end; ++i) {
             words[i] = ((bytes[i << 1] & 0xff) << 8) | (bytes[(i << 1) + 1] & 0xff);
         }
 


Test patch:
diff --git a/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandRequestDecoderTest.java b/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandRequestDecoderTest.java
index 8b0478a4b15..728ee1eb7a1 100755
--- a/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandRequestDecoderTest.java
+++ b/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandRequestDecoderTest.java
@@ -67,7 +67,7 @@ public void testCmdRequestDecoderIPv4() {
     @Test
     public void testCmdRequestDecoderIPv6() {
         String[] hosts = {
-                NetUtil.bytesToIpAddress(IPAddressUtil.textToNumericFormatV6("::1"), 0, 16) };
+                NetUtil.bytesToIpAddress(IPAddressUtil.textToNumericFormatV6("::1")) };
         int[] ports = {1, 32769, 65535};
         for (Socks5CommandType cmdType: Arrays.asList(Socks5CommandType.BIND,
                                                       Socks5CommandType.CONNECT,
diff --git a/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandResponseDecoderTest.java b/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandResponseDecoderTest.java
index 150a4169a1b..23f8cbe589f 100755
--- a/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandResponseDecoderTest.java
+++ b/codec-socks/src/test/java/io/netty/handler/codec/socksx/v5/Socks5CommandResponseDecoderTest.java
@@ -22,7 +22,8 @@
 
 import java.util.Arrays;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
 
 public class Socks5CommandResponseDecoderTest {
 
@@ -92,7 +93,7 @@ public void testSocksCmdResponseDecoderIncludingHost() {
             test(cmdStatus, Socks5AddressType.IPv6,
                  "2001:db8:85a3:42:1000:8a2e:370:7334", 80);
             test(cmdStatus, Socks5AddressType.IPv6,
-                 "1111:111:11:1:0:0:0:1", 80);
+                 "1111:111:11:1::1", 80);
         }
     }
 }
diff --git a/common/src/test/java/io/netty/util/NetUtilTest.java b/common/src/test/java/io/netty/util/NetUtilTest.java
index f0f600cf8a2..ecd60d5d3c1 100644
--- a/common/src/test/java/io/netty/util/NetUtilTest.java
+++ b/common/src/test/java/io/netty/util/NetUtilTest.java
@@ -17,12 +17,17 @@
 
 import org.junit.Test;
 
+import java.net.Inet6Address;
 import java.net.InetAddress;
 import java.net.UnknownHostException;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Map.Entry;
 
+import static io.netty.util.NetUtil.bytesToIpAddress;
+import static io.netty.util.NetUtil.createByteArrayFromIpAddressString;
+import static io.netty.util.NetUtil.getByName;
+import static io.netty.util.NetUtil.toAddressString;
 import static org.junit.Assert.*;
 
 public class NetUtilTest {
@@ -44,7 +49,8 @@ private static final class TestMap extends HashMap<String, String> {
             "10.255.255.254", "0afffffe",
             "172.18.5.4",     "ac120504",
             "0.0.0.0",        "00000000",
-            "127.0.0.1",      "7f000001");
+            "127.0.0.1",      "7f000001",
+            "1.2.3.4",        "01020304");
 
     private static final Map<String, String> invalidIpV4Hosts = new TestMap(
             "1.256.3.4",     null,
@@ -438,30 +444,40 @@ public void testIsValidIpV6Address() {
     @Test
     public void testCreateByteArrayFromIpAddressString() {
         for (Entry<String, String> e : validIpV4Hosts.entrySet()) {
-            assertHexDumpEquals(e.getValue(), NetUtil.createByteArrayFromIpAddressString(e.getKey()));
+            assertHexDumpEquals(e.getValue(), createByteArrayFromIpAddressString(e.getKey()));
         }
         for (Entry<String, String> e : invalidIpV4Hosts.entrySet()) {
-            assertHexDumpEquals(e.getValue(), NetUtil.createByteArrayFromIpAddressString(e.getKey()));
+            assertHexDumpEquals(e.getValue(), createByteArrayFromIpAddressString(e.getKey()));
         }
         for (Entry<String, String> e : validIpV6Hosts.entrySet()) {
-            assertHexDumpEquals(e.getValue(), NetUtil.createByteArrayFromIpAddressString(e.getKey()));
+            assertHexDumpEquals(e.getValue(), createByteArrayFromIpAddressString(e.getKey()));
         }
         for (Entry<String, String> e : invalidIpV6Hosts.entrySet()) {
-            assertHexDumpEquals(e.getValue(), NetUtil.createByteArrayFromIpAddressString(e.getKey()));
+            assertHexDumpEquals(e.getValue(), createByteArrayFromIpAddressString(e.getKey()));
+        }
+    }
+
+    @Test
+    public void testBytesToIpAddress() throws UnknownHostException {
+        for (Entry<String, String> e : validIpV4Hosts.entrySet()) {
+            assertEquals(e.getKey(), bytesToIpAddress(createByteArrayFromIpAddressString(e.getKey())));
+        }
+        for (Entry<byte[], String> testEntry : ipv6ToAddressStrings.entrySet()) {
+            assertEquals(testEntry.getValue(), bytesToIpAddress(testEntry.getKey()));
         }
     }
 
     @Test
     public void testIp6AddressToString() throws UnknownHostException {
         for (Entry<byte[], String> testEntry : ipv6ToAddressStrings.entrySet()) {
-            assertEquals(testEntry.getValue(), NetUtil.toAddressString(InetAddress.getByAddress(testEntry.getKey())));
+            assertEquals(testEntry.getValue(), toAddressString(InetAddress.getByAddress(testEntry.getKey())));
         }
     }
 
     @Test
     public void testIp4AddressToString() throws UnknownHostException {
         for (Entry<String, String> e : validIpV4Hosts.entrySet()) {
-            assertEquals(e.getKey(), NetUtil.toAddressString(InetAddress.getByAddress(unhex(e.getValue()))));
+            assertEquals(e.getKey(), toAddressString(InetAddress.getByAddress(unhex(e.getValue()))));
         }
     }
 
@@ -470,18 +486,18 @@ public void testIpv4MappedIp6GetByName() {
         for (Entry<String, String> testEntry : ipv4MappedToIPv6AddressStrings.entrySet()) {
             assertEquals(
                     testEntry.getValue(),
-                    NetUtil.toAddressString(NetUtil.getByName(testEntry.getKey(), true), true));
+                    toAddressString(getByName(testEntry.getKey(), true), true));
         }
     }
 
     @Test
     public void testinvalidIpv4MappedIp6GetByName() {
         for (String testEntry : invalidIpV4Hosts.keySet()) {
-            assertNull(NetUtil.getByName(testEntry, true));
+            assertNull(getByName(testEntry, true));
         }
 
         for (String testEntry : invalidIpV6Hosts.keySet()) {
-            assertNull(NetUtil.getByName(testEntry, true));
+            assertNull(getByName(testEntry, true));
         }
     }