commit b85963d5e1086e0192485f7dd1fe8d5f65fb7294
author Julie Hockett <juliehockett@google.com> Tue Oct 02 13:41:55 2018 -0700
committer Commit Bot <commit-bot@chromium.org> Wed Oct 03 00:03:20 2018 +0000
tree d37d3ff09b6f10019bd930602dc33f721cf05d2d
parent 091169beda92aff21ded3e9dfb392ea12b10c0e0

[functions][not_needed] Copy scope so it isn't overridden

An edge case found an ASAN use-after-free error where a scope value was
getting overridden by the evaluated result. This copies the scope in the
case where it would have been overridden.

Bug: crbug.com/gn/12
Change-Id: I08322fc73e6e459fbe4c5f35e7f5154ded74746c
Reviewed-on: https://gn-review.googlesource.com/c/2920
Reviewed-by: Scott Graham <scottmg@google.com>
Commit-Queue: Julie Hockett <juliehockett@google.com>

tools/gn/functions.cc

diff --git a/tools/gn/functions.cc b/tools/gn/functions.cc
index 7fc8b62..4c423cb 100644
--- a/tools/gn/functions.cc
+++ b/tools/gn/functions.cc
@@ -677,6 +677,7 @@
 
   Value* value = nullptr;  // Value to use, may point to result_value.
   Value result_value;      // Storage for the "evaluate" case.
+  Value scope_value;       // Storage for an evaluated scope.
   const IdentifierNode* identifier = (*args_cur)->AsIdentifier();
   if (identifier) {
     // Optimize the common case where the input scope is an identifier. This
@@ -699,7 +700,13 @@
   // Extract the source scope if different from current one.
   Scope* source = scope;
   if (value->type() == Value::SCOPE) {
-    source = value->scope_value();
+    // Copy the scope value if it will be overridden.
+    if (value == &result_value) {
+      scope_value = Value(nullptr, value->scope_value()->MakeClosure());
+      source = scope_value.scope_value();
+    } else {
+      source = value->scope_value();
+    }
     result_value = (*args_cur)->Execute(scope, err);
     if (err->has_error())
       return Value();

tools/gn/functions_unittest.cc

diff --git a/tools/gn/functions_unittest.cc b/tools/gn/functions_unittest.cc
index 589986c..8725c2e 100644
--- a/tools/gn/functions_unittest.cc
+++ b/tools/gn/functions_unittest.cc
@@ -202,3 +202,15 @@
   reading_from_different_call.parsed()->Execute(setup2.scope(), &err);
   ASSERT_FALSE(err.has_error());
 }
+
+TEST(Functions, NotNeeded) {
+  TestWithScope setup;
+
+  TestParseInput input("not_needed({ a = 1 }, \"*\")");
+  ASSERT_FALSE(input.has_error());
+
+  Err err;
+  input.parsed()->Execute(setup.scope(), &err);
+  ASSERT_FALSE(err.has_error())
+      << err.message() << err.location().Describe(true);
+}