From 7984854750224e53e66b175ea5e399b7d307463a Mon Sep 17 00:00:00 2001
From: Ryan Mitchell <rtmitchell@google.com>
Date: Tue, 11 Sep 2018 10:41:09 -0700
Subject: [PATCH] AAPT2: Fail on invalid id names in compiled xml

AAPT2 was not erroring on invalid resource ids created in layouts with
creation syntax. This change causes this to error suring compile.

Bug: 71394154
Test: aapt2_tests
Change-Id: Idf16fb4bd011ed2d65e8c48f7cba0429ead5a055
---
 tools/aapt2/Diagnostics.h                   |  8 ++++++
 tools/aapt2/compile/XmlIdCollector.cpp      | 28 +++++++++++++--------
 tools/aapt2/compile/XmlIdCollector_test.cpp | 10 ++++++++
 3 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/tools/aapt2/Diagnostics.h b/tools/aapt2/Diagnostics.h
index 50e8b33ab9b1b..30deb5555b216 100644
--- a/tools/aapt2/Diagnostics.h
+++ b/tools/aapt2/Diagnostics.h
@@ -133,11 +133,19 @@ class SourcePathDiagnostics : public IDiagnostics {
   void Log(Level level, DiagMessageActual& actual_msg) override {
     actual_msg.source.path = source_.path;
     diag_->Log(level, actual_msg);
+    if (level == Level::Error) {
+      error = true;
+    }
+  }
+
+  bool HadError() {
+    return error;
   }
 
  private:
   Source source_;
   IDiagnostics* diag_;
+  bool error = false;
 
   DISALLOW_COPY_AND_ASSIGN(SourcePathDiagnostics);
 };
diff --git a/tools/aapt2/compile/XmlIdCollector.cpp b/tools/aapt2/compile/XmlIdCollector.cpp
index d61a15af0d853..2199d003bccbc 100644
--- a/tools/aapt2/compile/XmlIdCollector.cpp
+++ b/tools/aapt2/compile/XmlIdCollector.cpp
@@ -21,6 +21,7 @@
 
 #include "ResourceUtils.h"
 #include "ResourceValues.h"
+#include "text/Unicode.h"
 #include "xml/XmlDom.h"
 
 namespace aapt {
@@ -35,8 +36,9 @@ struct IdCollector : public xml::Visitor {
  public:
   using xml::Visitor::Visit;
 
-  explicit IdCollector(std::vector<SourcedResourceName>* out_symbols)
-      : out_symbols_(out_symbols) {}
+  explicit IdCollector(std::vector<SourcedResourceName>* out_symbols,
+                       SourcePathDiagnostics* source_diag) : out_symbols_(out_symbols),
+                                                             source_diag_(source_diag) {}
 
   void Visit(xml::Element* element) override {
     for (xml::Attribute& attr : element->attributes) {
@@ -44,12 +46,16 @@ struct IdCollector : public xml::Visitor {
       bool create = false;
       if (ResourceUtils::ParseReference(attr.value, &name, &create, nullptr)) {
         if (create && name.type == ResourceType::kId) {
-          auto iter = std::lower_bound(out_symbols_->begin(),
-                                       out_symbols_->end(), name, cmp_name);
-          if (iter == out_symbols_->end() || iter->name != name) {
-            out_symbols_->insert(iter,
-                                 SourcedResourceName{name.ToResourceName(),
-                                                     element->line_number});
+          if (!text::IsValidResourceEntryName(name.entry)) {
+            source_diag_->Error(DiagMessage(element->line_number)
+                                   << "id '" << name << "' has an invalid entry name");
+          } else {
+            auto iter = std::lower_bound(out_symbols_->begin(),
+                                         out_symbols_->end(), name, cmp_name);
+            if (iter == out_symbols_->end() || iter->name != name) {
+              out_symbols_->insert(iter, SourcedResourceName{name.ToResourceName(),
+                                                             element->line_number});
+            }
           }
         }
       }
@@ -60,15 +66,17 @@ struct IdCollector : public xml::Visitor {
 
  private:
   std::vector<SourcedResourceName>* out_symbols_;
+  SourcePathDiagnostics* source_diag_;
 };
 
 }  // namespace
 
 bool XmlIdCollector::Consume(IAaptContext* context, xml::XmlResource* xmlRes) {
   xmlRes->file.exported_symbols.clear();
-  IdCollector collector(&xmlRes->file.exported_symbols);
+  SourcePathDiagnostics source_diag(xmlRes->file.source, context->GetDiagnostics());
+  IdCollector collector(&xmlRes->file.exported_symbols, &source_diag);
   xmlRes->root->Accept(&collector);
-  return true;
+  return !source_diag.HadError();
 }
 
 }  // namespace aapt
diff --git a/tools/aapt2/compile/XmlIdCollector_test.cpp b/tools/aapt2/compile/XmlIdCollector_test.cpp
index 98da56d03ae3e..d49af3bf903ce 100644
--- a/tools/aapt2/compile/XmlIdCollector_test.cpp
+++ b/tools/aapt2/compile/XmlIdCollector_test.cpp
@@ -64,4 +64,14 @@ TEST(XmlIdCollectorTest, DontCollectNonIds) {
   EXPECT_TRUE(doc->file.exported_symbols.empty());
 }
 
+TEST(XmlIdCollectorTest, ErrorOnInvalidIds) {
+  std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
+
+  std::unique_ptr<xml::XmlResource> doc =
+      test::BuildXmlDom("<View foo=\"@+id/foo$bar\"/>");
+
+  XmlIdCollector collector;
+  ASSERT_FALSE(collector.Consume(context.get(), doc.get()));
+}
+
 }  // namespace aapt