custom levels: fix potential use after free in custom region generation on windows (#4369)

On Windows, the pairs produced by custom regions all become bogus,
probably due to some compiler differences.
Changing the pairs that get stored in the regions to not be pointers
should hopefully resolve this.
To be extra sure, I also pulled some `add_word(0)`s out into locals to
avoid any argument evaluation order shenanigans.

---------

Co-authored-by: Tyler Wilding <xtvaser@gmail.com>

goalc/build_level/jak2/Region.cpp

@@ -41,17 +41,17 @@ void Region::generate_pairs(DataObjectGenerator& gen, const std::vector<size_t>&
   size_t on_inside_byte = 0;
   size_t on_exit_byte = 0;
   if (on_enter.has_value()) {
-    on_enter_byte = gen_pair(gen, *on_enter.value());
+    on_enter_byte = gen_pair(gen, on_enter.value());
   } else {
     gen.link_word_to_symbol("#f", on_enter_slot);
   }
   if (on_inside.has_value()) {
-    on_inside_byte = gen_pair(gen, *on_inside.value());
+    on_inside_byte = gen_pair(gen, on_inside.value());
   } else {
     gen.link_word_to_symbol("#f", on_inside_slot);
   }
   if (on_exit.has_value()) {
-    on_exit_byte = gen_pair(gen, *on_exit.value());
+    on_exit_byte = gen_pair(gen, on_exit.value());
   } else {
     gen.link_word_to_symbol("#f", on_exit_slot);
   }
@@ -75,17 +75,17 @@ size_t Region::generate(DataObjectGenerator& gen) const {
   size_t on_inside_byte = 0;
   size_t on_exit_byte = 0;
   if (on_enter.has_value()) {
-    on_enter_byte = gen_pair(gen, *on_enter.value());
+    on_enter_byte = gen_pair(gen, on_enter.value());
   } else {
     gen.link_word_to_symbol("#f", on_enter_slot);
   }
   if (on_inside.has_value()) {
-    on_inside_byte = gen_pair(gen, *on_inside.value());
+    on_inside_byte = gen_pair(gen, on_inside.value());
   } else {
     gen.link_word_to_symbol("#f", on_inside_slot);
   }
   if (on_exit.has_value()) {
-    on_exit_byte = gen_pair(gen, *on_exit.value());
+    on_exit_byte = gen_pair(gen, on_exit.value());
   } else {
     gen.link_word_to_symbol("#f", on_exit_slot);
   }
@@ -269,22 +269,22 @@ void add_regions_from_json(const nlohmann::json& json,
         region.faces = std::make_optional<std::vector<RegionFaceData>>(arr.faces);
       }
       if (region_json.find("on-enter") != region_json.end()) {
-        region.on_enter = std::make_optional(
-            &reader.read_from_string(region_json.at("on-enter").get<std::string>(), false)
-                 .as_pair()
-                 ->car);
+        region.on_enter =
+            reader.read_from_string(region_json.at("on-enter").get<std::string>(), false)
+                .as_pair()
+                ->car;
       }
       if (region_json.find("on-inside") != region_json.end()) {
-        region.on_inside = std::make_optional(
-            &reader.read_from_string(region_json.at("on-inside").get<std::string>(), false)
-                 .as_pair()
-                 ->car);
+        region.on_inside =
+            reader.read_from_string(region_json.at("on-inside").get<std::string>(), false)
+                .as_pair()
+                ->car;
       }
       if (region_json.find("on-exit") != region_json.end()) {
-        region.on_exit = std::make_optional(
-            &reader.read_from_string(region_json.at("on-exit").get<std::string>(), false)
-                 .as_pair()
-                 ->car);
+        region.on_exit =
+            reader.read_from_string(region_json.at("on-exit").get<std::string>(), false)
+                .as_pair()
+                ->car;
       }
       lg::print(region.print());
     } else {
@@ -298,17 +298,17 @@ std::string Region::print() {
   result += fmt::format("Region {} ({}):\n", id, tree);
   result += fmt::format("  shape: {}\n", shape);
   if (on_enter.has_value()) {
-    result += fmt::format("  on-enter: {}\n", on_enter.value()->print());
+    result += fmt::format("  on-enter: {}\n", on_enter.value().print());
   } else {
     result += fmt::format("  on-enter: #f\n");
   }
   if (on_inside.has_value()) {
-    result += fmt::format("  on-inside: {}\n", on_inside.value()->print());
+    result += fmt::format("  on-inside: {}\n", on_inside.value().print());
   } else {
     result += fmt::format("  on-inside: #f\n");
   }
   if (on_exit.has_value()) {
-    result += fmt::format("  on-exit: {}\n", on_exit.value()->print());
+    result += fmt::format("  on-exit: {}\n", on_exit.value().print());
   } else {
     result += fmt::format("  on-exit: #f\n");
   }

goalc/build_level/jak2/Region.h

@@ -39,9 +39,9 @@ struct Region {
   // (on-inside pair    :offset-assert 8)
   // (on-exit   pair    :offset-assert 12)
   u32 id;
-  std::optional<goos::Object*> on_enter;
-  std::optional<goos::Object*> on_inside;
-  std::optional<goos::Object*> on_exit;
+  std::optional<goos::Object> on_enter;
+  std::optional<goos::Object> on_inside;
+  std::optional<goos::Object> on_exit;
   math::Vector4f trans;
   math::Vector4f bsphere;
   std::string tree;   // target, camera, data, water, city_vis, sample, light, entity

goalc/build_level/jak3/Region.cpp

@@ -41,17 +41,17 @@ void Region::generate_pairs(DataObjectGenerator& gen, const std::vector<size_t>&
   size_t on_inside_byte = 0;
   size_t on_exit_byte = 0;
   if (on_enter.has_value()) {
-    on_enter_byte = gen_pair(gen, *on_enter.value());
+    on_enter_byte = gen_pair(gen, on_enter.value());
   } else {
     gen.link_word_to_symbol("#f", on_enter_slot);
   }
   if (on_inside.has_value()) {
-    on_inside_byte = gen_pair(gen, *on_inside.value());
+    on_inside_byte = gen_pair(gen, on_inside.value());
   } else {
     gen.link_word_to_symbol("#f", on_inside_slot);
   }
   if (on_exit.has_value()) {
-    on_exit_byte = gen_pair(gen, *on_exit.value());
+    on_exit_byte = gen_pair(gen, on_exit.value());
   } else {
     gen.link_word_to_symbol("#f", on_exit_slot);
   }
@@ -75,17 +75,17 @@ size_t Region::generate(DataObjectGenerator& gen) const {
   size_t on_inside_byte = 0;
   size_t on_exit_byte = 0;
   if (on_enter.has_value()) {
-    on_enter_byte = gen_pair(gen, *on_enter.value());
+    on_enter_byte = gen_pair(gen, on_enter.value());
   } else {
     gen.link_word_to_symbol("#f", on_enter_slot);
   }
   if (on_inside.has_value()) {
-    on_inside_byte = gen_pair(gen, *on_inside.value());
+    on_inside_byte = gen_pair(gen, on_inside.value());
   } else {
     gen.link_word_to_symbol("#f", on_inside_slot);
   }
   if (on_exit.has_value()) {
-    on_exit_byte = gen_pair(gen, *on_exit.value());
+    on_exit_byte = gen_pair(gen, on_exit.value());
   } else {
     gen.link_word_to_symbol("#f", on_exit_slot);
   }
@@ -269,22 +269,22 @@ void add_regions_from_json(const nlohmann::json& json,
         region.faces = std::make_optional<std::vector<RegionFaceData>>(arr.faces);
       }
       if (region_json.find("on-enter") != region_json.end()) {
-        region.on_enter = std::make_optional(
-            &reader.read_from_string(region_json.at("on-enter").get<std::string>(), false)
-                 .as_pair()
-                 ->car);
+        region.on_enter =
+            reader.read_from_string(region_json.at("on-enter").get<std::string>(), false)
+                .as_pair()
+                ->car;
       }
       if (region_json.find("on-inside") != region_json.end()) {
-        region.on_inside = std::make_optional(
-            &reader.read_from_string(region_json.at("on-inside").get<std::string>(), false)
-                 .as_pair()
-                 ->car);
+        region.on_inside =
+            reader.read_from_string(region_json.at("on-inside").get<std::string>(), false)
+                .as_pair()
+                ->car;
       }
       if (region_json.find("on-exit") != region_json.end()) {
-        region.on_exit = std::make_optional(
-            &reader.read_from_string(region_json.at("on-exit").get<std::string>(), false)
-                 .as_pair()
-                 ->car);
+        region.on_exit =
+            reader.read_from_string(region_json.at("on-exit").get<std::string>(), false)
+                .as_pair()
+                ->car;
       }
       lg::print(region.print());
     } else {
@@ -298,17 +298,17 @@ std::string Region::print() {
   result += fmt::format("Region {} ({}):\n", id, tree);
   result += fmt::format("  shape: {}\n", shape);
   if (on_enter.has_value()) {
-    result += fmt::format("  on-enter: {}\n", on_enter.value()->print());
+    result += fmt::format("  on-enter: {}\n", on_enter.value().print());
   } else {
     result += fmt::format("  on-enter: #f\n");
   }
   if (on_inside.has_value()) {
-    result += fmt::format("  on-inside: {}\n", on_inside.value()->print());
+    result += fmt::format("  on-inside: {}\n", on_inside.value().print());
   } else {
     result += fmt::format("  on-inside: #f\n");
   }
   if (on_exit.has_value()) {
-    result += fmt::format("  on-exit: {}\n", on_exit.value()->print());
+    result += fmt::format("  on-exit: {}\n", on_exit.value().print());
   } else {
     result += fmt::format("  on-exit: #f\n");
   }

goalc/build_level/jak3/Region.h

@@ -39,9 +39,9 @@ struct Region {
   // (on-inside pair    :offset-assert 8)
   // (on-exit   pair    :offset-assert 12)
   u32 id;
-  std::optional<goos::Object*> on_enter;
-  std::optional<goos::Object*> on_inside;
-  std::optional<goos::Object*> on_exit;
+  std::optional<goos::Object> on_enter;
+  std::optional<goos::Object> on_inside;
+  std::optional<goos::Object> on_exit;
   math::Vector4f trans;
   math::Vector4f bsphere;
   std::string tree;   // target, camera, data, water, city_vis, sample, light, entity

goalc/data_compiler/DataObjectGenerator.cpp

@@ -116,7 +116,8 @@ void DataObjectGenerator::add_goos_obj(
       if (obj.type == goos::ObjectType::INTEGER || obj.type == goos::ObjectType::FLOAT) {
         link_word_to_byte(next_slot, current_offset_bytes() + 2);
       } else {
-        link_word_to_byte(add_word(0), current_offset_bytes() + 2);
+        auto cdr_slot = add_word(0);
+        link_word_to_byte(cdr_slot, current_offset_bytes() + 2);
       }
     }
   }
@@ -146,7 +147,8 @@ int DataObjectGenerator::add_pair(const goos::Object& obj,
             if (current.as_pair()->cdr.is_empty_list()) {
               add_empty_list();
             } else {
-              link_word_to_byte(add_word(0), current_offset_bytes() + 2);
+              auto cdr_slot = add_word(0);
+              link_word_to_byte(cdr_slot, current_offset_bytes() + 2);
             }
           } else {
             ASSERT_MSG(false, fmt::format("error in pair {}: for {}, entity-actor {} not found",
@@ -164,7 +166,8 @@ int DataObjectGenerator::add_pair(const goos::Object& obj,
             if (current.as_pair()->cdr.is_empty_list()) {
               add_empty_list();
             } else {
-              link_word_to_byte(add_word(0), current_offset_bytes() + 2);
+              auto cdr_slot = add_word(0);
+              link_word_to_byte(cdr_slot, current_offset_bytes() + 2);
             }
           } else {
             ASSERT_MSG(false, fmt::format("error in pair {}: for {}, got invalid id {}",