From f9a4c5b33f653e8371dbbceb4836f2932750cb47 Mon Sep 17 00:00:00 2001 From: Hat Kid <6624576+Hat-Kid@users.noreply.github.com> Date: Tue, 28 Jul 2026 03:23:46 +0200 Subject: [PATCH] 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 --- goalc/build_level/jak2/Region.cpp | 42 ++++++++++----------- goalc/build_level/jak2/Region.h | 6 +-- goalc/build_level/jak3/Region.cpp | 42 ++++++++++----------- goalc/build_level/jak3/Region.h | 6 +-- goalc/data_compiler/DataObjectGenerator.cpp | 9 +++-- 5 files changed, 54 insertions(+), 51 deletions(-) diff --git a/goalc/build_level/jak2/Region.cpp b/goalc/build_level/jak2/Region.cpp index ae2c884590..93301fec93 100644 --- a/goalc/build_level/jak2/Region.cpp +++ b/goalc/build_level/jak2/Region.cpp @@ -41,17 +41,17 @@ void Region::generate_pairs(DataObjectGenerator& gen, const std::vector& 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>(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(), false) - .as_pair() - ->car); + region.on_enter = + reader.read_from_string(region_json.at("on-enter").get(), 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(), false) - .as_pair() - ->car); + region.on_inside = + reader.read_from_string(region_json.at("on-inside").get(), 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(), false) - .as_pair() - ->car); + region.on_exit = + reader.read_from_string(region_json.at("on-exit").get(), 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"); } diff --git a/goalc/build_level/jak2/Region.h b/goalc/build_level/jak2/Region.h index 27055b23ee..1146c6c237 100644 --- a/goalc/build_level/jak2/Region.h +++ b/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 on_enter; - std::optional on_inside; - std::optional on_exit; + std::optional on_enter; + std::optional on_inside; + std::optional on_exit; math::Vector4f trans; math::Vector4f bsphere; std::string tree; // target, camera, data, water, city_vis, sample, light, entity diff --git a/goalc/build_level/jak3/Region.cpp b/goalc/build_level/jak3/Region.cpp index c10a00f695..947d2fa87e 100644 --- a/goalc/build_level/jak3/Region.cpp +++ b/goalc/build_level/jak3/Region.cpp @@ -41,17 +41,17 @@ void Region::generate_pairs(DataObjectGenerator& gen, const std::vector& 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>(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(), false) - .as_pair() - ->car); + region.on_enter = + reader.read_from_string(region_json.at("on-enter").get(), 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(), false) - .as_pair() - ->car); + region.on_inside = + reader.read_from_string(region_json.at("on-inside").get(), 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(), false) - .as_pair() - ->car); + region.on_exit = + reader.read_from_string(region_json.at("on-exit").get(), 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"); } diff --git a/goalc/build_level/jak3/Region.h b/goalc/build_level/jak3/Region.h index 8c599c9d5f..bb45b2dd4e 100644 --- a/goalc/build_level/jak3/Region.h +++ b/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 on_enter; - std::optional on_inside; - std::optional on_exit; + std::optional on_enter; + std::optional on_inside; + std::optional on_exit; math::Vector4f trans; math::Vector4f bsphere; std::string tree; // target, camera, data, water, city_vis, sample, light, entity diff --git a/goalc/data_compiler/DataObjectGenerator.cpp b/goalc/data_compiler/DataObjectGenerator.cpp index 6fbf91aeac..3cc2c93e7e 100644 --- a/goalc/data_compiler/DataObjectGenerator.cpp +++ b/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 {}",