Cleanup in TcpInfo

Follow up clean up to address comment in aosp/1157145

Bug: 136162280
Test: atest NetworkStackTests
Change-Id: Ifef8e40b19e51a4dc63345250ece97388d706633

2 files changed, 36 insertions, 28 deletions

diff --git a/src/com/android/networkstack/netlink/TcpInfo.java b/src/com/android/networkstack/netlink/TcpInfo.java
index 7a2adc7..f2812e4 100644
--- a/src/com/android/networkstack/netlink/TcpInfo.java
+++ b/src/com/android/networkstack/netlink/TcpInfo.java
@@ -16,7 +16,6 @@
 package com.android.networkstack.netlink;
 
 import android.util.Log;
-import android.util.Range;
 
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
@@ -25,8 +24,9 @@ import com.android.internal.annotations.VisibleForTesting;
 
 import java.nio.BufferUnderflowException;
 import java.nio.ByteBuffer;
-import java.util.HashMap;
+import java.util.Collections;
 import java.util.LinkedHashMap;
+import java.util.Map;
 import java.util.Objects;
 
 /**
@@ -91,42 +91,44 @@ public class TcpInfo {
     }
 
     private static final String TAG = "TcpInfo";
-    private final LinkedHashMap<Field, Number> mFieldsValues = new LinkedHashMap<Field, Number>();
+    private final Map<Field, Number> mFieldsValues;
 
     private TcpInfo(@NonNull ByteBuffer bytes, int infolen) {
         final int start = bytes.position();
+        final LinkedHashMap<Field, Number> fields = new LinkedHashMap<>();
         for (final Field field : Field.values()) {
             switch (field.size) {
                 case Byte.BYTES:
-                    mFieldsValues.put(field, getByte(bytes, start, infolen));
+                    fields.put(field, getByte(bytes, start, infolen));
                     break;
                 case Integer.BYTES:
-                    mFieldsValues.put(field, getInt(bytes, start, infolen));
+                    fields.put(field, getInt(bytes, start, infolen));
                     break;
                 case Long.BYTES:
-                    mFieldsValues.put(field, getLong(bytes, start, infolen));
+                    fields.put(field, getLong(bytes, start, infolen));
                     break;
                 default:
                     Log.e(TAG, "Unexpected size:" + field.size);
             }
         }
-
+        mFieldsValues = Collections.unmodifiableMap(fields);
     }
 
     @VisibleForTesting
-    TcpInfo(@NonNull HashMap<Field, Number> info) {
+    TcpInfo(@NonNull Map<Field, Number> info) {
+        final LinkedHashMap<Field, Number> fields = new LinkedHashMap<>();
         for (final Field field : Field.values()) {
-            mFieldsValues.put(field, info.get(field));
+            fields.put(field, info.get(field));
         }
+        mFieldsValues = Collections.unmodifiableMap(fields);
     }
 
     /** Parse a TcpInfo from a giving ByteBuffer with a specific length. */
     @Nullable
     public static TcpInfo parse(@NonNull ByteBuffer bytes, int infolen) {
         try {
-            TcpInfo info = new TcpInfo(bytes, infolen);
-            return info;
-        } catch (BufferUnderflowException e) {
+            return new TcpInfo(bytes, infolen);
+        } catch (BufferUnderflowException | IllegalArgumentException e) {
             Log.e(TAG, "parsing error.", e);
             return null;
         }
@@ -135,10 +137,15 @@ public class TcpInfo {
     /**
      * Helper function for handling different struct tcp_info versions in the kernel.
      */
-    private static boolean isValidOffset(int start, int len, int pos, int targetBytes) {
-        final Range a = new Range(start, start + len);
-        final Range b = new Range(pos, pos + targetBytes);
-        return a.contains(b);
+    private static boolean isValidTargetPosition(int start, int len, int pos, int targetBytes)
+            throws IllegalArgumentException {
+        // Equivalent to new Range(start, start + len).contains(new Range(pos, pos + targetBytes))
+        if (len < 0 || targetBytes < 0) throw new IllegalArgumentException();
+        // Check that start < pos < start + len
+        if (pos < start || pos > start + len) return false;
+        // Pos is inside the range and targetBytes is positive. Offset is valid if end of 2nd range
+        // is below end of 1st range.
+        return pos + targetBytes <= start + len;
     }
 
     /** Get value for specific key. */
@@ -149,21 +156,21 @@ public class TcpInfo {
 
     @Nullable
     private static Byte getByte(@NonNull ByteBuffer buffer, int start, int len) {
-        if (!isValidOffset(start, len, buffer.position(), Byte.BYTES)) return null;
+        if (!isValidTargetPosition(start, len, buffer.position(), Byte.BYTES)) return null;
 
         return buffer.get();
     }
 
     @Nullable
     private static Integer getInt(@NonNull ByteBuffer buffer, int start, int len) {
-        if (!isValidOffset(start, len, buffer.position(), Integer.BYTES)) return null;
+        if (!isValidTargetPosition(start, len, buffer.position(), Integer.BYTES)) return null;
 
         return buffer.getInt();
     }
 
     @Nullable
     private static Long getLong(@NonNull ByteBuffer buffer, int start, int len) {
-        if (!isValidOffset(start, len, buffer.position(), Long.BYTES)) return null;
+        if (!isValidTargetPosition(start, len, buffer.position(), Long.BYTES)) return null;
 
         return buffer.getLong();
     }
diff --git a/tests/unit/src/com/android/networkstack/netlink/TcpInfoTest.java b/tests/unit/src/com/android/networkstack/netlink/TcpInfoTest.java
index 415bbe4..16cb59a 100644
--- a/tests/unit/src/com/android/networkstack/netlink/TcpInfoTest.java
+++ b/tests/unit/src/com/android/networkstack/netlink/TcpInfoTest.java
@@ -27,7 +27,8 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 
 import java.nio.ByteBuffer;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.Map;
 
 @RunWith(AndroidJUnit4.class)
 @SmallTest
@@ -92,7 +93,7 @@ public class TcpInfoTest {
     @Test
     public void testParseTcpInfo() {
         final ByteBuffer buffer = ByteBuffer.wrap(TCP_INFO_BYTES);
-        final HashMap<TcpInfo.Field, Number> expected = makeTestTcpInfoHash();
+        final Map<TcpInfo.Field, Number> expected = makeTestTcpInfoHash();
         final TcpInfo parsedInfo = TcpInfo.parse(buffer, TCP_INFO_LENGTH_V1);
 
         assertEquals(parsedInfo, new TcpInfo(expected));
@@ -102,7 +103,7 @@ public class TcpInfoTest {
     public void testValidOffset() {
         final ByteBuffer buffer = ByteBuffer.wrap(TCP_INFO_BYTES);
 
-        final HashMap<TcpInfo.Field, Number> expected = makeShortTestTcpInfoHash();
+        final Map<TcpInfo.Field, Number> expected = makeShortTestTcpInfoHash();
         final TcpInfo parsedInfo = TcpInfo.parse(buffer, SHORT_TEST_TCP_INFO);
 
         assertEquals(parsedInfo, new TcpInfo(expected));
@@ -131,7 +132,7 @@ public class TcpInfoTest {
     @Test
     public void testMalformedTcpInfo() {
         final ByteBuffer buffer = ByteBuffer.wrap(MALFORMED_TCP_INFO_BYTES);
-        final HashMap<TcpInfo.Field, Number> expected = makeShortTestTcpInfoHash();
+        final Map<TcpInfo.Field, Number> expected = makeShortTestTcpInfoHash();
 
         TcpInfo parsedInfo = TcpInfo.parse(buffer, SHORT_TEST_TCP_INFO);
         assertEquals(parsedInfo, new TcpInfo(expected));
@@ -144,7 +145,7 @@ public class TcpInfoTest {
     public void testGetValue() {
         ByteBuffer buffer = ByteBuffer.wrap(TCP_INFO_BYTES);
 
-        final HashMap<TcpInfo.Field, Number> expected = makeShortTestTcpInfoHash();
+        final Map<TcpInfo.Field, Number> expected = makeShortTestTcpInfoHash();
         expected.put(TcpInfo.Field.MAX_PACING_RATE, 10_000L);
         expected.put(TcpInfo.Field.FACKETS, 10);
 
@@ -165,8 +166,8 @@ public class TcpInfoTest {
     }
 
     // Make a TcpInfo contains only first 8 bytes.
-    private HashMap<TcpInfo.Field, Number> makeShortTestTcpInfoHash() {
-        final HashMap<TcpInfo.Field, Number> info = new HashMap<TcpInfo.Field, Number>();
+    private Map<TcpInfo.Field, Number> makeShortTestTcpInfoHash() {
+        final Map<TcpInfo.Field, Number> info = new LinkedHashMap<>();
         info.put(TcpInfo.Field.STATE, (byte) 0x01);
         info.put(TcpInfo.Field.CASTATE, (byte) 0x00);
         info.put(TcpInfo.Field.RETRANSMITS, (byte) 0x00);
@@ -179,8 +180,8 @@ public class TcpInfoTest {
         return info;
     }
 
-    private HashMap<TcpInfo.Field, Number> makeTestTcpInfoHash() {
-        final HashMap<TcpInfo.Field, Number> info = makeShortTestTcpInfoHash();
+    private Map<TcpInfo.Field, Number> makeTestTcpInfoHash() {
+        final Map<TcpInfo.Field, Number> info = makeShortTestTcpInfoHash();
         info.put(TcpInfo.Field.RTO, 1806666);
         info.put(TcpInfo.Field.ATO, 0);
         info.put(TcpInfo.Field.SND_MSS, 1326);