From b6f98485ca10c7ac528d87a8cf68e81f690abcd2 Mon Sep 17 00:00:00 2001 From: nonoge <2359216537tjf@gmail.com> Date: Sun, 5 Apr 2026 00:52:00 +0800 Subject: [PATCH] fix compileoptions includer use after free CompileOptions stores include callbacks inside the cloned or moved C options object but originally kept the C++ includer under unique ownership. Copying only cloned options_ and moving only transferred options_, so the registered include user_data pointer could outlive the owning CompileOptions object and become dangling after destruction. Keep the includer alive across copies and moves by storing it in a shared_ptr and re-registering the callback user_data whenever CompileOptions is copied or moved. Add regression tests that verify the includer remains alive after the source object is destroyed and that preprocessing with #include still succeeds. --- libshaderc/include/shaderc/shaderc.hpp | 52 ++++++++++------ libshaderc/src/shaderc_cpp_test.cc | 84 ++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 20 deletions(-) diff --git a/libshaderc/include/shaderc/shaderc.hpp b/libshaderc/include/shaderc/shaderc.hpp index 77ccd7a70..042e7ad9b 100644 --- a/libshaderc/include/shaderc/shaderc.hpp +++ b/libshaderc/include/shaderc/shaderc.hpp @@ -139,14 +139,17 @@ using PreprocessedSourceCompilationResult = CompilationResult; // Contains any options that can have default values for a compilation. class CompileOptions { public: - CompileOptions() { options_ = shaderc_compile_options_initialize(); } + CompileOptions() : options_(shaderc_compile_options_initialize()) {} ~CompileOptions() { shaderc_compile_options_release(options_); } - CompileOptions(const CompileOptions& other) { - options_ = shaderc_compile_options_clone(other.options_); + CompileOptions(const CompileOptions& other) + : options_(shaderc_compile_options_clone(other.options_)), + includer_(other.includer_) { + if (includer_) RegisterIncluderCallbacks(); } - CompileOptions(CompileOptions&& other) { - options_ = other.options_; + CompileOptions(CompileOptions&& other) + : options_(other.options_), includer_(std::move(other.includer_)) { other.options_ = nullptr; + if (includer_) RegisterIncluderCallbacks(); } // Adds a predefined macro to the compilation options. It behaves the same as @@ -198,20 +201,12 @@ class CompileOptions { // are routed to this includer's methods. void SetIncluder(std::unique_ptr&& includer) { includer_ = std::move(includer); - shaderc_compile_options_set_include_callbacks( - options_, - [](void* user_data, const char* requested_source, int type, - const char* requesting_source, size_t include_depth) { - auto* sub_includer = static_cast(user_data); - return sub_includer->GetInclude( - requested_source, static_cast(type), - requesting_source, include_depth); - }, - [](void* user_data, shaderc_include_result* include_result) { - auto* sub_includer = static_cast(user_data); - return sub_includer->ReleaseInclude(include_result); - }, - includer_.get()); + if (includer_) { + RegisterIncluderCallbacks(); + } else { + shaderc_compile_options_set_include_callbacks(options_, nullptr, nullptr, + nullptr); + } } // Forces the GLSL language version and profile to a given pair. The version @@ -378,9 +373,26 @@ class CompileOptions { } private: + void RegisterIncluderCallbacks() { + shaderc_compile_options_set_include_callbacks( + options_, + [](void* user_data, const char* requested_source, int type, + const char* requesting_source, size_t include_depth) { + auto* sub_includer = static_cast(user_data); + return sub_includer->GetInclude( + requested_source, static_cast(type), + requesting_source, include_depth); + }, + [](void* user_data, shaderc_include_result* include_result) { + auto* sub_includer = static_cast(user_data); + return sub_includer->ReleaseInclude(include_result); + }, + includer_.get()); + } + CompileOptions& operator=(const CompileOptions& other) = delete; shaderc_compile_options_t options_; - std::unique_ptr includer_; + std::shared_ptr includer_; friend class Compiler; }; diff --git a/libshaderc/src/shaderc_cpp_test.cc b/libshaderc/src/shaderc_cpp_test.cc index 401d5b984..f94e72076 100644 --- a/libshaderc/src/shaderc_cpp_test.cc +++ b/libshaderc/src/shaderc_cpp_test.cc @@ -811,6 +811,38 @@ class TestIncluder : public shaderc::CompileOptions::IncluderInterface { using IncluderTests = testing::TestWithParam; +class LifetimeTrackedIncluder + : public shaderc::CompileOptions::IncluderInterface { + public: + LifetimeTrackedIncluder(std::shared_ptr alive, const FakeFS& fake_fs) + : alive_(std::move(alive)), fake_fs_(fake_fs), responses_({}) { + *alive_ = true; + } + + ~LifetimeTrackedIncluder() override { *alive_ = false; } + + shaderc_include_result* GetInclude(const char* requested_source, + shaderc_include_type type, + const char* requesting_source, + size_t include_depth) override { + (void)type; + (void)requesting_source; + (void)include_depth; + responses_.emplace_back(shaderc_include_result{ + requested_source, strlen(requested_source), + fake_fs_.at(std::string(requested_source)).c_str(), + fake_fs_.at(std::string(requested_source)).size()}); + return &responses_.back(); + } + + void ReleaseInclude(shaderc_include_result*) override {} + + private: + std::shared_ptr alive_; + const FakeFS& fake_fs_; + std::vector responses_; +}; + // Parameterized tests for includer. TEST_P(IncluderTests, SetIncluder) { const IncluderTestCase& test_case = GetParam(); @@ -843,6 +875,58 @@ TEST_P(IncluderTests, SetIncluderClonedOptions) { HasSubstr(test_case.expected_substring())); } +TEST_F(CppInterface, CopiedOptionsKeepIncluderAliveAfterSourceDestroyed) { + const FakeFS fs = { + {"root", + "#version 150\n" + "void foo() {}\n" + "#include \"path/to/file_1\"\n"}, + {"path/to/file_1", "content of file_1\n"}, + }; + const std::string& shader = fs.at("root"); + auto alive = std::make_shared(false); + std::unique_ptr cloned_options; + + { + CompileOptions options; + options.SetIncluder(std::unique_ptr( + new LifetimeTrackedIncluder(alive, fs))); + cloned_options.reset(new CompileOptions(options)); + } + + ASSERT_TRUE(*alive); + const auto compilation_result = compiler_.PreprocessGlsl( + shader.c_str(), shaderc_glsl_vertex_shader, "shader", *cloned_options); + EXPECT_THAT(CompilerOutputAsString(compilation_result), + HasSubstr("content of file_1")); +} + +TEST_F(CppInterface, MovedOptionsKeepIncluderAliveAfterSourceDestroyed) { + const FakeFS fs = { + {"root", + "#version 150\n" + "void foo() {}\n" + "#include \"path/to/file_1\"\n"}, + {"path/to/file_1", "content of file_1\n"}, + }; + const std::string& shader = fs.at("root"); + auto alive = std::make_shared(false); + std::unique_ptr moved_options; + + { + CompileOptions options; + options.SetIncluder(std::unique_ptr( + new LifetimeTrackedIncluder(alive, fs))); + moved_options.reset(new CompileOptions(std::move(options))); + } + + ASSERT_TRUE(*alive); + const auto compilation_result = compiler_.PreprocessGlsl( + shader.c_str(), shaderc_glsl_vertex_shader, "shader", *moved_options); + EXPECT_THAT(CompilerOutputAsString(compilation_result), + HasSubstr("content of file_1")); +} + INSTANTIATE_TEST_SUITE_P(CppInterface, IncluderTests, testing::ValuesIn(std::vector{ IncluderTestCase(