Actually fall back from yiaddr to ciaddr.

The initial implementation of toDhcpResults attempted to get the
leased IP address from ciaddr if yiaddr was 0.0.0.0, but it never
actually did so because a) it used == instead of equals(), and b)
the parsing code never populated mClientIp for a DhcpOfferPacket
or DhcpAckPacket.

Fix this and add a test for it.. Technically DHCP does not use
ciaddr (only bootp uses it), but in 5.0 we would use ciaddr if
yiaddr was 0.0.0.0 and a bit more compatibility shouldn't hurt.

Bug: 19704592
Change-Id: I1f58555f0c10b9c576995a6edb759a83d8938ea0

4 files changed, 76 insertions, 13 deletions

diff --git a/services/net/java/android/net/dhcp/DhcpAckPacket.java b/services/net/java/android/net/dhcp/DhcpAckPacket.java
index c0e1d19..334f708 100644
--- a/services/net/java/android/net/dhcp/DhcpAckPacket.java
+++ b/services/net/java/android/net/dhcp/DhcpAckPacket.java
@@ -30,8 +30,8 @@ class DhcpAckPacket extends DhcpPacket {
     private final Inet4Address mSrcIp;
 
     DhcpAckPacket(int transId, short secs, boolean broadcast, Inet4Address serverAddress,
-                  Inet4Address clientIp, byte[] clientMac) {
-        super(transId, secs, INADDR_ANY, clientIp, serverAddress, INADDR_ANY, clientMac, broadcast);
+                  Inet4Address clientIp, Inet4Address yourIp, byte[] clientMac) {
+        super(transId, secs, clientIp, yourIp, serverAddress, INADDR_ANY, clientMac, broadcast);
         mBroadcast = broadcast;
         mSrcIp = serverAddress;
     }
diff --git a/services/net/java/android/net/dhcp/DhcpOfferPacket.java b/services/net/java/android/net/dhcp/DhcpOfferPacket.java
index af41708..7ca7100 100644
--- a/services/net/java/android/net/dhcp/DhcpOfferPacket.java
+++ b/services/net/java/android/net/dhcp/DhcpOfferPacket.java
@@ -32,8 +32,8 @@ class DhcpOfferPacket extends DhcpPacket {
      * Generates a OFFER packet with the specified parameters.
      */
     DhcpOfferPacket(int transId, short secs, boolean broadcast, Inet4Address serverAddress,
-                    Inet4Address clientIp, byte[] clientMac) {
-        super(transId, secs, INADDR_ANY, clientIp, INADDR_ANY, INADDR_ANY, clientMac, broadcast);
+                    Inet4Address clientIp, Inet4Address yourIp, byte[] clientMac) {
+        super(transId, secs, clientIp, yourIp, INADDR_ANY, INADDR_ANY, clientMac, broadcast);
         mSrcIp = serverAddress;
     }
 
diff --git a/services/net/java/android/net/dhcp/DhcpPacket.java b/services/net/java/android/net/dhcp/DhcpPacket.java
index 2a25d30..d42404b 100644
--- a/services/net/java/android/net/dhcp/DhcpPacket.java
+++ b/services/net/java/android/net/dhcp/DhcpPacket.java
@@ -919,7 +919,7 @@ abstract class DhcpPacket {
                 break;
             case DHCP_MESSAGE_TYPE_OFFER:
                 newPacket = new DhcpOfferPacket(
-                    transactionId, secs, broadcast, ipSrc, yourIp, clientMac);
+                    transactionId, secs, broadcast, ipSrc, clientIp, yourIp, clientMac);
                 break;
             case DHCP_MESSAGE_TYPE_REQUEST:
                 newPacket = new DhcpRequestPacket(
@@ -932,7 +932,7 @@ abstract class DhcpPacket {
                 break;
             case DHCP_MESSAGE_TYPE_ACK:
                 newPacket = new DhcpAckPacket(
-                    transactionId, secs, broadcast, ipSrc, yourIp, clientMac);
+                    transactionId, secs, broadcast, ipSrc, clientIp, yourIp, clientMac);
                 break;
             case DHCP_MESSAGE_TYPE_NAK:
                 newPacket = new DhcpNakPacket(
@@ -982,9 +982,9 @@ abstract class DhcpPacket {
      */
     public DhcpResults toDhcpResults() {
         Inet4Address ipAddress = mYourIp;
-        if (ipAddress == Inet4Address.ANY) {
+        if (ipAddress.equals(Inet4Address.ANY)) {
             ipAddress = mClientIp;
-            if (ipAddress == Inet4Address.ANY) {
+            if (ipAddress.equals(Inet4Address.ANY)) {
                 return null;
             }
         }
@@ -1052,7 +1052,7 @@ abstract class DhcpPacket {
         Inet4Address gateway, List<Inet4Address> dnsServers,
         Inet4Address dhcpServerIdentifier, String domainName) {
         DhcpPacket pkt = new DhcpOfferPacket(
-            transactionId, (short) 0, broadcast, serverIpAddr, clientIpAddr, mac);
+            transactionId, (short) 0, broadcast, serverIpAddr, INADDR_ANY, clientIpAddr, mac);
         pkt.mGateway = gateway;
         pkt.mDnsServers = dnsServers;
         pkt.mLeaseTime = timeout;
@@ -1072,7 +1072,7 @@ abstract class DhcpPacket {
         Inet4Address gateway, List<Inet4Address> dnsServers,
         Inet4Address dhcpServerIdentifier, String domainName) {
         DhcpPacket pkt = new DhcpAckPacket(
-            transactionId, (short) 0, broadcast, serverIpAddr, clientIpAddr, mac);
+            transactionId, (short) 0, broadcast, serverIpAddr, INADDR_ANY, clientIpAddr, mac);
         pkt.mGateway = gateway;
         pkt.mDnsServers = dnsServers;
         pkt.mLeaseTime = timeout;
diff --git a/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java b/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java
index f116042..e0e3fcf 100644
--- a/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java
+++ b/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java
@@ -18,6 +18,7 @@ package android.net.dhcp;
 
 import android.net.NetworkUtils;
 import android.net.DhcpResults;
+import android.net.LinkAddress;
 import android.system.OsConstants;
 import android.test.suitebuilder.annotation.SmallTest;
 import junit.framework.TestCase;
@@ -34,19 +35,27 @@ public class DhcpPacketTest extends TestCase {
             (Inet4Address) NetworkUtils.numericToInetAddress("192.0.2.1");
     private static Inet4Address CLIENT_ADDR =
             (Inet4Address) NetworkUtils.numericToInetAddress("192.0.2.234");
+    // Use our own empty address instead of Inet4Address.ANY or INADDR_ANY to ensure that the code
+    // doesn't use == instead of equals when comparing addresses.
+    private static Inet4Address ANY = (Inet4Address) NetworkUtils.numericToInetAddress("0.0.0.0");
+
     private static byte[] CLIENT_MAC = new byte[] { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05 };
 
     class TestDhcpPacket extends DhcpPacket {
         private byte mType;
         // TODO: Make this a map of option numbers to bytes instead.
-        private byte[] mDomainBytes, mVendorInfoBytes, mLeaseTimeBytes;
+        private byte[] mDomainBytes, mVendorInfoBytes, mLeaseTimeBytes, mNetmaskBytes;
 
-        public TestDhcpPacket(byte type) {
-            super(0xdeadbeef, (short) 0, INADDR_ANY, CLIENT_ADDR, INADDR_ANY, INADDR_ANY,
+        public TestDhcpPacket(byte type, Inet4Address clientIp, Inet4Address yourIp) {
+            super(0xdeadbeef, (short) 0, clientIp, yourIp, INADDR_ANY, INADDR_ANY,
                   CLIENT_MAC, true);
             mType = type;
         }
 
+        public TestDhcpPacket(byte type) {
+            this(type, INADDR_ANY, CLIENT_ADDR);
+        }
+
         public TestDhcpPacket setDomainBytes(byte[] domainBytes) {
             mDomainBytes = domainBytes;
             return this;
@@ -62,6 +71,11 @@ public class DhcpPacketTest extends TestCase {
             return this;
         }
 
+        public TestDhcpPacket setNetmaskBytes(byte[] netmaskBytes) {
+            mNetmaskBytes = netmaskBytes;
+            return this;
+        }
+
         public ByteBuffer buildPacket(int encap, short unusedDestUdp, short unusedSrcUdp) {
             ByteBuffer result = ByteBuffer.allocate(MAX_LENGTH);
             fillInPacket(encap, CLIENT_ADDR, SERVER_ADDR,
@@ -80,6 +94,9 @@ public class DhcpPacketTest extends TestCase {
             if (mLeaseTimeBytes != null) {
                 addTlv(buffer, DHCP_LEASE_TIME, mLeaseTimeBytes);
             }
+            if (mNetmaskBytes != null) {
+                addTlv(buffer, DHCP_SUBNET_MASK, mNetmaskBytes);
+            }
             addTlvEnd(buffer);
         }
 
@@ -175,4 +192,50 @@ public class DhcpPacketTest extends TestCase {
         assertLeaseTimeParses(true, -2147483647, 2147483649L * 1000, maxIntPlusOneLease);
         assertLeaseTimeParses(true, DhcpPacket.INFINITE_LEASE, 0, infiniteLease);
     }
+
+    private void checkIpAddress(String expected, Inet4Address clientIp, Inet4Address yourIp,
+                                byte[] netmaskBytes) {
+        checkIpAddress(expected, DHCP_MESSAGE_TYPE_OFFER, clientIp, yourIp, netmaskBytes);
+        checkIpAddress(expected, DHCP_MESSAGE_TYPE_ACK, clientIp, yourIp, netmaskBytes);
+    }
+
+    private void checkIpAddress(String expected, byte type,
+                                Inet4Address clientIp, Inet4Address yourIp,
+                                byte[] netmaskBytes) {
+        ByteBuffer packet = new TestDhcpPacket(type, clientIp, yourIp)
+                .setNetmaskBytes(netmaskBytes)
+                .build();
+        DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_BOOTP);
+        DhcpResults results = offerPacket.toDhcpResults();
+
+        if (expected != null) {
+            LinkAddress expectedAddress = new LinkAddress(expected);
+            assertEquals(expectedAddress, results.ipAddress);
+        } else {
+            assertNull(results);
+        }
+    }
+
+    @SmallTest
+    public void testIpAddress() throws Exception {
+        byte[] slash11Netmask = new byte[] { (byte) 0xff, (byte) 0xe0, 0x00, 0x00 };
+        byte[] slash24Netmask = new byte[] { (byte) 0xff, (byte) 0xff, (byte) 0xff, 0x00 };
+        byte[] invalidNetmask = new byte[] { (byte) 0xff, (byte) 0xfb, (byte) 0xff, 0x00 };
+        Inet4Address example1 = (Inet4Address) NetworkUtils.numericToInetAddress("192.0.2.1");
+        Inet4Address example2 = (Inet4Address) NetworkUtils.numericToInetAddress("192.0.2.43");
+
+        // A packet without any addresses is not valid.
+        checkIpAddress(null, ANY, ANY, slash24Netmask);
+
+        // ClientIP is used iff YourIP is not present.
+        checkIpAddress("192.0.2.1/24", example2, example1, slash24Netmask);
+        checkIpAddress("192.0.2.43/11", example2, ANY, slash11Netmask);
+        checkIpAddress("192.0.2.43/11", ANY, example2, slash11Netmask);
+
+        // Invalid netmasks are ignored.
+        checkIpAddress(null, example2, ANY, invalidNetmask);
+
+        // If there is no netmask, implicit netmasks are used.
+        checkIpAddress("192.0.2.43/24", ANY, example2, null);
+    }
 }