gn edit: keep build files alive for warnings

Using `gn edit` with something like `remove testonly` results in a
non-fatal warning if the target does not have such attribute, and that
warning is collected in the `EditState` that `RunEditImpl` returns. This
run in UaF as every warning is an `Err` whose `Location` points into the
`InputFile` owned by the `BuildFile` it came from, however with the
that list being scoped to `RunEditImpl`, we end up with `RunEdit`
formatting those warnings using freed memory.

This CL fixes that by hoisting `build_files_` as a sibling field to
`edit_state_`.

Change-Id: I12acd0c02f3196c5f122642fadf6be2184769256
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26700
Reviewed-by: Matt Stark <msta@google.com>
Commit-Queue: Matt Stark <msta@google.com>
Reviewed-by: Takuto Ikuta <tikuta@google.com>

src/gn/command_suggest.cc

diff --git a/src/gn/command_suggest.cc b/src/gn/command_suggest.cc
index 28b21f6..07345ca 100644
--- a/src/gn/command_suggest.cc
+++ b/src/gn/command_suggest.cc
@@ -519,7 +519,8 @@
         }
       }
       if (res != ApplyResult::kAmbiguous) {
-        auto edit_result = RunEditImpl(edit.Args(), *setup);
+        std::vector<BuildFile> build_files;
+        auto edit_result = RunEditImpl(edit.Args(), *setup, build_files);
         if (!edit_result.has_value() ||
             !edit_result.value().second.warnings.empty()) {
           // Warnings should be treated as errors.

src/gn/edit_command.cc

diff --git a/src/gn/edit_command.cc b/src/gn/edit_command.cc
index 62d49a3..c36647b 100644
--- a/src/gn/edit_command.cc
+++ b/src/gn/edit_command.cc
@@ -122,7 +122,8 @@
 
 Result<std::pair<std::vector<SourceFile>, EditState>> RunEditImpl(
     const std::vector<std::string>& args,
-    Setup& setup) {
+    Setup& setup,
+    std::vector<BuildFile>& build_files) {
   if (args.size() < 2) {
     return Err(Location(), "Insufficient arguments.",
                "Usage: gn edit <command> <labels...>\n"
@@ -172,7 +173,7 @@
     patterns.push_back(std::move(pattern));
   }
 
-  ASSIGN_OR_RETURN(std::vector<BuildFile> build_files,
+  ASSIGN_OR_RETURN(build_files,
                    ::ResolvePatternsToBuildFiles(&setup.build_settings(),
                                                  setup.loader(), patterns));
 
@@ -198,7 +199,8 @@
   if (!setup.DoSetupForEditing()) {
     return 1;
   }
-  auto result = RunEditImpl(args, setup);
+  std::vector<BuildFile> build_files;
+  auto result = RunEditImpl(args, setup, build_files);
   if (result.has_error()) {
     result.error().PrintToStdout();
     return 1;

src/gn/edit_command.h

diff --git a/src/gn/edit_command.h b/src/gn/edit_command.h
index c3414d3..f90b255 100644
--- a/src/gn/edit_command.h
+++ b/src/gn/edit_command.h
@@ -12,13 +12,17 @@
 #include "gn/err.h"
 #include "gn/source_file.h"
 
+class BuildFile;
 class Setup;
 namespace commands {
 
 // Runs an edit command, and returns a list of files that were modified.
+//
+// `build_files` is filled with the build files the edit ran against.
 Result<std::pair<std::vector<SourceFile>, EditState>> RunEditImpl(
     const std::vector<std::string>& args,
-    Setup& setup);
+    Setup& setup,
+    std::vector<BuildFile>& build_files);
 
 }  // namespace commands
 

src/gn/edit_command_unittest.cc

diff --git a/src/gn/edit_command_unittest.cc b/src/gn/edit_command_unittest.cc
index d61120d..6d7ab07 100644
--- a/src/gn/edit_command_unittest.cc
+++ b/src/gn/edit_command_unittest.cc
@@ -24,8 +24,11 @@
 std::string Pretty(const Edited& edited);
 
 struct Edited {
-  Edited(std::string_view contents, EditState edit_state = EditState())
+  Edited(std::string_view contents,
+         EditState edit_state = EditState(),
+         std::vector<BuildFile> build_files = {})
       : contents_(contents.starts_with('\n') ? contents.substr(1) : contents),
+        build_files_(std::move(build_files)),
         edit_state_(std::move(edit_state)) {}
 
   bool operator==(const Edited& other) const {
@@ -33,6 +36,8 @@
   }
 
   std::string contents_;
+  // Owns the input files `edit_state_`'s warnings point at.
+  std::vector<BuildFile> build_files_;
   EditState edit_state_;
 };
 
@@ -81,7 +86,8 @@
     args.push_back(std::move(p));
   }
 
-  auto result = RunEditImpl(args, setup);
+  std::vector<BuildFile> build_files;
+  auto result = RunEditImpl(args, setup, build_files);
   if (result.has_error()) {
     return result.error();
   }
@@ -90,7 +96,7 @@
   if (!base::ReadFileToString(build_gn_path, &after)) {
     return Err(Location(), "Failed to read BUILD.gn");
   }
-  return Edited(after, std::move(result->second));
+  return Edited(after, std::move(result->second), std::move(build_files));
 }
 
 // Runs an edit command matching all targets in the root BUILD.gn ("//:*").
@@ -505,6 +511,21 @@
                             "attribute \"nonexistent_attribute\".")}}));
 }
 
+TEST_F(EditCommandTest, WarningLocationOutlivesTheEdit) {
+  // A warning points at the build file it came from, so the parsed file has to
+  // outlive the edit that produced the warning.
+  auto edited = DoEdit("remove nonexistent_attribute", {"//:foo"},
+                       R"(
+executable("foo") {
+}
+)");
+  ASSERT_TRUE(edited.has_value());
+  ASSERT_EQ(edited->edit_state_.warnings.size(), 1u);
+  // The build file above starts with a newline, so the target is on line 2.
+  EXPECT_EQ(edited->edit_state_.warnings[0].location().Describe(true),
+            "//BUILD.gn:2:1");
+}
+
 TEST_F(EditCommandTest, RemoveFromAttributeSubcommand) {
   EXPECT_SUCCESS(DoEdit("remove deps //base :bar",
                         R"(