Parcel only the canonical Uri.Part representation, not both.

Before this CL, Uri.AbstractPart's implementation of Parcelable
was parceling and unparceling both the encoded and the decoded
representation. A Uri with inconsistent decoded/encoded
representation of its Parts would have remained inconsistent
across parcel/unparcel cycles. For example, such a Uri's
uri.getDecodedAuthority() might have returned "good.com" while
url.getEncodedAuthority() (used e.g. for toString()) returned
"evil.com".

After this CL, AbstractPart's constructor allows at most one of
the representations to be set (exception: NULL and EMPTY); this
means that no Part instance with inconsistent values can be
constructed via the constructor (e.g. by unparceling parceled
data). The historical parcel representation of a Part with both
values present can no longer be unparceled, which is safe because
Parcel does not guarantee backwards compatibility (the parceled
form must not be persisted across Android version upgrades).

When parceling, only one of the values is now stored, namely
the (canonical) one that was passed to the constructor.

Fixes: 124526860
Test: atest FrameworksCoreTests:android.net.UriTest
Test: Checked that if run before this CL, the added tests
      would fail with a failure along the lines of:
      ComparisonFailure: expected:</foo/[b]> but was:</foo/[a]>
      or (if the first assertion was commented out):
      ComparisonFailure: expected:<[b].com> but was:<[a].com>

Change-Id: I2bc2008e49de5a66641ecdbd8e5354dfa647269d
Merged-In: I2bc2008e49de5a66641ecdbd8e5354dfa647269d
(cherry picked from commit c9afa38f974317b6b0e28410bbc3426a41791660)

2 files changed, 94 insertions, 27 deletions

diff --git a/core/java/android/net/Uri.java b/core/java/android/net/Uri.java
index c3166e91fac7..8cf182b41566 100644
--- a/core/java/android/net/Uri.java
+++ b/core/java/android/net/Uri.java
@@ -1987,17 +1987,26 @@ public abstract class Uri implements Parcelable, Comparable<Uri> {
          * Enum which indicates which representation of a given part we have.
          */
         static class Representation {
-            static final int BOTH = 0;
             static final int ENCODED = 1;
             static final int DECODED = 2;
         }
 
         volatile String encoded;
         volatile String decoded;
+        private final int mCanonicalRepresentation;
 
         AbstractPart(String encoded, String decoded) {
-            this.encoded = encoded;
-            this.decoded = decoded;
+            if (encoded != NOT_CACHED) {
+                this.mCanonicalRepresentation = Representation.ENCODED;
+                this.encoded = encoded;
+                this.decoded = NOT_CACHED;
+            } else if (decoded != NOT_CACHED) {
+                this.mCanonicalRepresentation = Representation.DECODED;
+                this.encoded = NOT_CACHED;
+                this.decoded = decoded;
+            } else {
+                throw new IllegalArgumentException("Neither encoded nor decoded");
+            }
         }
 
         abstract String getEncoded();
@@ -2009,25 +2018,21 @@ public abstract class Uri implements Parcelable, Comparable<Uri> {
         }
 
         final void writeTo(Parcel parcel) {
-            @SuppressWarnings("StringEquality")
-            boolean hasEncoded = encoded != NOT_CACHED;
-
-            @SuppressWarnings("StringEquality")
-            boolean hasDecoded = decoded != NOT_CACHED;
-
-            if (hasEncoded && hasDecoded) {
-                parcel.writeInt(Representation.BOTH);
-                parcel.writeString(encoded);
-                parcel.writeString(decoded);
-            } else if (hasEncoded) {
-                parcel.writeInt(Representation.ENCODED);
-                parcel.writeString(encoded);
-            } else if (hasDecoded) {
-                parcel.writeInt(Representation.DECODED);
-                parcel.writeString(decoded);
+            final String canonicalValue;
+            if (mCanonicalRepresentation == Representation.ENCODED) {
+                canonicalValue = encoded;
+            } else if (mCanonicalRepresentation == Representation.DECODED) {
+                canonicalValue = decoded;
             } else {
-                throw new IllegalArgumentException("Neither encoded nor decoded");
+                throw new IllegalArgumentException("Unknown representation: "
+                    + mCanonicalRepresentation);
+            }
+            if (canonicalValue == NOT_CACHED) {
+                throw new AssertionError("Canonical value not cached ("
+                    + mCanonicalRepresentation + ")");
             }
+            parcel.writeInt(mCanonicalRepresentation);
+            parcel.writeString(canonicalValue);
         }
     }
 
@@ -2059,13 +2064,12 @@ public abstract class Uri implements Parcelable, Comparable<Uri> {
 
         static Part readFrom(Parcel parcel) {
             int representation = parcel.readInt();
+            String value = parcel.readString();
             switch (representation) {
-                case Representation.BOTH:
-                    return from(parcel.readString(), parcel.readString());
                 case Representation.ENCODED:
-                    return fromEncoded(parcel.readString());
+                    return fromEncoded(value);
                 case Representation.DECODED:
-                    return fromDecoded(parcel.readString());
+                    return fromDecoded(value);
                 default:
                     throw new IllegalArgumentException("Unknown representation: "
                             + representation);
@@ -2127,6 +2131,11 @@ public abstract class Uri implements Parcelable, Comparable<Uri> {
         private static class EmptyPart extends Part {
             public EmptyPart(String value) {
                 super(value, value);
+                if (value != null && !value.isEmpty()) {
+                    throw new IllegalArgumentException("Expected empty value, got: " + value);
+                }
+                // Avoid having to re-calculate the non-canonical value.
+                encoded = decoded = value;
             }
 
             @Override
@@ -2245,14 +2254,12 @@ public abstract class Uri implements Parcelable, Comparable<Uri> {
         static PathPart readFrom(Parcel parcel) {
             int representation = parcel.readInt();
             switch (representation) {
-                case Representation.BOTH:
-                    return from(parcel.readString(), parcel.readString());
                 case Representation.ENCODED:
                     return fromEncoded(parcel.readString());
                 case Representation.DECODED:
                     return fromDecoded(parcel.readString());
                 default:
-                    throw new IllegalArgumentException("Bad representation: " + representation);
+                    throw new IllegalArgumentException("Unknown representation: " + representation);
             }
         }
 
diff --git a/core/tests/coretests/src/android/net/UriTest.java b/core/tests/coretests/src/android/net/UriTest.java
index a71000bd220c..f20220c4ab9b 100644
--- a/core/tests/coretests/src/android/net/UriTest.java
+++ b/core/tests/coretests/src/android/net/UriTest.java
@@ -24,6 +24,9 @@ import androidx.test.filters.SmallTest;
 import junit.framework.TestCase;
 
 import java.io.File;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
 import java.util.Arrays;
 import java.util.Iterator;
 import java.util.List;
@@ -816,6 +819,63 @@ public class UriTest extends TestCase {
                 Uri.parse("content://com.example/path%2Fpath")));
     }
 
+
+    /**
+     * Check that calling Part(String, String) with inconsistent Strings does not lead
+     * to the Part's encoded vs. decoded values being inconsistent.
+     */
+    public void testPart_consistentEncodedVsDecoded() throws Exception {
+        Object authority = createPart(Class.forName("android.net.Uri$Part"), "a.com", "b.com");
+        Object path = createPart(Class.forName("android.net.Uri$PathPart"), "/foo/a", "/foo/b");
+        Uri uri = makeHierarchicalUri(authority, path);
+        // In these cases, decoding/encoding the encoded/decoded representation yields the same
+        // String, so we can just assert equality.
+        // assertEquals(uri.getPath(), uri.getEncodedPath());
+        assertEquals(uri.getAuthority(), uri.getEncodedAuthority());
+
+        // When both encoded and decoded strings are given, the encoded one is preferred.
+        assertEquals("a.com", uri.getAuthority());
+        assertEquals("/foo/a", uri.getPath());
+    }
+
+    private Object createPart(Class partClass, String encoded, String decoded) throws Exception {
+        Constructor partConstructor = partClass.getDeclaredConstructor(String.class, String.class);
+        partConstructor.setAccessible(true);
+        return partConstructor.newInstance(encoded, decoded);
+    }
+
+    private static Uri makeHierarchicalUri(Object authority, Object path) throws Exception {
+        Class hierarchicalUriClass = Class.forName("android.net.Uri$HierarchicalUri");
+        Constructor hierarchicalUriConstructor = hierarchicalUriClass.getDeclaredConstructors()[0];
+        hierarchicalUriConstructor.setAccessible(true);
+        return (Uri) hierarchicalUriConstructor.newInstance("https", authority, path, null, null);
+    }
+
+    /** Attempting to unparcel a legacy parcel format of Uri.{,Path}Part should fail. */
+    public void testUnparcelLegacyPart_fails() throws Exception {
+        assertUnparcelLegacyPart_fails(Class.forName("android.net.Uri$Part"));
+        assertUnparcelLegacyPart_fails(Class.forName("android.net.Uri$PathPart"));
+    }
+
+    private static void assertUnparcelLegacyPart_fails(Class partClass) throws Exception {
+        Parcel parcel = Parcel.obtain();
+        parcel.writeInt(0 /* BOTH */);
+        parcel.writeString("encoded");
+        parcel.writeString("decoded");
+        parcel.setDataPosition(0);
+
+        Method readFromMethod = partClass.getDeclaredMethod("readFrom", Parcel.class);
+        readFromMethod.setAccessible(true);
+        try {
+            readFromMethod.invoke(null, parcel);
+            fail();
+        } catch (InvocationTargetException expected) {
+            Throwable targetException = expected.getTargetException();
+            // Check that the exception was thrown for the correct reason.
+            assertEquals("Unknown representation: 0", targetException.getMessage());
+        }
+    }
+
     public void testToSafeString() {
         checkToSafeString("tel:xxxxxx", "tel:Google");
         checkToSafeString("tel:xxxxxxxxxx", "tel:1234567890");