Repository: netty/netty
Issue: Wrong regular expression to check MACAdresses in DefaultChannelId
Labels: defect

Using Netty on a Java-VM running on IBM z/OS it occurs, that creating a unique channel-id leads to problems.
It seems to me, that this process bases at least on the MAC-Address of networking hardware.  On IBM z/OS this information is not available with the usual JAVA-methods in NetworkInterface.  Netty logs always a warning to System.err running on z/OS that it fell back to a random MAC.
So I decided to use the VM-Option io.netty.machineId=xxxxxxxx to inject here a valid MAC-Address retrieved from non-java resources on z/OS.

**To my regrets it does not work. The problem is not platform-dependent, even with in a SUN-VM running on Windows it is not possible to use this option.** 

Probably the Regular Expression, used to check the input, is not correct.
I would recommend to change the used RegExp 
` private static final Pattern MACHINE_ID_PATTERN = Pattern.compile("^(?:[0-9a-fA-F][:-]?){6,8}$");`
to an expression more like 
`([0-9a-fA-F]{2}[:-]){5,7}[0-9a-fA-F]{2}`


### Expected behavior
DefaultChannelId allows to customize the used MACAddress via System.property.
Using -Dio.netty.machineId=00-14-5E-5A-AF-77 as Java-VM-option should be accepted as valid MACAddress.

### Actual behavior
Netty rejects this MACAddress as malformed:

`Dez 14, 2016 2:10:49 PM io.netty.channel.DefaultChannelId <clinit>
WARNING: -Dio.netty.machineId: 00-14-5E-5A-AF-77 (malformed)
Dez 14, 2016 2:10:49 PM io.netty.handler.logging.LoggingHandler channelRegistered
INFO: [id: 0xa9354b7b] REGISTERED
Dez 14, 2016 2:10:49 PM io.netty.handler.logging.LoggingHandler bind
INFO: [id: 0xa9354b7b] BIND: 0.0.0.0/0.0.0.0:8007
Dez 14, 2016 2:10:49 PM io.netty.handler.logging.LoggingHandler channelActive
INFO: [id: 0xa9354b7b, L:/0:0:0:0:0:0:0:0:8007] ACTIVE`

### Steps to reproduce
Start any Netty-using application (i.e. your sample io.netty.example.echo.EchoServer) with VM-option -Dio.netty.machineId=xxxxxxxx to overrule the standard MAC-Address ascertaining in MacAddressUtil.bestAvailableMac(), where xxxxxxx is any MAC-Address in standard (IEEE 802) format for printing MAC-48 addresses in human-friendly form.

### Minimal yet complete reproducer code (or URL to code)
see above. Reproducible via VM-option

### Netty version
4.1.5

### JVM version (e.g. `java -version`)
java version "1.8.0_111"
Java(TM) SE Runtime Environment (build 1.8.0_111-b14)
Java HotSpot(TM) 64-Bit Server VM (build 25.111-b14, mixed mode)

### OS version (e.g. `uname -a`)
Windows 7 Professional Service Pack 1


Issue comments:
@jo-kin - Thanks for reporting. Stand by for a PR.
Thanx a lot.
Deeply impressed by speed and thoroughness. Will report how it works for me soon.
Wishing you a very Happy Christmas and New Year 2017.  

Pull request review comments:
@Scottmitch just remove the `@return` as it is the same as what is stated already ? 
@Scottmitch `:-]` looks nice and friendly ;)
sure

Patch that resolved the issue:
diff --git a/common/src/main/java/io/netty/util/internal/MacAddressUtil.java b/common/src/main/java/io/netty/util/internal/MacAddressUtil.java
index 858368d6dd9..49559b175e2 100644
--- a/common/src/main/java/io/netty/util/internal/MacAddressUtil.java
+++ b/common/src/main/java/io/netty/util/internal/MacAddressUtil.java
@@ -16,6 +16,10 @@
 
 package io.netty.util.internal;
 
+import io.netty.util.NetUtil;
+import io.netty.util.internal.logging.InternalLogger;
+import io.netty.util.internal.logging.InternalLoggerFactory;
+
 import java.net.InetAddress;
 import java.net.NetworkInterface;
 import java.net.SocketException;
@@ -25,21 +29,14 @@
 import java.util.Map;
 import java.util.Map.Entry;
 
-import io.netty.util.NetUtil;
-import io.netty.util.internal.logging.InternalLogger;
-import io.netty.util.internal.logging.InternalLoggerFactory;
+import static io.netty.util.internal.EmptyArrays.EMPTY_BYTES;
 
 public final class MacAddressUtil {
-
-    /**
-     * Length of a valid MAC address.
-     */
-    public static final int MAC_ADDRESS_LENGTH = 8;
-
-    private static final byte[] NOT_FOUND = { -1 };
-
     private static final InternalLogger logger = InternalLoggerFactory.getInstance(MacAddressUtil.class);
 
+    private static final int EUI64_MAC_ADDRESS_LENGTH = 8;
+    private static final int EUI48_MAC_ADDRESS_LENGTH = 6;
+
     /**
      * Obtains the best MAC address found on local network interfaces.
      * Generally speaking, an active network interface used on public
@@ -49,7 +46,7 @@ public final class MacAddressUtil {
      */
     public static byte[] bestAvailableMac() {
         // Find the best MAC address available.
-        byte[] bestMacAddr = NOT_FOUND;
+        byte[] bestMacAddr = EMPTY_BYTES;
         InetAddress bestInetAddr = NetUtil.LOCALHOST4;
 
         // Retrieve the list of available network interfaces.
@@ -110,13 +107,13 @@ public static byte[] bestAvailableMac() {
             }
         }
 
-        if (bestMacAddr == NOT_FOUND) {
+        if (bestMacAddr == EMPTY_BYTES) {
             return null;
         }
 
         switch (bestMacAddr.length) {
-            case 6: // EUI-48 - convert to EUI-64
-                byte[] newAddr = new byte[MAC_ADDRESS_LENGTH];
+            case EUI48_MAC_ADDRESS_LENGTH: // EUI-48 - convert to EUI-64
+                byte[] newAddr = new byte[EUI64_MAC_ADDRESS_LENGTH];
                 System.arraycopy(bestMacAddr, 0, newAddr, 0, 3);
                 newAddr[3] = (byte) 0xFF;
                 newAddr[4] = (byte) 0xFE;
@@ -124,12 +121,73 @@ public static byte[] bestAvailableMac() {
                 bestMacAddr = newAddr;
                 break;
             default: // Unknown
-                bestMacAddr = Arrays.copyOf(bestMacAddr, MAC_ADDRESS_LENGTH);
+                bestMacAddr = Arrays.copyOf(bestMacAddr, EUI64_MAC_ADDRESS_LENGTH);
         }
 
         return bestMacAddr;
     }
 
+    /**
+     * Returns the result of {@link #bestAvailableMac()} if non-{@code null} otherwise returns a random EUI-64 MAC
+     * address.
+     */
+    public static byte[] defaultMachineId() {
+        byte[] bestMacAddr = MacAddressUtil.bestAvailableMac();
+        if (bestMacAddr == null) {
+            bestMacAddr = new byte[EUI64_MAC_ADDRESS_LENGTH];
+            ThreadLocalRandom.current().nextBytes(bestMacAddr);
+            logger.warn(
+                    "Failed to find a usable hardware address from the network interfaces; using random bytes: {}",
+                    MacAddressUtil.formatAddress(bestMacAddr));
+        }
+        return bestMacAddr;
+    }
+
+    /**
+     * Parse a EUI-48, MAC-48, or EUI-64 MAC address from a {@link String} and return it as a {@code byte[]}.
+     * @param value The string representation of the MAC address.
+     * @return The byte representation of the MAC address.
+     */
+    public static byte[] parseMAC(String value) {
+        final byte[] machineId;
+        final char separator;
+        switch (value.length()) {
+            case 17:
+                separator = value.charAt(2);
+                validateMacSeparator(separator);
+                machineId = new byte[EUI48_MAC_ADDRESS_LENGTH];
+                break;
+            case 23:
+                separator = value.charAt(2);
+                validateMacSeparator(separator);
+                machineId = new byte[EUI64_MAC_ADDRESS_LENGTH];
+                break;
+            default:
+                throw new IllegalArgumentException("value is not supported [MAC-48, EUI-48, EUI-64]");
+        }
+
+        final int end = machineId.length - 1;
+        int j = 0;
+        for (int i = 0; i < end; ++i, j += 3) {
+            final int sIndex = j + 2;
+            machineId[i] = (byte) Integer.parseInt(value.substring(j, sIndex), 16);
+            if (value.charAt(sIndex) != separator) {
+                throw new IllegalArgumentException("expected separator '" + separator + " but got '" +
+                        value.charAt(sIndex) + "' at index: " + sIndex);
+            }
+        }
+
+        machineId[end] = (byte) Integer.parseInt(value.substring(j, value.length()), 16);
+
+        return machineId;
+    }
+
+    private static void validateMacSeparator(char separator) {
+        if (separator != ':' && separator != '-') {
+            throw new IllegalArgumentException("unsupported seperator: " + separator + " (expected: [:-])");
+        }
+    }
+
     /**
      * @param addr byte array of a MAC address.
      * @return hex formatted MAC address.
@@ -146,12 +204,7 @@ public static String formatAddress(byte[] addr) {
      * @return positive - current is better, 0 - cannot tell from MAC addr, negative - candidate is better.
      */
     private static int compareAddresses(byte[] current, byte[] candidate) {
-        if (candidate == null) {
-            return 1;
-        }
-
-        // Must be EUI-48 or longer.
-        if (candidate.length < 6) {
+        if (candidate == null || candidate.length < EUI48_MAC_ADDRESS_LENGTH) {
             return 1;
         }
 
@@ -174,7 +227,7 @@ private static int compareAddresses(byte[] current, byte[] candidate) {
         }
 
         // Prefer globally unique address.
-        if ((current[0] & 2) == 0) {
+        if (current.length == 0 || (current[0] & 2) == 0) {
             if ((candidate[0] & 2) == 0) {
                 // Both current and candidate are globally unique addresses.
                 return 0;
@@ -182,15 +235,12 @@ private static int compareAddresses(byte[] current, byte[] candidate) {
                 // Only current is globally unique.
                 return 1;
             }
-        } else {
-            if ((candidate[0] & 2) == 0) {
-                // Only candidate is globally unique.
-                return -1;
-            } else {
-                // Both current and candidate are non-unique.
-                return 0;
-            }
+        } else if ((candidate[0] & 2) == 0) {
+            // Only candidate is globally unique.
+            return -1;
         }
+        // Both current and candidate are non-unique.
+        return 0;
     }
 
     /**
diff --git a/transport/src/main/java/io/netty/channel/DefaultChannelId.java b/transport/src/main/java/io/netty/channel/DefaultChannelId.java
index 0bb2a8afa41..734094f91c2 100644
--- a/transport/src/main/java/io/netty/channel/DefaultChannelId.java
+++ b/transport/src/main/java/io/netty/channel/DefaultChannelId.java
@@ -28,7 +28,9 @@
 import java.lang.reflect.Method;
 import java.util.Arrays;
 import java.util.concurrent.atomic.AtomicInteger;
-import java.util.regex.Pattern;
+
+import static io.netty.util.internal.MacAddressUtil.defaultMachineId;
+import static io.netty.util.internal.MacAddressUtil.parseMAC;
 
 /**
  * The default {@link ChannelId} implementation.
@@ -38,9 +40,6 @@ public final class DefaultChannelId implements ChannelId {
     private static final long serialVersionUID = 3884076183504074063L;
 
     private static final InternalLogger logger = InternalLoggerFactory.getInstance(DefaultChannelId.class);
-
-    private static final Pattern MACHINE_ID_PATTERN = Pattern.compile("^(?:[0-9a-fA-F][:-]?){6,8}$");
-    private static final int MACHINE_ID_LEN = MacAddressUtil.MAC_ADDRESS_LENGTH;
     private static final byte[] MACHINE_ID;
     private static final int PROCESS_ID_LEN = 4;
     private static final int PROCESS_ID;
@@ -54,9 +53,7 @@ public final class DefaultChannelId implements ChannelId {
      * Returns a new {@link DefaultChannelId} instance.
      */
     public static DefaultChannelId newInstance() {
-        DefaultChannelId id = new DefaultChannelId();
-        id.init();
-        return id;
+        return new DefaultChannelId();
     }
 
     static {
@@ -89,11 +86,13 @@ public static DefaultChannelId newInstance() {
         byte[] machineId = null;
         String customMachineId = SystemPropertyUtil.get("io.netty.machineId");
         if (customMachineId != null) {
-            if (MACHINE_ID_PATTERN.matcher(customMachineId).matches()) {
-                machineId = parseMachineId(customMachineId);
+            try {
+                machineId = parseMAC(customMachineId);
+            } catch (Exception e) {
+                logger.warn("-Dio.netty.machineId: {} (malformed)", customMachineId, e);
+            }
+            if (machineId != null) {
                 logger.debug("-Dio.netty.machineId: {} (user-set)", customMachineId);
-            } else {
-                logger.warn("-Dio.netty.machineId: {} (malformed)", customMachineId);
             }
         }
 
@@ -107,31 +106,6 @@ public static DefaultChannelId newInstance() {
         MACHINE_ID = machineId;
     }
 
-    @SuppressWarnings("DynamicRegexReplaceableByCompiledPattern")
-    private static byte[] parseMachineId(String value) {
-        // Strip separators.
-        value = value.replaceAll("[:-]", "");
-
-        byte[] machineId = new byte[MACHINE_ID_LEN];
-        for (int i = 0; i < value.length(); i += 2) {
-            machineId[i] = (byte) Integer.parseInt(value.substring(i, i + 2), 16);
-        }
-
-        return machineId;
-    }
-
-    private static byte[] defaultMachineId() {
-        byte[] bestMacAddr = MacAddressUtil.bestAvailableMac();
-        if (bestMacAddr == null) {
-            bestMacAddr = new byte[MacAddressUtil.MAC_ADDRESS_LENGTH];
-            ThreadLocalRandom.current().nextBytes(bestMacAddr);
-            logger.warn(
-                    "Failed to find a usable hardware address from the network interfaces; using random bytes: {}",
-                    MacAddressUtil.formatAddress(bestMacAddr));
-        }
-        return bestMacAddr;
-    }
-
     private static int defaultProcessId() {
         final ClassLoader loader = PlatformDependent.getClassLoader(DefaultChannelId.class);
         String value;
@@ -178,20 +152,19 @@ private static int defaultProcessId() {
         return pid;
     }
 
-    private final byte[] data = new byte[MACHINE_ID_LEN + PROCESS_ID_LEN + SEQUENCE_LEN + TIMESTAMP_LEN + RANDOM_LEN];
+    private final byte[] data;
     private int hashCode;
 
     private transient String shortValue;
     private transient String longValue;
 
-    private DefaultChannelId() { }
-
-    private void init() {
+    private DefaultChannelId() {
+        data = new byte[MACHINE_ID.length + PROCESS_ID_LEN + SEQUENCE_LEN + TIMESTAMP_LEN + RANDOM_LEN];
         int i = 0;
 
         // machineId
-        System.arraycopy(MACHINE_ID, 0, data, i, MACHINE_ID_LEN);
-        i += MACHINE_ID_LEN;
+        System.arraycopy(MACHINE_ID, 0, data, i, MACHINE_ID.length);
+        i += MACHINE_ID.length;
 
         // processId
         i = writeInt(i, PROCESS_ID);
@@ -234,8 +207,7 @@ private int writeLong(int i, long value) {
     public String asShortText() {
         String shortValue = this.shortValue;
         if (shortValue == null) {
-            this.shortValue = shortValue = ByteBufUtil.hexDump(
-                    data, MACHINE_ID_LEN + PROCESS_ID_LEN + SEQUENCE_LEN + TIMESTAMP_LEN, RANDOM_LEN);
+            this.shortValue = shortValue = ByteBufUtil.hexDump(data, data.length - RANDOM_LEN, RANDOM_LEN);
         }
         return shortValue;
     }
@@ -252,7 +224,7 @@ public String asLongText() {
     private String newLongValue() {
         StringBuilder buf = new StringBuilder(2 * data.length + 5);
         int i = 0;
-        i = appendHexDumpField(buf, i, MACHINE_ID_LEN);
+        i = appendHexDumpField(buf, i, MACHINE_ID.length);
         i = appendHexDumpField(buf, i, PROCESS_ID_LEN);
         i = appendHexDumpField(buf, i, SEQUENCE_LEN);
         i = appendHexDumpField(buf, i, TIMESTAMP_LEN);


Test patch:
diff --git a/common/src/test/java/io/netty/util/internal/MacAddressUtilTest.java b/common/src/test/java/io/netty/util/internal/MacAddressUtilTest.java
new file mode 100644
index 00000000000..84d4e1229c7
--- /dev/null
+++ b/common/src/test/java/io/netty/util/internal/MacAddressUtilTest.java
@@ -0,0 +1,99 @@
+/*
+ * Copyright 2016 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package io.netty.util.internal;
+
+import org.junit.Test;
+
+import static io.netty.util.internal.MacAddressUtil.parseMAC;
+import static org.junit.Assert.assertArrayEquals;
+
+public class MacAddressUtilTest {
+    @Test
+    public void testParseMacEUI48() {
+        assertArrayEquals(new byte[]{0, (byte) 0xaa, 0x11, (byte) 0xbb, 0x22, (byte) 0xcc},
+                parseMAC("00-AA-11-BB-22-CC"));
+        assertArrayEquals(new byte[]{0, (byte) 0xaa, 0x11, (byte) 0xbb, 0x22, (byte) 0xcc},
+                parseMAC("00:AA:11:BB:22:CC"));
+    }
+
+    @Test
+    public void testParseMacMAC48ToEUI64() {
+        // MAC-48 into an EUI-64
+        assertArrayEquals(new byte[]{0, (byte) 0xaa, 0x11, (byte) 0xff, (byte) 0xff, (byte) 0xbb, 0x22, (byte) 0xcc},
+                parseMAC("00-AA-11-FF-FF-BB-22-CC"));
+        assertArrayEquals(new byte[]{0, (byte) 0xaa, 0x11, (byte) 0xff, (byte) 0xff, (byte) 0xbb, 0x22, (byte) 0xcc},
+                parseMAC("00:AA:11:FF:FF:BB:22:CC"));
+    }
+
+    @Test
+    public void testParseMacEUI48ToEUI64() {
+        // EUI-48 into an EUI-64
+        assertArrayEquals(new byte[]{0, (byte) 0xaa, 0x11, (byte) 0xff, (byte) 0xfe, (byte) 0xbb, 0x22, (byte) 0xcc},
+                parseMAC("00-AA-11-FF-FE-BB-22-CC"));
+        assertArrayEquals(new byte[]{0, (byte) 0xaa, 0x11, (byte) 0xff, (byte) 0xfe, (byte) 0xbb, 0x22, (byte) 0xcc},
+                parseMAC("00:AA:11:FF:FE:BB:22:CC"));
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testParseMacInvalid7HexGroupsA() {
+        parseMAC("00-AA-11-BB-22-CC-FF");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testParseMacInvalid7HexGroupsB() {
+        parseMAC("00:AA:11:BB:22:CC:FF");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testParseMacInvalidEUI48MixedSeparatorA() {
+        parseMAC("00-AA:11-BB-22-CC");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testParseMacInvalidEUI48MixedSeparatorB() {
+        parseMAC("00:AA-11:BB:22:CC");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testParseMacInvalidEUI64MixedSeparatorA() {
+        parseMAC("00-AA-11-FF-FE-BB-22:CC");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testParseMacInvalidEUI64MixedSeparatorB() {
+        parseMAC("00:AA:11:FF:FE:BB:22-CC");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testParseMacInvalidEUI48TrailingSeparatorA() {
+        parseMAC("00-AA-11-BB-22-CC-");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testParseMacInvalidEUI48TrailingSeparatorB() {
+        parseMAC("00:AA:11:BB:22:CC:");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testParseMacInvalidEUI64TrailingSeparatorA() {
+        parseMAC("00-AA-11-FF-FE-BB-22-CC-");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testParseMacInvalidEUI64TrailingSeparatorB() {
+        parseMAC("00:AA:11:FF:FE:BB:22:CC:");
+    }
+}