AAPT2: Fix issue with enums and integer attributes

When an attribute had the format "enum|integer", and a max or min
allowed value set, any value set for this attribute would have its
enum symbol's value checked against the valid integer range.

This would lead to the following:

android:numColumns="autofit"

being interpreted as an integer value of -1, which violated the minimum
expected value for numColumns, which was 0.

Bug: 62358540
Test: make aapt2_tests
Change-Id: I3150410448a533d3595a08ac6b2966264db874d8

6 files changed, 138 insertions, 44 deletions

diff --git a/tools/aapt2/ResourceValues.cpp b/tools/aapt2/ResourceValues.cpp
index e808984c75f5..947e091e2d48 100644
--- a/tools/aapt2/ResourceValues.cpp
+++ b/tools/aapt2/ResourceValues.cpp
@@ -533,75 +533,119 @@ void Attribute::Print(std::ostream* out) const {
   }
 }
 
-static void BuildAttributeMismatchMessage(DiagMessage* msg,
-                                          const Attribute* attr,
-                                          const Item* value) {
-  *msg << "expected";
-  if (attr->type_mask & android::ResTable_map::TYPE_BOOLEAN) {
-    *msg << " boolean";
+static void BuildAttributeMismatchMessage(const Attribute& attr, const Item& value,
+                                          DiagMessage* out_msg) {
+  *out_msg << "expected";
+  if (attr.type_mask & android::ResTable_map::TYPE_BOOLEAN) {
+    *out_msg << " boolean";
   }
 
-  if (attr->type_mask & android::ResTable_map::TYPE_COLOR) {
-    *msg << " color";
+  if (attr.type_mask & android::ResTable_map::TYPE_COLOR) {
+    *out_msg << " color";
   }
 
-  if (attr->type_mask & android::ResTable_map::TYPE_DIMENSION) {
-    *msg << " dimension";
+  if (attr.type_mask & android::ResTable_map::TYPE_DIMENSION) {
+    *out_msg << " dimension";
   }
 
-  if (attr->type_mask & android::ResTable_map::TYPE_ENUM) {
-    *msg << " enum";
+  if (attr.type_mask & android::ResTable_map::TYPE_ENUM) {
+    *out_msg << " enum";
   }
 
-  if (attr->type_mask & android::ResTable_map::TYPE_FLAGS) {
-    *msg << " flags";
+  if (attr.type_mask & android::ResTable_map::TYPE_FLAGS) {
+    *out_msg << " flags";
   }
 
-  if (attr->type_mask & android::ResTable_map::TYPE_FLOAT) {
-    *msg << " float";
+  if (attr.type_mask & android::ResTable_map::TYPE_FLOAT) {
+    *out_msg << " float";
   }
 
-  if (attr->type_mask & android::ResTable_map::TYPE_FRACTION) {
-    *msg << " fraction";
+  if (attr.type_mask & android::ResTable_map::TYPE_FRACTION) {
+    *out_msg << " fraction";
   }
 
-  if (attr->type_mask & android::ResTable_map::TYPE_INTEGER) {
-    *msg << " integer";
+  if (attr.type_mask & android::ResTable_map::TYPE_INTEGER) {
+    *out_msg << " integer";
   }
 
-  if (attr->type_mask & android::ResTable_map::TYPE_REFERENCE) {
-    *msg << " reference";
+  if (attr.type_mask & android::ResTable_map::TYPE_REFERENCE) {
+    *out_msg << " reference";
   }
 
-  if (attr->type_mask & android::ResTable_map::TYPE_STRING) {
-    *msg << " string";
+  if (attr.type_mask & android::ResTable_map::TYPE_STRING) {
+    *out_msg << " string";
   }
 
-  *msg << " but got " << *value;
+  *out_msg << " but got " << value;
 }
 
-bool Attribute::Matches(const Item* item, DiagMessage* out_msg) const {
+bool Attribute::Matches(const Item& item, DiagMessage* out_msg) const {
+  constexpr const uint32_t TYPE_ENUM = android::ResTable_map::TYPE_ENUM;
+  constexpr const uint32_t TYPE_FLAGS = android::ResTable_map::TYPE_FLAGS;
+  constexpr const uint32_t TYPE_INTEGER = android::ResTable_map::TYPE_INTEGER;
+  constexpr const uint32_t TYPE_REFERENCE = android::ResTable_map::TYPE_REFERENCE;
+
   android::Res_value val = {};
-  item->Flatten(&val);
+  item.Flatten(&val);
+
+  const uint32_t flattened_data = util::DeviceToHost32(val.data);
 
   // Always allow references.
-  const uint32_t mask = type_mask | android::ResTable_map::TYPE_REFERENCE;
-  if (!(mask & ResourceUtils::AndroidTypeToAttributeTypeMask(val.dataType))) {
+  const uint32_t actual_type = ResourceUtils::AndroidTypeToAttributeTypeMask(val.dataType);
+
+  // Only one type must match between the actual and expected.
+  if ((actual_type & (type_mask | TYPE_REFERENCE)) == 0) {
     if (out_msg) {
-      BuildAttributeMismatchMessage(out_msg, this, item);
+      BuildAttributeMismatchMessage(*this, item, out_msg);
     }
     return false;
+  }
+
+  // Enums and flags are encoded as integers, so check them first before doing any range checks.
+  if ((type_mask & TYPE_ENUM) != 0 && (actual_type & TYPE_ENUM) != 0) {
+    for (const Symbol& s : symbols) {
+      if (flattened_data == s.value) {
+        return true;
+      }
+    }
+
+    // If the attribute accepts integers, we can't fail here.
+    if ((type_mask & TYPE_INTEGER) == 0) {
+      if (out_msg) {
+        *out_msg << item << " is not a valid enum";
+      }
+      return false;
+    }
+  }
+
+  if ((type_mask & TYPE_FLAGS) != 0 && (actual_type & TYPE_FLAGS) != 0) {
+    uint32_t mask = 0u;
+    for (const Symbol& s : symbols) {
+      mask |= s.value;
+    }
+
+    // Check if the flattened data is covered by the flag bit mask.
+    // If the attribute accepts integers, we can't fail here.
+    if ((mask & flattened_data) == flattened_data) {
+      return true;
+    } else if ((type_mask & TYPE_INTEGER) == 0) {
+      if (out_msg) {
+        *out_msg << item << " is not a valid flag";
+      }
+      return false;
+    }
+  }
 
-  } else if (ResourceUtils::AndroidTypeToAttributeTypeMask(val.dataType) &
-             android::ResTable_map::TYPE_INTEGER) {
-    if (static_cast<int32_t>(util::DeviceToHost32(val.data)) < min_int) {
+  // Finally check the integer range of the value.
+  if ((type_mask & TYPE_INTEGER) != 0 && (actual_type & TYPE_INTEGER) != 0) {
+    if (static_cast<int32_t>(flattened_data) < min_int) {
       if (out_msg) {
-        *out_msg << *item << " is less than minimum integer " << min_int;
+        *out_msg << item << " is less than minimum integer " << min_int;
       }
       return false;
-    } else if (static_cast<int32_t>(util::DeviceToHost32(val.data)) > max_int) {
+    } else if (static_cast<int32_t>(flattened_data) > max_int) {
       if (out_msg) {
-        *out_msg << *item << " is greater than maximum integer " << max_int;
+        *out_msg << item << " is greater than maximum integer " << max_int;
       }
       return false;
     }
diff --git a/tools/aapt2/ResourceValues.h b/tools/aapt2/ResourceValues.h
index ac5795fb9774..7e7547fc1b94 100644
--- a/tools/aapt2/ResourceValues.h
+++ b/tools/aapt2/ResourceValues.h
@@ -264,7 +264,7 @@ struct Attribute : public BaseValue<Attribute> {
   Attribute* Clone(StringPool* new_pool) const override;
   void PrintMask(std::ostream* out) const;
   void Print(std::ostream* out) const override;
-  bool Matches(const Item* item, DiagMessage* out_msg) const;
+  bool Matches(const Item& item, DiagMessage* out_msg = nullptr) const;
 };
 
 struct Style : public BaseValue<Style> {
diff --git a/tools/aapt2/ResourceValues_test.cpp b/tools/aapt2/ResourceValues_test.cpp
index 69f8e67530f6..06c3404561de 100644
--- a/tools/aapt2/ResourceValues_test.cpp
+++ b/tools/aapt2/ResourceValues_test.cpp
@@ -190,4 +190,52 @@ TEST(ResourcesValuesTest, EmptyReferenceFlattens) {
   EXPECT_EQ(0x0u, value.data);
 }
 
+TEST(ResourcesValuesTest, AttributeMatches) {
+  constexpr const uint32_t TYPE_DIMENSION = android::ResTable_map::TYPE_DIMENSION;
+  constexpr const uint32_t TYPE_ENUM = android::ResTable_map::TYPE_ENUM;
+  constexpr const uint32_t TYPE_FLAGS = android::ResTable_map::TYPE_FLAGS;
+  constexpr const uint32_t TYPE_INTEGER = android::ResTable_map::TYPE_INTEGER;
+  constexpr const uint8_t TYPE_INT_DEC = android::Res_value::TYPE_INT_DEC;
+
+  Attribute attr1(false /*weak*/, TYPE_DIMENSION);
+  EXPECT_FALSE(attr1.Matches(*ResourceUtils::TryParseColor("#7fff00")));
+  EXPECT_TRUE(attr1.Matches(*ResourceUtils::TryParseFloat("23dp")));
+  EXPECT_TRUE(attr1.Matches(*ResourceUtils::TryParseReference("@android:string/foo")));
+
+  Attribute attr2(false /*weak*/, TYPE_INTEGER | TYPE_ENUM);
+  attr2.min_int = 0;
+  attr2.symbols.push_back(Attribute::Symbol{Reference(test::ParseNameOrDie("android:id/foo")),
+                                            static_cast<uint32_t>(-1)});
+  EXPECT_FALSE(attr2.Matches(*ResourceUtils::TryParseColor("#7fff00")));
+  EXPECT_TRUE(attr2.Matches(BinaryPrimitive(TYPE_INT_DEC, static_cast<uint32_t>(-1))));
+  EXPECT_TRUE(attr2.Matches(BinaryPrimitive(TYPE_INT_DEC, 1u)));
+  EXPECT_FALSE(attr2.Matches(BinaryPrimitive(TYPE_INT_DEC, static_cast<uint32_t>(-2))));
+
+  Attribute attr3(false /*weak*/, TYPE_INTEGER | TYPE_FLAGS);
+  attr3.max_int = 100;
+  attr3.symbols.push_back(
+      Attribute::Symbol{Reference(test::ParseNameOrDie("android:id/foo")), 0x01u});
+  attr3.symbols.push_back(
+      Attribute::Symbol{Reference(test::ParseNameOrDie("android:id/bar")), 0x02u});
+  attr3.symbols.push_back(
+      Attribute::Symbol{Reference(test::ParseNameOrDie("android:id/baz")), 0x04u});
+  attr3.symbols.push_back(
+      Attribute::Symbol{Reference(test::ParseNameOrDie("android:id/bat")), 0x80u});
+  EXPECT_FALSE(attr3.Matches(*ResourceUtils::TryParseColor("#7fff00")));
+  EXPECT_TRUE(attr3.Matches(BinaryPrimitive(TYPE_INT_DEC, 0x01u | 0x02u)));
+  EXPECT_TRUE(attr3.Matches(BinaryPrimitive(TYPE_INT_DEC, 0x01u | 0x02u | 0x80u)));
+
+  // Not a flag, but a value less than max_int.
+  EXPECT_TRUE(attr3.Matches(BinaryPrimitive(TYPE_INT_DEC, 0x08u)));
+
+  // Not a flag and greater than max_int.
+  EXPECT_FALSE(attr3.Matches(BinaryPrimitive(TYPE_INT_DEC, 127u)));
+
+  Attribute attr4(false /*weak*/, TYPE_ENUM);
+  attr4.symbols.push_back(
+      Attribute::Symbol{Reference(test::ParseNameOrDie("android:id/foo")), 0x01u});
+  EXPECT_TRUE(attr4.Matches(BinaryPrimitive(TYPE_INT_DEC, 0x01u)));
+  EXPECT_FALSE(attr4.Matches(BinaryPrimitive(TYPE_INT_DEC, 0x02u)));
+}
+
 } // namespace aapt
diff --git a/tools/aapt2/integration-tests/AppOne/res/values/styles.xml b/tools/aapt2/integration-tests/AppOne/res/values/styles.xml
index f05845cfab28..19d96c0809db 100644
--- a/tools/aapt2/integration-tests/AppOne/res/values/styles.xml
+++ b/tools/aapt2/integration-tests/AppOne/res/values/styles.xml
@@ -25,6 +25,7 @@
     <style name="Pop">
         <item name="custom">@android:drawable/btn_default</item>
         <item name="android:focusable">true</item>
+        <item name="android:numColumns">auto_fit</item>
     </style>
     <string name="yo">@string/wow</string>
 
diff --git a/tools/aapt2/link/ReferenceLinker.cpp b/tools/aapt2/link/ReferenceLinker.cpp
index a8e510cd6140..414e56eb5dcc 100644
--- a/tools/aapt2/link/ReferenceLinker.cpp
+++ b/tools/aapt2/link/ReferenceLinker.cpp
@@ -109,18 +109,15 @@ class ReferenceLinkerVisitor : public ValueVisitor {
         entry.value->Accept(this);
 
         // Now verify that the type of this item is compatible with the
-        // attribute it
-        // is defined for. We pass `nullptr` as the DiagMessage so that this
-        // check is
-        // fast and we avoid creating a DiagMessage when the match is
-        // successful.
-        if (!symbol->attribute->Matches(entry.value.get(), nullptr)) {
+        // attribute it is defined for. We pass `nullptr` as the DiagMessage so that this
+        // check is fast and we avoid creating a DiagMessage when the match is successful.
+        if (!symbol->attribute->Matches(*entry.value, nullptr)) {
           // The actual type of this item is incompatible with the attribute.
           DiagMessage msg(entry.key.GetSource());
 
           // Call the matches method again, this time with a DiagMessage so we
           // fill in the actual error message.
-          symbol->attribute->Matches(entry.value.get(), &msg);
+          symbol->attribute->Matches(*entry.value, &msg);
           context_->GetDiagnostics()->Error(msg);
           error_ = true;
         }
diff --git a/tools/aapt2/readme.md b/tools/aapt2/readme.md
index 49cb8d4ca827..62945b1aea8d 100644
--- a/tools/aapt2/readme.md
+++ b/tools/aapt2/readme.md
@@ -1,5 +1,9 @@
 # Android Asset Packaging Tool 2.0 (AAPT2) release notes
 
+## Version 2.18
+### `aapt2 ...`
+- Fixed issue where enum values were interpreted as integers and range checked. (bug 62358540)
+
 ## Version 2.17
 ### `aapt2 ...`
 - Fixed issue where symlinks would not be followed when compiling PNGs. (bug 62144459)