From 42c035982093d6e08201af9572ea43c7cf269adf Mon Sep 17 00:00:00 2001
From: Jason Tedor <jason@tedor.me>
Date: Thu, 2 Feb 2017 21:14:07 -0500
Subject: [PATCH] Do not prefer empty MAC address

Motivation:

When comparing MAC addresses searching for the best MAC address, if
locally-administered address (e.g., from a Docker container) is compared
against an empty MAC address, the empty MAC address will be marked as
preferred. In cases this is the only available MAC address, this leaves
Netty using a random machine ID instead of using a perfectly valid
machine ID from the locally-adminstered address.

Modifications:

This commit modifies the MAC address logic so that the empty MAC address
is not preferred over a locally-administered address. This commit also
simplifies the comparison logic here.

Result:

Empty MAC addresses will not be preferred over locally-administered
addresses thus permitting the default machine ID to be the
locally-adminstered MAC address if it is the only available MAC address.
---
 .../netty/util/internal/MacAddressUtil.java   | 20 ++++----
 .../util/internal/MacAddressUtilTest.java     | 47 +++++++++++++++++++
 2 files changed, 59 insertions(+), 8 deletions(-)

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 41d4964c9e..4ac088750c 100644
--- a/common/src/main/java/io/netty/util/internal/MacAddressUtil.java
+++ b/common/src/main/java/io/netty/util/internal/MacAddressUtil.java
@@ -203,7 +203,8 @@ public final class MacAddressUtil {
     /**
      * @return positive - current is better, 0 - cannot tell from MAC addr, negative - candidate is better.
      */
-    private static int compareAddresses(byte[] current, byte[] candidate) {
+    // visible for testing
+    static int compareAddresses(byte[] current, byte[] candidate) {
         if (candidate == null || candidate.length < EUI48_MAC_ADDRESS_LENGTH) {
             return 1;
         }
@@ -227,20 +228,23 @@ public final class MacAddressUtil {
         }
 
         // Prefer globally unique address.
-        if (current.length == 0 || (current[0] & 2) == 0) {
-            if ((candidate[0] & 2) == 0) {
+        if ((candidate[0] & 2) == 0) {
+            if (current.length != 0 && (current[0] & 2) == 0) {
                 // Both current and candidate are globally unique addresses.
                 return 0;
             } else {
+                // Only candidate is globally unique.
+                return -1;
+            }
+        } else {
+            if (current.length != 0 && (current[0] & 2) == 0) {
                 // Only current 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/common/src/test/java/io/netty/util/internal/MacAddressUtilTest.java b/common/src/test/java/io/netty/util/internal/MacAddressUtilTest.java
index 84d4e1229c..0af14f07ad 100644
--- a/common/src/test/java/io/netty/util/internal/MacAddressUtilTest.java
+++ b/common/src/test/java/io/netty/util/internal/MacAddressUtilTest.java
@@ -17,10 +17,57 @@ package io.netty.util.internal;
 
 import org.junit.Test;
 
+import static io.netty.util.internal.EmptyArrays.EMPTY_BYTES;
 import static io.netty.util.internal.MacAddressUtil.parseMAC;
 import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
 
 public class MacAddressUtilTest {
+    @Test
+    public void testCompareAddresses() {
+        // should not prefer empty address when candidate is not globally unique
+        assertEquals(
+                0,
+                MacAddressUtil.compareAddresses(
+                        EMPTY_BYTES,
+                        new byte[]{(byte) 0x52, (byte) 0x54, (byte) 0x00, (byte) 0xf9, (byte) 0x32, (byte) 0xbd}));
+
+        // only candidate is globally unique
+        assertEquals(
+                -1,
+                MacAddressUtil.compareAddresses(
+                        EMPTY_BYTES,
+                        new byte[]{(byte) 0x50, (byte) 0x54, (byte) 0x00, (byte) 0xf9, (byte) 0x32, (byte) 0xbd}));
+
+        // only candidate is globally unique
+        assertEquals(
+                -1,
+                MacAddressUtil.compareAddresses(
+                        new byte[]{(byte) 0x52, (byte) 0x54, (byte) 0x00, (byte) 0xf9, (byte) 0x32, (byte) 0xbd},
+                        new byte[]{(byte) 0x50, (byte) 0x54, (byte) 0x00, (byte) 0xf9, (byte) 0x32, (byte) 0xbd}));
+
+        // only current is globally unique
+        assertEquals(
+                1,
+                MacAddressUtil.compareAddresses(
+                        new byte[]{(byte) 0x52, (byte) 0x54, (byte) 0x00, (byte) 0xf9, (byte) 0x32, (byte) 0xbd},
+                        EMPTY_BYTES));
+
+        // only current is globally unique
+        assertEquals(
+                1,
+                MacAddressUtil.compareAddresses(
+                        new byte[]{(byte) 0x50, (byte) 0x54, (byte) 0x00, (byte) 0xf9, (byte) 0x32, (byte) 0xbd},
+                        new byte[]{(byte) 0x52, (byte) 0x54, (byte) 0x00, (byte) 0xf9, (byte) 0x32, (byte) 0xbd}));
+
+        // both are globally unique
+        assertEquals(
+                0,
+                MacAddressUtil.compareAddresses(
+                        new byte[]{(byte) 0x50, (byte) 0x54, (byte) 0x00, (byte) 0xf9, (byte) 0x32, (byte) 0xbd},
+                        new byte[]{(byte) 0x50, (byte) 0x55, (byte) 0x01, (byte) 0xfa, (byte) 0x33, (byte) 0xbe}));
+    }
+
     @Test
     public void testParseMacEUI48() {
         assertArrayEquals(new byte[]{0, (byte) 0xaa, 0x11, (byte) 0xbb, 0x22, (byte) 0xcc},