AAPT2: Differentiate between Android and Java package names

Android package names are more strict (ASCII only) than Java package names.
Also fixed an issue where trailing underscores were disallowed in Android
package names.

Bug: 68468089
Test: make aapt2_tests
Change-Id: I1052e9e82b6617db6065ce448d9bf7972bb68d59

tools/aapt2/link/ManifestFixer.cpp

@@ -127,9 +127,9 @@ static bool VerifyManifest(xml::Element* el, SourcePathDiagnostics* diag) {
     diag->Error(DiagMessage(el->line_number)
                 << "attribute 'package' in <manifest> tag must not be a reference");
     return false;
-  } else if (!util::IsJavaPackageName(attr->value)) {
+  } else if (!util::IsAndroidPackageName(attr->value)) {
     diag->Error(DiagMessage(el->line_number)
-                << "attribute 'package' in <manifest> tag is not a valid Java package name: '"
+                << "attribute 'package' in <manifest> tag is not a valid Android package name: '"
                 << attr->value << "'");
     return false;
   }

tools/aapt2/text/Unicode.cpp

@@ -85,7 +85,8 @@ bool IsJavaIdentifier(const StringPiece& str) {
     return false;
   }
 
-  if (!IsXidStart(iter.Next())) {
+  const char32_t first_codepoint = iter.Next();
+  if (!IsXidStart(first_codepoint) && first_codepoint != U'_' && first_codepoint != U'$') {
     return false;
   }
 

tools/aapt2/text/Unicode_test.cpp

@@ -44,10 +44,11 @@ TEST(UnicodeTest, IsXidContinue) {
 TEST(UnicodeTest, IsJavaIdentifier) {
   EXPECT_TRUE(IsJavaIdentifier("FøøBar_12"));
   EXPECT_TRUE(IsJavaIdentifier("Føø$Bar"));
+  EXPECT_TRUE(IsJavaIdentifier("_FøøBar"));
+  EXPECT_TRUE(IsJavaIdentifier("$Føø$Bar"));
 
   EXPECT_FALSE(IsJavaIdentifier("12FøøBar"));
-  EXPECT_FALSE(IsJavaIdentifier("_FøøBar"));
-  EXPECT_FALSE(IsJavaIdentifier("$Føø$Bar"));
+  EXPECT_FALSE(IsJavaIdentifier(".Hello"));
 }
 
 TEST(UnicodeTest, IsValidResourceEntryName) {

tools/aapt2/util/Util.cpp

@@ -24,6 +24,7 @@
 #include "androidfw/StringPiece.h"
 #include "utils/Unicode.h"
 
+#include "text/Unicode.h"
 #include "text/Utf8Iterator.h"
 #include "util/BigBuffer.h"
 #include "util/Maybe.h"
@@ -94,72 +95,55 @@ StringPiece TrimWhitespace(const StringPiece& str) {
   return StringPiece(start, end - start);
 }
 
-StringPiece::const_iterator FindNonAlphaNumericAndNotInSet(
-    const StringPiece& str, const StringPiece& allowed_chars) {
-  const auto end_iter = str.end();
-  for (auto iter = str.begin(); iter != end_iter; ++iter) {
-    char c = *iter;
-    if ((c >= u'a' && c <= u'z') || (c >= u'A' && c <= u'Z') ||
-        (c >= u'0' && c <= u'9')) {
-      continue;
-    }
-
-    bool match = false;
-    for (char i : allowed_chars) {
-      if (c == i) {
-        match = true;
-        break;
-      }
-    }
-
-    if (!match) {
-      return iter;
+static int IsJavaNameImpl(const StringPiece& str) {
+  int pieces = 0;
+  for (const StringPiece& piece : Tokenize(str, '.')) {
+    pieces++;
+    if (!text::IsJavaIdentifier(piece)) {
+      return -1;
     }
   }
-  return end_iter;
+  return pieces;
 }
 
 bool IsJavaClassName(const StringPiece& str) {
-  size_t pieces = 0;
-  for (const StringPiece& piece : Tokenize(str, '.')) {
-    pieces++;
-    if (piece.empty()) {
-      return false;
-    }
-
-    // Can't have starting or trailing $ character.
-    if (piece.data()[0] == '$' || piece.data()[piece.size() - 1] == '$') {
-      return false;
-    }
-
-    if (FindNonAlphaNumericAndNotInSet(piece, "$_") != piece.end()) {
-      return false;
-    }
-  }
-  return pieces >= 2;
+  return IsJavaNameImpl(str) >= 2;
 }
 
 bool IsJavaPackageName(const StringPiece& str) {
-  if (str.empty()) {
-    return false;
-  }
+  return IsJavaNameImpl(str) >= 1;
+}
 
-  size_t pieces = 0;
+static int IsAndroidNameImpl(const StringPiece& str) {
+  int pieces = 0;
   for (const StringPiece& piece : Tokenize(str, '.')) {
-    pieces++;
     if (piece.empty()) {
-      return false;
+      return -1;
     }
 
-    if (piece.data()[0] == '_' || piece.data()[piece.size() - 1] == '_') {
-      return false;
+    const char first_character = piece.data()[0];
+    if (!::isalpha(first_character)) {
+      return -1;
     }
 
-    if (FindNonAlphaNumericAndNotInSet(piece, "_") != piece.end()) {
-      return false;
+    bool valid = std::all_of(piece.begin() + 1, piece.end(), [](const char c) -> bool {
+      return ::isalnum(c) || c == '_';
+    });
+
+    if (!valid) {
+      return -1;
     }
+    pieces++;
   }
-  return pieces >= 1;
+  return pieces;
+}
+
+bool IsAndroidPackageName(const StringPiece& str) {
+  return IsAndroidNameImpl(str) > 1 || str == "android";
+}
+
+bool IsAndroidSplitName(const StringPiece& str) {
+  return IsAndroidNameImpl(str) > 0;
 }
 
 Maybe<std::string> GetFullyQualifiedClassName(const StringPiece& package,
@@ -176,7 +160,7 @@ Maybe<std::string> GetFullyQualifiedClassName(const StringPiece& package,
     return {};
   }
 
-  std::string result(package.data(), package.size());
+  std::string result = package.to_string();
   if (classname.data()[0] != '.') {
     result += '.';
   }

tools/aapt2/util/Util.h

@@ -53,48 +53,40 @@ struct Range {
 std::vector<std::string> Split(const android::StringPiece& str, char sep);
 std::vector<std::string> SplitAndLowercase(const android::StringPiece& str, char sep);
 
-/**
- * Returns true if the string starts with prefix.
- */
+// Returns true if the string starts with prefix.
 bool StartsWith(const android::StringPiece& str, const android::StringPiece& prefix);
 
-/**
- * Returns true if the string ends with suffix.
- */
+// Returns true if the string ends with suffix.
 bool EndsWith(const android::StringPiece& str, const android::StringPiece& suffix);
 
-/**
- * Creates a new StringPiece16 that points to a substring
- * of the original string without leading or trailing whitespace.
- */
+// Creates a new StringPiece16 that points to a substring of the original string without leading or
+// trailing whitespace.
 android::StringPiece TrimWhitespace(const android::StringPiece& str);
 
-/**
- * Returns an iterator to the first character that is not alpha-numeric and that
- * is not in the allowedChars set.
- */
-android::StringPiece::const_iterator FindNonAlphaNumericAndNotInSet(
-    const android::StringPiece& str, const android::StringPiece& allowed_chars);
-
-/**
- * Tests that the string is a valid Java class name.
- */
+// Tests that the string is a valid Java class name.
 bool IsJavaClassName(const android::StringPiece& str);
 
-/**
- * Tests that the string is a valid Java package name.
- */
+// Tests that the string is a valid Java package name.
 bool IsJavaPackageName(const android::StringPiece& str);
 
-/**
- * Converts the class name to a fully qualified class name from the given
- * `package`. Ex:
- *
- * asdf         --> package.asdf
- * .asdf        --> package.asdf
- * .a.b         --> package.a.b
- * asdf.adsf    --> asdf.adsf
- */
+// Tests that the string is a valid Android package name. More strict than a Java package name.
+// - First character of each component (separated by '.') must be an ASCII letter.
+// - Subsequent characters of a component can be ASCII alphanumeric or an underscore.
+// - Package must contain at least two components, unless it is 'android'.
+bool IsAndroidPackageName(const android::StringPiece& str);
+
+// Tests that the string is a valid Android split name.
+// - First character of each component (separated by '.') must be an ASCII letter.
+// - Subsequent characters of a component can be ASCII alphanumeric or an underscore.
+bool IsAndroidSplitName(const android::StringPiece& str);
+
+// Converts the class name to a fully qualified class name from the given
+// `package`. Ex:
+//
+// asdf         --> package.asdf
+// .asdf        --> package.asdf
+// .a.b         --> package.a.b
+// asdf.adsf    --> asdf.adsf
 Maybe<std::string> GetFullyQualifiedClassName(const android::StringPiece& package,
                                               const android::StringPiece& class_name);
 
@@ -108,23 +100,17 @@ typename std::enable_if<std::is_arithmetic<T>::value, int>::type compare(const T
   return 0;
 }
 
-/**
- * Makes a std::unique_ptr<> with the template parameter inferred by the compiler.
- * This will be present in C++14 and can be removed then.
- */
+// Makes a std::unique_ptr<> with the template parameter inferred by the compiler.
+// This will be present in C++14 and can be removed then.
 template <typename T, class... Args>
 std::unique_ptr<T> make_unique(Args&&... args) {
   return std::unique_ptr<T>(new T{std::forward<Args>(args)...});
 }
 
-/**
- * Writes a set of items to the std::ostream, joining the times with the
- * provided
- * separator.
- */
+// Writes a set of items to the std::ostream, joining the times with the provided separator.
 template <typename Container>
-::std::function<::std::ostream&(::std::ostream&)> Joiner(
-    const Container& container, const char* sep) {
+::std::function<::std::ostream&(::std::ostream&)> Joiner(const Container& container,
+                                                         const char* sep) {
   using std::begin;
   using std::end;
   const auto begin_iter = begin(container);
@@ -140,32 +126,19 @@ template <typename Container>
   };
 }
 
-/**
- * Helper method to extract a UTF-16 string from a StringPool. If the string is
- * stored as UTF-8,
- * the conversion to UTF-16 happens within ResStringPool.
- */
+// Helper method to extract a UTF-16 string from a StringPool. If the string is stored as UTF-8,
+// the conversion to UTF-16 happens within ResStringPool.
 android::StringPiece16 GetString16(const android::ResStringPool& pool, size_t idx);
 
-/**
- * Helper method to extract a UTF-8 string from a StringPool. If the string is
- * stored as UTF-16,
- * the conversion from UTF-16 to UTF-8 does not happen in ResStringPool and is
- * done by this method,
- * which maintains no state or cache. This means we must return an std::string
- * copy.
- */
+// Helper method to extract a UTF-8 string from a StringPool. If the string is stored as UTF-16,
+// the conversion from UTF-16 to UTF-8 does not happen in ResStringPool and is done by this method,
+// which maintains no state or cache. This means we must return an std::string copy.
 std::string GetString(const android::ResStringPool& pool, size_t idx);
 
-/**
- * Checks that the Java string format contains no non-positional arguments
- * (arguments without
- * explicitly specifying an index) when there are more than one argument. This
- * is an error
- * because translations may rearrange the order of the arguments in the string,
- * which will
- * break the string interpolation.
- */
+// Checks that the Java string format contains no non-positional arguments (arguments without
+// explicitly specifying an index) when there are more than one argument. This is an error
+// because translations may rearrange the order of the arguments in the string, which will
+// break the string interpolation.
 bool VerifyJavaStringFormat(const android::StringPiece& str);
 
 class StringBuilder {
@@ -194,36 +167,38 @@ class StringBuilder {
   std::string error_;
 };
 
-inline const std::string& StringBuilder::ToString() const { return str_; }
+inline const std::string& StringBuilder::ToString() const {
+  return str_;
+}
 
-inline const std::string& StringBuilder::Error() const { return error_; }
+inline const std::string& StringBuilder::Error() const {
+  return error_;
+}
 
-inline bool StringBuilder::IsEmpty() const { return str_.empty(); }
+inline bool StringBuilder::IsEmpty() const {
+  return str_.empty();
+}
 
-inline size_t StringBuilder::Utf16Len() const { return utf16_len_; }
+inline size_t StringBuilder::Utf16Len() const {
+  return utf16_len_;
+}
 
-inline StringBuilder::operator bool() const { return error_.empty(); }
+inline StringBuilder::operator bool() const {
+  return error_.empty();
+}
 
-/**
- * Converts a UTF8 string to a UTF16 string.
- */
+// Converts a UTF8 string to a UTF16 string.
 std::u16string Utf8ToUtf16(const android::StringPiece& utf8);
 std::string Utf16ToUtf8(const android::StringPiece16& utf16);
 
-/**
- * Writes the entire BigBuffer to the output stream.
- */
+// Writes the entire BigBuffer to the output stream.
 bool WriteAll(std::ostream& out, const BigBuffer& buffer);
 
-/*
- * Copies the entire BigBuffer into a single buffer.
- */
+// Copies the entire BigBuffer into a single buffer.
 std::unique_ptr<uint8_t[]> Copy(const BigBuffer& buffer);
 
-/**
- * A Tokenizer implemented as an iterable collection. It does not allocate
- * any memory on the heap nor use standard containers.
- */
+// A Tokenizer implemented as an iterable collection. It does not allocate any memory on the heap
+// nor use standard containers.
 class Tokenizer {
  public:
   class iterator {
@@ -269,38 +244,42 @@ class Tokenizer {
   const iterator end_;
 };
 
-inline Tokenizer Tokenize(const android::StringPiece& str, char sep) { return Tokenizer(str, sep); }
+inline Tokenizer Tokenize(const android::StringPiece& str, char sep) {
+  return Tokenizer(str, sep);
+}
 
-inline uint16_t HostToDevice16(uint16_t value) { return htods(value); }
+inline uint16_t HostToDevice16(uint16_t value) {
+  return htods(value);
+}
 
-inline uint32_t HostToDevice32(uint32_t value) { return htodl(value); }
+inline uint32_t HostToDevice32(uint32_t value) {
+  return htodl(value);
+}
 
-inline uint16_t DeviceToHost16(uint16_t value) { return dtohs(value); }
+inline uint16_t DeviceToHost16(uint16_t value) {
+  return dtohs(value);
+}
 
-inline uint32_t DeviceToHost32(uint32_t value) { return dtohl(value); }
+inline uint32_t DeviceToHost32(uint32_t value) {
+  return dtohl(value);
+}
 
-/**
- * Given a path like: res/xml-sw600dp/foo.xml
- *
- * Extracts "res/xml-sw600dp/" into outPrefix.
- * Extracts "foo" into outEntry.
- * Extracts ".xml" into outSuffix.
- *
- * Returns true if successful.
- */
+// Given a path like: res/xml-sw600dp/foo.xml
+//
+// Extracts "res/xml-sw600dp/" into outPrefix.
+// Extracts "foo" into outEntry.
+// Extracts ".xml" into outSuffix.
+//
+// Returns true if successful.
 bool ExtractResFilePathParts(const android::StringPiece& path, android::StringPiece* out_prefix,
                              android::StringPiece* out_entry, android::StringPiece* out_suffix);
 
 }  // namespace util
 
-/**
- * Stream operator for functions. Calls the function with the stream as an
- * argument.
- * In the aapt namespace for lookup.
- */
-inline ::std::ostream& operator<<(
-    ::std::ostream& out,
-    const ::std::function<::std::ostream&(::std::ostream&)>& f) {
+// Stream operator for functions. Calls the function with the stream as an argument.
+// In the aapt namespace for lookup.
+inline ::std::ostream& operator<<(::std::ostream& out,
+                                  const ::std::function<::std::ostream&(::std::ostream&)>& f) {
   return f(out);
 }
 

tools/aapt2/util/Util_test.cpp

@@ -117,24 +117,46 @@ TEST(UtilTest, IsJavaClassName) {
   EXPECT_TRUE(util::IsJavaClassName("android.test.Class$Inner"));
   EXPECT_TRUE(util::IsJavaClassName("android_test.test.Class"));
   EXPECT_TRUE(util::IsJavaClassName("_android_.test._Class_"));
-  EXPECT_FALSE(util::IsJavaClassName("android.test.$Inner"));
-  EXPECT_FALSE(util::IsJavaClassName("android.test.Inner$"));
+  EXPECT_TRUE(util::IsJavaClassName("android.test.$Inner"));
+  EXPECT_TRUE(util::IsJavaClassName("android.test.Inner$"));
+  EXPECT_TRUE(util::IsJavaClassName("com.foo.FøøBar"));
+
   EXPECT_FALSE(util::IsJavaClassName(".test.Class"));
   EXPECT_FALSE(util::IsJavaClassName("android"));
+  EXPECT_FALSE(util::IsJavaClassName("FooBar"));
 }
 
 TEST(UtilTest, IsJavaPackageName) {
   EXPECT_TRUE(util::IsJavaPackageName("android"));
   EXPECT_TRUE(util::IsJavaPackageName("android.test"));
   EXPECT_TRUE(util::IsJavaPackageName("android.test_thing"));
-  EXPECT_FALSE(util::IsJavaPackageName("_android"));
-  EXPECT_FALSE(util::IsJavaPackageName("android_"));
+  EXPECT_TRUE(util::IsJavaPackageName("_android"));
+  EXPECT_TRUE(util::IsJavaPackageName("android_"));
+  EXPECT_TRUE(util::IsJavaPackageName("android._test"));
+  EXPECT_TRUE(util::IsJavaPackageName("cøm.foo"));
+
   EXPECT_FALSE(util::IsJavaPackageName("android."));
   EXPECT_FALSE(util::IsJavaPackageName(".android"));
-  EXPECT_FALSE(util::IsJavaPackageName("android._test"));
   EXPECT_FALSE(util::IsJavaPackageName(".."));
 }
 
+TEST(UtilTest, IsAndroidPackageName) {
+  EXPECT_TRUE(util::IsAndroidPackageName("android"));
+  EXPECT_TRUE(util::IsAndroidPackageName("android.test"));
+  EXPECT_TRUE(util::IsAndroidPackageName("com.foo"));
+  EXPECT_TRUE(util::IsAndroidPackageName("com.foo.test_thing"));
+  EXPECT_TRUE(util::IsAndroidPackageName("com.foo.testing_thing_"));
+  EXPECT_TRUE(util::IsAndroidPackageName("com.foo.test_99_"));
+
+  EXPECT_FALSE(util::IsAndroidPackageName("android._test"));
+  EXPECT_FALSE(util::IsAndroidPackageName("com"));
+  EXPECT_FALSE(util::IsAndroidPackageName("_android"));
+  EXPECT_FALSE(util::IsAndroidPackageName("android."));
+  EXPECT_FALSE(util::IsAndroidPackageName(".android"));
+  EXPECT_FALSE(util::IsAndroidPackageName(".."));
+  EXPECT_FALSE(util::IsAndroidPackageName("cøm.foo"));
+}
+
 TEST(UtilTest, FullyQualifiedClassName) {
   EXPECT_THAT(util::GetFullyQualifiedClassName("android", ".asdf"), Eq("android.asdf"));
   EXPECT_THAT(util::GetFullyQualifiedClassName("android", ".a.b"), Eq("android.a.b"));