Fail dex verification for duplicate static/instance fields.

Just like we ensure direct and virtual methods don't conflict, check
that static and instance fields also don't conflict.

Test: test.py
Test: 734-duplicate-fields
Change-Id: I3b959bf68a43fda573788ed848976a5fae91f8d1

8 files changed, 150 insertions, 5 deletions

diff --git a/libdexfile/dex/dex_file_verifier.cc b/libdexfile/dex/dex_file_verifier.cc
index d840027286..76828442ea 100644
--- a/libdexfile/dex/dex_file_verifier.cc
+++ b/libdexfile/dex/dex_file_verifier.cc
@@ -315,8 +315,11 @@ class DexFileVerifier {
   bool CheckIntraMethodHandleItem();
   bool CheckIntraTypeList();
   // Check all fields of the given type, reading `encoded_field` entries from `ptr_`.
+  // Check instance fields against duplicates with static fields.
   template <bool kStatic>
-  bool CheckIntraClassDataItemFields(size_t count);
+  bool CheckIntraClassDataItemFields(size_t num_fields,
+                                     ClassAccessor::Field* static_fields,
+                                     size_t num_static_fields);
   // Check direct or virtual methods, reading `encoded_method` entries from `ptr_`.
   // Check virtual methods against duplicates with direct methods.
   bool CheckIntraClassDataItemMethods(size_t num_methods,
@@ -1522,14 +1525,23 @@ bool DexFileVerifier::CheckIntraTypeList() {
 }
 
 template <bool kStatic>
-bool DexFileVerifier::CheckIntraClassDataItemFields(size_t count) {
+bool DexFileVerifier::CheckIntraClassDataItemFields(size_t num_fields,
+                                                    ClassAccessor::Field* static_fields,
+                                                    size_t num_static_fields) {
   constexpr const char* kTypeDescr = kStatic ? "static field" : "instance field";
 
   // We cannot use ClassAccessor::Field yet as it could read beyond the end of the data section.
   const uint8_t* ptr = ptr_;
 
+  // Load the first static field for the check below.
+  size_t remaining_static_fields = num_static_fields;
+  if (remaining_static_fields != 0u) {
+    DCHECK(static_fields != nullptr);
+    static_fields->Read();
+  }
+
   uint32_t prev_index = 0;
-  for (size_t i = 0; i != count; ++i) {
+  for (size_t i = 0; i != num_fields; ++i) {
     uint32_t field_idx_diff, access_flags;
     if (UNLIKELY(!DecodeUnsignedLeb128Checked(&ptr, data_.end(), &field_idx_diff)) ||
         UNLIKELY(!DecodeUnsignedLeb128Checked(&ptr, data_.end(), &access_flags))) {
@@ -1551,6 +1563,29 @@ bool DexFileVerifier::CheckIntraClassDataItemFields(size_t count) {
       return false;
     }
 
+    // For instance fields, we cross reference the field index to make sure
+    // it doesn't match any static fields.
+    if (remaining_static_fields != 0) {
+      // The static fields are already known to be in ascending index order.
+      // So just keep up with the current index.
+      while (true) {
+        const uint32_t static_idx = static_fields->GetIndex();
+        if (static_idx > curr_index) {
+          break;
+        }
+        if (static_idx == curr_index) {
+          ErrorStringPrintf("Found instance field with same index as static field: %u",
+                            curr_index);
+          return false;
+        }
+        --remaining_static_fields;
+        if (remaining_static_fields == 0u) {
+          break;
+        }
+        static_fields->Read();
+      }
+    }
+
     prev_index = curr_index;
   }
 
@@ -1636,10 +1671,17 @@ bool DexFileVerifier::CheckIntraClassDataItem() {
   ptr_ = ptr;
 
   // Check fields.
-  if (!CheckIntraClassDataItemFields</*kStatic=*/ true>(static_fields_size)) {
+  const uint8_t* static_fields_ptr = ptr_;
+  if (!CheckIntraClassDataItemFields</*kStatic=*/ true>(static_fields_size,
+                                                        /*static_fields=*/ nullptr,
+                                                        /*num_static_fields=*/ 0u)) {
     return false;
   }
-  if (!CheckIntraClassDataItemFields</*kStatic=*/ false>(instance_fields_size)) {
+  // Static fields have been checked, so we can now use ClassAccessor::Field to read them again.
+  ClassAccessor::Field static_fields(*dex_file_, static_fields_ptr);
+  if (!CheckIntraClassDataItemFields</*kStatic=*/ false>(instance_fields_size,
+                                                         &static_fields,
+                                                         static_fields_size)) {
     return false;
   }
 
diff --git a/test/734-duplicate-fields/build.py b/test/734-duplicate-fields/build.py
new file mode 100644
index 0000000000..d531de7a5c
--- /dev/null
+++ b/test/734-duplicate-fields/build.py
@@ -0,0 +1,20 @@
+#
+# Copyright (C) 2025 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+
+def build(ctx):
+  if ctx.jvm:
+    return  # The test does not build on JVM
+  ctx.default_build()
diff --git a/test/734-duplicate-fields/expected-stderr.txt b/test/734-duplicate-fields/expected-stderr.txt
new file mode 100644
index 0000000000..e69de29bb2
--- /dev/null
+++ b/test/734-duplicate-fields/expected-stderr.txt
diff --git a/test/734-duplicate-fields/expected-stdout.txt b/test/734-duplicate-fields/expected-stdout.txt
new file mode 100644
index 0000000000..e69de29bb2
--- /dev/null
+++ b/test/734-duplicate-fields/expected-stdout.txt
diff --git a/test/734-duplicate-fields/info.txt b/test/734-duplicate-fields/info.txt
new file mode 100644
index 0000000000..94fefae9e7
--- /dev/null
+++ b/test/734-duplicate-fields/info.txt
@@ -0,0 +1,3 @@
+Regression test for duplicate static/instance fields, where the runtime would
+have non-deterministic behavior. We now fail dex verification in the presence of
+duplicate instance/static fields.
diff --git a/test/734-duplicate-fields/run.py b/test/734-duplicate-fields/run.py
new file mode 100644
index 0000000000..d98d701b6a
--- /dev/null
+++ b/test/734-duplicate-fields/run.py
@@ -0,0 +1,18 @@
+# Copyright 2025 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+
+def run(ctx, args):
+  # Disable prebuild to avoid dex2oat failing with dex file verification.
+  ctx.default_run(args, prebuild=False)
diff --git a/test/734-duplicate-fields/smali-ex/Cls.smali b/test/734-duplicate-fields/smali-ex/Cls.smali
new file mode 100644
index 0000000000..f9c76f3146
--- /dev/null
+++ b/test/734-duplicate-fields/smali-ex/Cls.smali
@@ -0,0 +1,28 @@
+#
+# Copyright (C) 2025 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+.class public LCls;
+.super Ljava/lang/Object;
+
+.method public constructor <init>()V
+.registers 2
+    invoke-direct {p0}, Ljava/lang/Object;-><init>()V
+    const/16 v0, 0x42
+    iput v0, p0, LCls;->myField:I
+    return-void
+.end method
+
+.field public myField:I
+.field public static myField:I = 0x1
diff --git a/test/734-duplicate-fields/src/Main.java b/test/734-duplicate-fields/src/Main.java
new file mode 100644
index 0000000000..0d97d02744
--- /dev/null
+++ b/test/734-duplicate-fields/src/Main.java
@@ -0,0 +1,34 @@
+/*
+ * Copyright (C) 2025 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import dalvik.system.PathClassLoader;
+
+public class Main {
+  static final String TEST_NAME = "734-duplicate-fields";
+
+  static final String SECONDARY_NAME = TEST_NAME + "-ex";
+  static final String SECONDARY_DEX_FILE =
+    System.getenv("DEX_LOCATION") + "/" + SECONDARY_NAME + ".jar";
+
+  public static void main(String[] args) throws Exception {
+    PathClassLoader pcl = new PathClassLoader(SECONDARY_DEX_FILE, Main.class.getClassLoader());
+    try {
+      Class.forName("Cls", true, pcl);
+      throw new Error("Expected ClassNotFoundException");
+    } catch (ClassNotFoundException expected) {
+    }
+  }
+}