am 2893051a: am b0b3d0bc: Fix two parsing bugs in new DHCP client.

* commit '2893051aaed12a511fb4c929ddc30f3d1ace64ea':
  Fix two parsing bugs in new DHCP client.

services/net/java/android/net/dhcp/DhcpPacket.java

@@ -154,6 +154,12 @@ abstract class DhcpPacket {
     protected static final byte DHCP_BROADCAST_ADDRESS = 28;
     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
      */
@@ -226,6 +232,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
      */
@@ -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;
     }

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 =
+    private static Inet4Address SERVER_ADDR = v4Address("192.0.2.1");
-            (Inet4Address) NetworkUtils.numericToInetAddress("192.0.2.1");
+    private static Inet4Address CLIENT_ADDR = v4Address("192.0.2.234");
-    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 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 example1 = v4Address("192.0.2.1");
-        Inet4Address example2 = (Inet4Address) NetworkUtils.numericToInetAddress("192.0.2.43");
+        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);
+    }
 }