Consider MethodType/Handle annotations in VisitClassAnnotations

These annotations are possible (due to the DEX spec) but
unexpected in this scenario.

Update dexdump to dump those annotations too.

Update the gtest to not dereference h_klass until we know
it's safe.

Bug: 365807384
Fixes: 365807384
Test: m test-art-host-gtest-art_runtime_tests64
Change-Id: I7fc8ff2bc60f0b7e6af3047ab721b18427ee5f96

6 files changed, 78 insertions, 22 deletions

diff --git a/dexdump/dexdump.cc b/dexdump/dexdump.cc
index e8af600165..ce0eb130fb 100644
--- a/dexdump/dexdump.cc
+++ b/dexdump/dexdump.cc
@@ -47,13 +47,13 @@
 #include "android-base/file.h"
 #include "android-base/logging.h"
 #include "android-base/stringprintf.h"
-
 #include "base/bit_utils.h"
 #include "dex/class_accessor-inl.h"
 #include "dex/code_item_accessors-inl.h"
 #include "dex/dex_file-inl.h"
 #include "dex/dex_file_exception_helpers.h"
 #include "dex/dex_file_loader.h"
+#include "dex/dex_file_structs.h"
 #include "dex/dex_file_types.h"
 #include "dex/dex_instruction-inl.h"
 #include "dexdump_cfg.h"
@@ -578,6 +578,17 @@ static void dumpEncodedValue(const DexFile* pDexFile, const u1** data, u1 type,
       fprintf(gOutFile, "%g", conv.d);
       break;
     }
+    case DexFile::kDexAnnotationMethodType: {
+      const u4 proto_idx = static_cast<u4>(readVarWidth(data, arg, false));
+      const dex::ProtoId& pProtoId = pDexFile->GetProtoId(dex::ProtoIndex(proto_idx));
+      fputs(pDexFile->GetProtoSignature(pProtoId).ToString().c_str(), gOutFile);
+      break;
+    }
+    case DexFile::kDexAnnotationMethodHandle: {
+      const u4 method_handle_idx = static_cast<u4>(readVarWidth(data, arg, false));
+      fprintf(gOutFile, "method_handle@%u", method_handle_idx);
+      break;
+    }
     case DexFile::kDexAnnotationString: {
       const u4 idx = static_cast<u4>(readVarWidth(data, arg, false));
       if (gOptions.outputFormat == OUTPUT_PLAIN) {
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index dd78be3105..2d1bf6f780 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -9602,7 +9602,7 @@ class RecordAnnotationVisitor final : public annotations::AnnotationVisitor {
   RecordAnnotationVisitor() {}
 
   bool ValidateCounts() {
-    if (is_error_) {
+    if (has_error_) {
       return false;
     }
 
@@ -9634,15 +9634,13 @@ class RecordAnnotationVisitor final : public annotations::AnnotationVisitor {
                                names_count_));
     }
 
-    return !is_error_;
+    return !has_error_;
   }
 
-  const std::string& GetErrorMsg() { return error_msg_; }
-
   bool IsRecordAnnotationFound() { return count_ != 0; }
 
   annotations::VisitorStatus VisitAnnotation(const char* descriptor, uint8_t visibility) override {
-    if (is_error_) {
+    if (has_error_) {
       return annotations::VisitorStatus::kVisitBreak;
     }
 
@@ -9664,7 +9662,7 @@ class RecordAnnotationVisitor final : public annotations::AnnotationVisitor {
   annotations::VisitorStatus VisitAnnotationElement(const char* element_name,
                                                     uint8_t type,
                                                     [[maybe_unused]] const JValue& value) override {
-    if (is_error_) {
+    if (has_error_) {
       return annotations::VisitorStatus::kVisitBreak;
     }
 
@@ -9710,7 +9708,7 @@ class RecordAnnotationVisitor final : public annotations::AnnotationVisitor {
                                                uint32_t index,
                                                uint8_t type,
                                                [[maybe_unused]] const JValue& value) override {
-    if (is_error_) {
+    if (has_error_) {
       return annotations::VisitorStatus::kVisitBreak;
     }
     switch (visiting_type_) {
@@ -9794,14 +9792,12 @@ class RecordAnnotationVisitor final : public annotations::AnnotationVisitor {
   }
 
  private:
-  bool is_error_ = false;
   uint32_t count_ = 0;
   uint32_t names_count_ = UINT32_MAX;
   uint32_t types_count_ = UINT32_MAX;
   uint32_t signatures_count_ = UINT32_MAX;
   uint32_t visibilities_count_ = UINT32_MAX;
   uint32_t annotations_count_ = UINT32_MAX;
-  std::string error_msg_;
   RecordElementType visiting_type_;
 
   inline bool ExpectedTypeOrError(uint8_t type,
@@ -9823,11 +9819,6 @@ class RecordAnnotationVisitor final : public annotations::AnnotationVisitor {
     return false;
   }
 
-  void SetErrorMsg(const std::string& msg) {
-    is_error_ = true;
-    error_msg_ = msg;
-  }
-
   DISALLOW_COPY_AND_ASSIGN(RecordAnnotationVisitor);
 };
 
@@ -9871,6 +9862,11 @@ bool ClassLinker::VerifyRecordClass(Handle<mirror::Class> klass, ObjPtr<mirror::
   // optional, but should have the same size if it exists.
   RecordAnnotationVisitor visitor;
   annotations::VisitClassAnnotations(klass, &visitor);
+  if (UNLIKELY(visitor.HasError())) {
+    ThrowClassFormatError(klass.Get(), "%s", visitor.GetErrorMsg().c_str());
+    return false;
+  }
+
   if (!visitor.IsRecordAnnotationFound()) {
     return true;
   }
diff --git a/runtime/dex/dex_file_annotations.cc b/runtime/dex/dex_file_annotations.cc
index 3d334e70fd..a020627f94 100644
--- a/runtime/dex/dex_file_annotations.cc
+++ b/runtime/dex/dex_file_annotations.cc
@@ -26,6 +26,7 @@
 #include "class_linker-inl.h"
 #include "class_root-inl.h"
 #include "dex/dex_file-inl.h"
+#include "dex/dex_file_types.h"
 #include "dex/dex_instruction-inl.h"
 #include "jni/jni_internal.h"
 #include "jvalue-inl.h"
@@ -221,6 +222,8 @@ bool SkipAnnotationValue(const DexFile& dex_file, const uint8_t** annotation_ptr
     case DexFile::kDexAnnotationLong:
     case DexFile::kDexAnnotationFloat:
     case DexFile::kDexAnnotationDouble:
+    case DexFile::kDexAnnotationMethodType:
+    case DexFile::kDexAnnotationMethodHandle:
     case DexFile::kDexAnnotationString:
     case DexFile::kDexAnnotationType:
     case DexFile::kDexAnnotationMethod:
@@ -465,6 +468,11 @@ bool ProcessAnnotationValue(const ClassData& klass,
       primitive_type = Primitive::kPrimBoolean;
       width = 0;
       break;
+    case DexFile::kDexAnnotationMethodType:
+    case DexFile::kDexAnnotationMethodHandle:
+      // These annotations are unexpected here. Don't process them.
+      LOG(ERROR) << StringPrintf("Unexpected annotation of type 0x%02x", value_type);
+      return false;
     case DexFile::kDexAnnotationString: {
       uint32_t index = DexFile::ReadUnsignedInt(annotation, value_arg, false);
       if (result_style == DexFile::kAllRaw) {
@@ -1929,6 +1937,11 @@ static VisitorStatus VisitEncodedValue(const ClassData& klass,
 
   VisitorStatus status =
       VisitElement(visitor, element_name, depth, element_index, annotation_value);
+  if (UNLIKELY(visitor->HasError())) {
+    // Stop visiting since we won't verify the class anyway.
+    return annotations::VisitorStatus::kVisitBreak;
+  }
+
   switch (annotation_value.type_) {
     case DexFile::kDexAnnotationArray: {
       DCHECK(!is_consumed) << " unexpected consumption of array-typed element '" << element_name
@@ -1943,6 +1956,10 @@ static VisitorStatus VisitEncodedValue(const ClassData& klass,
       for (; i < array_size && element_status != VisitorStatus::kVisitBreak; ++i) {
         element_status = VisitEncodedValue(
             klass, dex_file, annotation_ptr, visitor, element_name, next_depth, i);
+        if (UNLIKELY(visitor->HasError())) {
+          // Stop visiting since we won't verify the class anyway.
+          return annotations::VisitorStatus::kVisitBreak;
+        }
       }
       for (; i < array_size; ++i) {
         SkipAnnotationValue(dex_file, annotation_ptr);
@@ -1962,6 +1979,17 @@ static VisitorStatus VisitEncodedValue(const ClassData& klass,
       }
       break;
     }
+    case DexFile::kDexAnnotationMethodType:
+    case DexFile::kDexAnnotationMethodHandle:
+      // kDexAnnotationMethodType and kDexAnnotationMethodHandle return false in order to not
+      // crash the process but they are unexpected here.
+      visitor->SetErrorMsg(StringPrintf(
+          "Encountered unexpected annotation element type 0x%02x of %s for the class %s",
+          annotation_value.type_,
+          element_name,
+          klass.GetRealClass()->PrettyClass().c_str()));
+      // Stop visiting since we won't verify the class anyway.
+      return annotations::VisitorStatus::kVisitBreak;
     default: {
       // kDexAnnotationArray and kDexAnnotationAnnotation are the only 2 known value_types causing
       // ProcessAnnotationValue return false. For other value_types, we shouldn't need to iterate
@@ -2014,6 +2042,10 @@ void VisitClassAnnotations(Handle<mirror::Class> klass, AnnotationVisitor* visit
 
       status = VisitEncodedValue(
           data, dex_file, &annotation, visitor, element_name, /*depth=*/0, /*ignored*/ 0);
+      if (UNLIKELY(visitor->HasError())) {
+        // Encountered an error, bail out since we won't verify the class anyway.
+        return;
+      }
       if (status == VisitorStatus::kVisitBreak) {
         break;
       }
diff --git a/runtime/dex/dex_file_annotations.h b/runtime/dex/dex_file_annotations.h
index 6f6c6ddc1f..29267b89b8 100644
--- a/runtime/dex/dex_file_annotations.h
+++ b/runtime/dex/dex_file_annotations.h
@@ -203,6 +203,21 @@ class AnnotationVisitor {
                                           uint32_t index,
                                           uint8_t type,
                                           const JValue& value) = 0;
+
+  bool HasError() const { return has_error_; }
+  void SetErrorMsg(const std::string& msg) {
+    DCHECK(!has_error_) << "Already had an error set. New error: " << msg
+                        << ", old error: " << error_msg_;
+    has_error_ = true;
+    error_msg_ = msg;
+  }
+  const std::string& GetErrorMsg() const { return error_msg_; }
+
+ protected:
+  // Whether we found an error while visiting the annotations. If true, `error_msg_` will contain
+  // the information about the error.
+  bool has_error_ = false;
+  std::string error_msg_;
 };
 
 // Visit all annotation elements and array elements without creating
diff --git a/runtime/fuzzer_corpus_test.cc b/runtime/fuzzer_corpus_test.cc
index 17520f172e..0475713c96 100644
--- a/runtime/fuzzer_corpus_test.cc
+++ b/runtime/fuzzer_corpus_test.cc
@@ -19,6 +19,7 @@
 #include <unordered_set>
 
 #include "android-base/file.h"
+#include "android-base/macros.h"
 #include "common_runtime_test.h"
 #include "dex/class_accessor-inl.h"
 #include "dex/dex_file_verifier.h"
@@ -96,21 +97,22 @@ class FuzzerCorpusTest : public CommonRuntimeTest {
       art::StackHandleScope<3> scope(soa.Self());
       art::Handle<art::mirror::ClassLoader> h_loader =
           scope.NewHandle(soa.Decode<art::mirror::ClassLoader>(class_loader));
+      art::MutableHandle<art::mirror::Class> h_klass(scope.NewHandle<art::mirror::Class>(nullptr));
+      art::MutableHandle<art::mirror::DexCache> h_dex_cache(
+          scope.NewHandle<art::mirror::DexCache>(nullptr));
 
-      for (ClassAccessor accessor : dex_file.GetClasses()) {
+      for (art::ClassAccessor accessor : dex_file.GetClasses()) {
         const char* descriptor = accessor.GetDescriptor();
-        const art::Handle<art::mirror::Class> h_klass(scope.NewHandle<art::mirror::Class>(
-            class_linker->FindClass(soa.Self(), descriptor, h_loader)));
-        const art::Handle<art::mirror::DexCache> h_dex_cache(
-            scope.NewHandle<art::mirror::DexCache>(h_klass->GetDexCache()));
-
+        h_klass.Assign(class_linker->FindClass(soa.Self(), descriptor, h_loader));
         // Ignore classes that couldn't be loaded since we are looking for crashes during
         // class/method verification.
         if (h_klass == nullptr || h_klass->IsErroneous()) {
+          // Treat as failure to pass verification
+          passed_class_verification = false;
           soa.Self()->ClearException();
           continue;
         }
-
+        h_dex_cache.Assign(h_klass->GetDexCache());
         verifier::FailureKind failure =
             art::verifier::ClassVerifier::VerifyClass(soa.Self(),
                                                       /* verifier_deps= */ nullptr,
diff --git a/tools/fuzzer/class-verifier-corpus/b365807384.dex b/tools/fuzzer/class-verifier-corpus/b365807384.dex
new file mode 100644
index 0000000000..ad88b174ab
--- /dev/null
+++ b/tools/fuzzer/class-verifier-corpus/b365807384.dex