Fix two parsing bugs in new DHCP client.

1. We don't parse PAD options properly, leading in failure to
   parse packets sent by DHCP servers that put the end of options
   marker after pad options and at an odd offset.
2. We get the DhcpResults vendorInfo from the wrong option type
   (60 instead of 43).

Fix these and add unit tests for the offer packets sent by a few
different DHCP servers.

Bug: 21955617
Bug: 22281295
Change-Id: I5d13f1a6a3ff0b53112f18f3db8792fa32ad2da3

2 files changed, 169 insertions, 11 deletions

diff --git a/services/net/java/android/net/dhcp/DhcpPacket.java b/services/net/java/android/net/dhcp/DhcpPacket.java
index d42404b..a91ddb8 100644
--- a/services/net/java/android/net/dhcp/DhcpPacket.java
+++ b/services/net/java/android/net/dhcp/DhcpPacket.java
@@ -155,6 +155,12 @@ abstract class DhcpPacket {
     protected Inet4Address mBroadcastAddress;
 
     /**
+     * DHCP Optional Type: Vendor specific information
+     */
+    protected static final byte DHCP_VENDOR_INFO = 43;
+    protected String mVendorInfo;
+
+    /**
      * DHCP Optional Type: DHCP Requested IP Address
      */
     protected static final byte DHCP_REQUESTED_IP = 50;
@@ -227,6 +233,16 @@ abstract class DhcpPacket {
     protected static final byte DHCP_CLIENT_IDENTIFIER = 61;
 
     /**
+     * DHCP zero-length option code: pad
+     */
+    protected static final byte DHCP_OPTION_PAD = 0x00;
+
+    /**
+     * DHCP zero-length option code: end of options
+     */
+    protected static final byte DHCP_OPTION_END = (byte) 0xff;
+
+    /**
      * The transaction identifier used in this particular DHCP negotiation
      */
     protected final int mTransId;
@@ -676,6 +692,7 @@ abstract class DhcpPacket {
         Inet4Address netMask = null;
         String message = null;
         String vendorId = null;
+        String vendorInfo = null;
         byte[] expectedParams = null;
         String hostName = null;
         String domainName = null;
@@ -810,8 +827,10 @@ abstract class DhcpPacket {
             try {
                 byte optionType = packet.get();
 
-                if (optionType == (byte) 0xFF) {
+                if (optionType == DHCP_OPTION_END) {
                     notFinishedOptions = false;
+                } else if (optionType == DHCP_OPTION_PAD) {
+                    // The pad option doesn't have a length field. Nothing to do.
                 } else {
                     int optionLen = packet.get() & 0xFF;
                     int expectedLen = 0;
@@ -885,6 +904,7 @@ abstract class DhcpPacket {
                             break;
                         case DHCP_VENDOR_CLASS_ID:
                             expectedLen = optionLen;
+                            // Embedded nulls are safe as this does not get passed to netd.
                             vendorId = readAsciiString(packet, optionLen, true);
                             break;
                         case DHCP_CLIENT_IDENTIFIER: { // Client identifier
@@ -892,6 +912,11 @@ abstract class DhcpPacket {
                             packet.get(id);
                             expectedLen = optionLen;
                         } break;
+                        case DHCP_VENDOR_INFO:
+                            expectedLen = optionLen;
+                            // Embedded nulls are safe as this does not get passed to netd.
+                            vendorInfo = readAsciiString(packet, optionLen, true);
+                            break;
                         default:
                             // ignore any other parameters
                             for (int i = 0; i < optionLen; i++) {
@@ -965,6 +990,7 @@ abstract class DhcpPacket {
         newPacket.mT1 = T1;
         newPacket.mT2 = T2;
         newPacket.mVendorId = vendorId;
+        newPacket.mVendorInfo = vendorInfo;
         return newPacket;
     }
 
@@ -1011,7 +1037,7 @@ abstract class DhcpPacket {
         results.dnsServers.addAll(mDnsServers);
         results.domains = mDomainName;
         results.serverAddress = mServerIdentifier;
-        results.vendorInfo = mVendorId;
+        results.vendorInfo = mVendorInfo;
         results.leaseDuration = (mLeaseTime != null) ? mLeaseTime : INFINITE_LEASE;
         return results;
     }
diff --git a/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java b/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java
index e0e3fcf..ba1231f 100644
--- a/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java
+++ b/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java
@@ -25,22 +25,27 @@ import junit.framework.TestCase;
 
 import java.net.Inet4Address;
 import java.nio.ByteBuffer;
+import java.util.ArrayList;
+
+import libcore.util.HexEncoding;
 
 import static android.net.dhcp.DhcpPacket.*;
 
 
 public class DhcpPacketTest extends TestCase {
 
-    private static Inet4Address SERVER_ADDR =
-            (Inet4Address) NetworkUtils.numericToInetAddress("192.0.2.1");
-    private static Inet4Address CLIENT_ADDR =
-            (Inet4Address) NetworkUtils.numericToInetAddress("192.0.2.234");
+    private static Inet4Address SERVER_ADDR = v4Address("192.0.2.1");
+    private static Inet4Address CLIENT_ADDR = v4Address("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 Inet4Address ANY = (Inet4Address) v4Address("0.0.0.0");
 
     private static byte[] CLIENT_MAC = new byte[] { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05 };
 
+    private static final Inet4Address v4Address(String addrString) throws IllegalArgumentException {
+        return (Inet4Address) NetworkUtils.numericToInetAddress(addrString);
+    }
+
     class TestDhcpPacket extends DhcpPacket {
         private byte mType;
         // TODO: Make this a map of option numbers to bytes instead.
@@ -89,7 +94,7 @@ public class DhcpPacketTest extends TestCase {
                 addTlv(buffer, DHCP_DOMAIN_NAME, mDomainBytes);
             }
             if (mVendorInfoBytes != null) {
-                addTlv(buffer, DHCP_VENDOR_CLASS_ID, mVendorInfoBytes);
+                addTlv(buffer, DHCP_VENDOR_INFO, mVendorInfoBytes);
             }
             if (mLeaseTimeBytes != null) {
                 addTlv(buffer, DHCP_LEASE_TIME, mLeaseTimeBytes);
@@ -118,7 +123,7 @@ public class DhcpPacketTest extends TestCase {
                 .build();
         DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_BOOTP);
         assertEquals(expectedDomain, offerPacket.mDomainName);
-        assertEquals(expectedVendorInfo, offerPacket.mVendorId);
+        assertEquals(expectedVendorInfo, offerPacket.mVendorInfo);
     }
 
     @SmallTest
@@ -221,8 +226,8 @@ public class DhcpPacketTest extends TestCase {
         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");
+        Inet4Address example1 = v4Address("192.0.2.1");
+        Inet4Address example2 = v4Address("192.0.2.43");
 
         // A packet without any addresses is not valid.
         checkIpAddress(null, ANY, ANY, slash24Netmask);
@@ -238,4 +243,131 @@ public class DhcpPacketTest extends TestCase {
         // If there is no netmask, implicit netmasks are used.
         checkIpAddress("192.0.2.43/24", ANY, example2, null);
     }
+
+    private void assertDhcpResults(String ipAddress, String gateway, String dnsServersString,
+            String domains, String serverAddress, String vendorInfo, int leaseDuration,
+            boolean hasMeteredHint, DhcpResults dhcpResults) throws Exception {
+        assertEquals(new LinkAddress(ipAddress), dhcpResults.ipAddress);
+        assertEquals(v4Address(gateway), dhcpResults.gateway);
+
+        String[] dnsServerStrings = dnsServersString.split(",");
+        ArrayList dnsServers = new ArrayList();
+        for (String dnsServerString : dnsServerStrings) {
+            dnsServers.add(v4Address(dnsServerString));
+        }
+        assertEquals(dnsServers, dhcpResults.dnsServers);
+
+        assertEquals(domains, dhcpResults.domains);
+        assertEquals(v4Address(serverAddress), dhcpResults.serverAddress);
+        assertEquals(vendorInfo, dhcpResults.vendorInfo);
+        assertEquals(leaseDuration, dhcpResults.leaseDuration);
+        assertEquals(hasMeteredHint, dhcpResults.hasMeteredHint());
+    }
+
+    @SmallTest
+    public void testOffer1() throws Exception {
+        // TODO: Turn all of these into golden files. This will probably require modifying
+        // Android.mk appropriately, making this into an AndroidTestCase, and adding code to read
+        // the golden files from the test APK's assets via mContext.getAssets().
+        final ByteBuffer packet = ByteBuffer.wrap(HexEncoding.decode((
+            // IP header.
+            "451001480000000080118849c0a89003c0a89ff7" +
+            // UDP header.
+            "004300440134dcfa" +
+            // BOOTP header.
+            "02010600c997a63b0000000000000000c0a89ff70000000000000000" +
+            // MAC address.
+            "30766ff2a90c00000000000000000000" +
+            // Server name.
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            // File.
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            // Options
+            "638253633501023604c0a89003330400001c200104fffff0000304c0a89ffe06080808080808080404" +
+            "3a0400000e103b040000189cff00000000000000000000"
+        ).toCharArray(), false));
+
+        DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L3);
+        assertTrue(offerPacket instanceof DhcpOfferPacket);  // Implicitly checks it's non-null.
+        DhcpResults dhcpResults = offerPacket.toDhcpResults();
+        assertDhcpResults("192.168.159.247/20", "192.168.159.254", "8.8.8.8,8.8.4.4",
+                null, "192.168.144.3", null, 7200, false, dhcpResults);
+    }
+
+    @SmallTest
+    public void testOffer2() throws Exception {
+        final ByteBuffer packet = ByteBuffer.wrap(HexEncoding.decode((
+            // IP header.
+            "450001518d0600004011144dc0a82b01c0a82bf7" +
+            // UDP header.
+            "00430044013d9ac7" +
+            // BOOTP header.
+            "02010600dfc23d1f0002000000000000c0a82bf7c0a82b0100000000" +
+            // MAC address.
+            "30766ff2a90c00000000000000000000" +
+            // Server name.
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            // File.
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            // Options
+            "638253633501023604c0a82b01330400000e103a04000007083b0400000c4e0104ffffff00" +
+            "1c04c0a82bff0304c0a82b010604c0a82b012b0f414e44524f49445f4d455445524544ff"
+        ).toCharArray(), false));
+
+        assertEquals(337, packet.limit());
+        DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L3);
+        assertTrue(offerPacket instanceof DhcpOfferPacket);  // Implicitly checks it's non-null.
+        DhcpResults dhcpResults = offerPacket.toDhcpResults();
+        assertDhcpResults("192.168.43.247/24", "192.168.43.1", "192.168.43.1",
+                null, "192.168.43.1", "ANDROID_METERED", 3600, true, dhcpResults);
+        assertTrue(dhcpResults.hasMeteredHint());
+    }
+
+    @SmallTest
+    public void testPadAndOverloadedOptionsOffer() throws Exception {
+        // A packet observed in the real world that is interesting for two reasons:
+        //
+        // 1. It uses pad bytes, which we previously didn't support correctly.
+        // 2. It uses DHCP option overloading, which we don't currently support (but it doesn't
+        //    store any information in the overloaded fields).
+        //
+        // For now, we just check that it parses correctly.
+        final ByteBuffer packet = ByteBuffer.wrap(HexEncoding.decode((
+            // Ethernet header.
+            "b4cef6000000e80462236e300800" +
+            // IP header.
+            "4500014c00000000ff11741701010101ac119876" +
+            // UDP header. TODO: fix invalid checksum (due to MAC address obfuscation).
+            "004300440138ae5a" +
+            // BOOTP header.
+            "020106000fa0059f0000000000000000ac1198760000000000000000" +
+            // MAC address.
+            "b4cef600000000000000000000000000" +
+            // Server name.
+            "ff00000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            // File.
+            "ff00000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            // Options
+            "638253633501023604010101010104ffff000033040000a8c03401030304ac1101010604ac110101" +
+            "0000000000000000000000000000000000000000000000ff000000"
+        ).toCharArray(), false));
+
+        DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L2);
+        assertTrue(offerPacket instanceof DhcpOfferPacket);
+        DhcpResults dhcpResults = offerPacket.toDhcpResults();
+        assertDhcpResults("172.17.152.118/16", "172.17.1.1", "172.17.1.1",
+                null, "1.1.1.1", null, 43200, false, dhcpResults);
+    }
 }