From 05f24516dd737fb0698fd4eca0996570113a413b Mon Sep 17 00:00:00 2001 From: Sungun Park Date: Mon, 15 Dec 2025 13:04:36 -0800 Subject: [PATCH] Fix Build.builder.feature() for Java bindings (#9517) The "const char*" used for the string literal parameter of the `feature` method becomes invalid once it passes outside the Java binding scope, leading to invalid data access. This fix ensures the string is safely stored. Add support for heterogeneous lookup in associative containers that use CString keys. Add conversion operators for std::string_view and raw string literals to CString. --- filament/src/details/Engine.cpp | 7 +- filament/src/details/Material.cpp | 3 +- libs/gltfio/src/ArchiveCache.h | 2 +- libs/uberz/include/uberz/WritableArchive.h | 2 +- libs/utils/include/utils/CString.h | 112 ++++++++++++++---- libs/utils/test/test_CString.cpp | 128 ++++++++++++++++++++- 6 files changed, 223 insertions(+), 31 deletions(-) diff --git a/filament/src/details/Engine.cpp b/filament/src/details/Engine.cpp index 2ea13ed5de..5e27345e37 100644 --- a/filament/src/details/Engine.cpp +++ b/filament/src/details/Engine.cpp @@ -55,6 +55,7 @@ #include #include +#include #include #include #include @@ -132,7 +133,7 @@ struct Engine::BuilderDetails { FeatureLevel mFeatureLevel = FeatureLevel::FEATURE_LEVEL_1; void* mSharedContext = nullptr; bool mPaused = false; - std::unordered_map mFeatureFlags; + std::unordered_map mFeatureFlags; static Config validateConfig(Config config) noexcept; }; @@ -283,7 +284,7 @@ FEngine::FEngine(Builder const& builder) : // update all the features flags specified in the builder for (auto const& feature : builder->mFeatureFlags) { - auto* const p = getFeatureFlagPtr(feature.first, true); + auto* const p = getFeatureFlagPtr(feature.first.c_str_safe(), true); if (p) { *p = feature.second; } @@ -1589,7 +1590,7 @@ Engine::Builder& Engine::Builder::paused(bool const paused) noexcept { } Engine::Builder& Engine::Builder::feature(char const* name, bool const value) noexcept { - mImpl->mFeatureFlags[name] = value; + mImpl->mFeatureFlags.emplace(name, value); return *this; } diff --git a/filament/src/details/Material.cpp b/filament/src/details/Material.cpp index a58f320509..da15020990 100644 --- a/filament/src/details/Material.cpp +++ b/filament/src/details/Material.cpp @@ -93,8 +93,7 @@ struct Material::BuilderDetails { UboBatchingMode mUboBatchingMode = UboBatchingMode::DEFAULT; std::unordered_map< CString, - std::variant, - CString::Hasher> mConstantSpecializations; + std::variant> mConstantSpecializations; }; FMaterial::DefaultMaterialBuilder::DefaultMaterialBuilder() { diff --git a/libs/gltfio/src/ArchiveCache.h b/libs/gltfio/src/ArchiveCache.h index fdc9fd153b..bbd157f1f3 100644 --- a/libs/gltfio/src/ArchiveCache.h +++ b/libs/gltfio/src/ArchiveCache.h @@ -58,7 +58,7 @@ namespace filament::gltfio { struct ArchiveRequirements { Shading shadingModel; BlendingMode blendingMode; - tsl::robin_map features; + tsl::robin_map features; }; } // namespace filament::uberz diff --git a/libs/uberz/include/uberz/WritableArchive.h b/libs/uberz/include/uberz/WritableArchive.h index e511d95f8d..b383b70c19 100644 --- a/libs/uberz/include/uberz/WritableArchive.h +++ b/libs/uberz/include/uberz/WritableArchive.h @@ -56,7 +56,7 @@ private: utils::FixedCapacityVector package; Shading shadingModel; BlendingMode blendingMode; - tsl::robin_map flags; + tsl::robin_map flags; }; utils::FixedCapacityVector mMaterials; diff --git a/libs/utils/include/utils/CString.h b/libs/utils/include/utils/CString.h index f591384c75..935845f2fa 100644 --- a/libs/utils/include/utils/CString.h +++ b/libs/utils/include/utils/CString.h @@ -37,19 +37,6 @@ namespace io { class ostream; } -//! \privatesection -struct hashCStrings { - typedef const char* argument_type; - typedef size_t result_type; - result_type operator()(argument_type cstr) const noexcept { - size_t hash = 5381; - while (int const c = static_cast(*cstr++)) { - hash = (hash * 33u) ^ size_t(c); - } - return hash; - } -}; - template using StringLiteral = const char[N]; @@ -86,6 +73,11 @@ public: // inside the string. explicit CString(size_t length); + // Conversion from std::string_view + explicit CString(const std::string_view& str) + : CString(str.data(), str.size()) { + } + // Allocates memory and copies traditional C string content. Unlike the above constructor, this // does not allow embedded nulls. This is explicit because this operation is costly. // This is a template to ensure it's not preferred over the string literal constructor below. @@ -338,13 +330,10 @@ public: return ptr; } - struct Hasher : private hashCStrings { - typedef CString argument_type; - typedef size_t result_type; - result_type operator()(const argument_type& s) const noexcept { - return hashCStrings::operator()(s.c_str()); - } - }; + // conversion to std::string_view + operator std::string_view() const noexcept { + return std::string_view{data(), size()}; + } private: static void do_tracking(bool ctor); @@ -370,10 +359,12 @@ private: Data* mData; // Data is stored at mData[-1] }; + int compare(const std::string_view& rhs) const noexcept { + return std::string_view{data(), size()}.compare(rhs); + } + int compare(const CString& rhs) const noexcept { - auto const l = std::string_view{data(), size()}; - auto const r = std::string_view{rhs.data(), rhs.size()}; - return l.compare(r); + return compare(std::string_view{rhs.data(), rhs.size()}); } friend bool operator==(CString const& lhs, CString const& rhs) noexcept { @@ -394,6 +385,43 @@ private: friend bool operator<=(CString const& lhs, CString const& rhs) noexcept { return !(lhs > rhs); } + + friend bool operator==(CString const& lhs, std::string_view const& rhs) noexcept { + return lhs.compare(rhs) == 0; + } + friend bool operator==(std::string_view const& lhs, CString const& rhs) noexcept { + return lhs.compare(rhs) == 0; + } + friend bool operator!=(CString const& lhs, std::string_view const& rhs) noexcept { + return !(lhs == rhs); + } + friend bool operator!=(std::string_view const& lhs, CString const& rhs) noexcept { + return !(lhs == rhs); + } + friend bool operator<(CString const& lhs, std::string_view const& rhs) noexcept { + return lhs.compare(rhs) < 0; + } + friend bool operator<(std::string_view const& lhs, CString const& rhs) noexcept { + return lhs.compare(rhs) < 0; + } + friend bool operator>(CString const& lhs, std::string_view const& rhs) noexcept { + return lhs.compare(rhs) > 0; + } + friend bool operator>(std::string_view const& lhs, CString const& rhs) noexcept { + return lhs.compare(rhs) > 0; + } + friend bool operator>=(CString const& lhs, std::string_view const& rhs) noexcept { + return !(lhs < rhs); + } + friend bool operator>=(std::string_view const& lhs, CString const& rhs) noexcept { + return !(lhs < rhs); + } + friend bool operator<=(CString const& lhs, std::string_view const& rhs) noexcept { + return !(lhs > rhs); + } + friend bool operator<=(std::string_view const& lhs, CString const& rhs) noexcept { + return !(lhs > rhs); + } }; // operator+ @@ -504,4 +532,42 @@ private: } // namespace utils +// heterogeneous lookup support for associative containers +namespace std { + template <> + struct hash { + using is_transparent = void; // Enable heterogeneous lookup + + size_t operator()(const utils::CString& k) const noexcept { + return compute_hash(std::string_view(k)); + } + + template && + !std::is_same_v, utils::CString>>> + size_t operator()(const T& k) const noexcept { + return compute_hash(std::string_view(k)); + } + + private: + size_t compute_hash(std::string_view k) const noexcept { + size_t hash = 5381; + for (char const c : k) { + hash = (hash * 33u) ^ size_t(c); + } + return hash; + } + }; + + template<> + struct equal_to { + using is_transparent = void; // Enable heterogeneous lookup + + template + bool operator()(const T& lhs, const U& rhs) const { + return lhs == rhs; + } + }; +} + #endif // TNT_UTILS_CSTRING_H diff --git a/libs/utils/test/test_CString.cpp b/libs/utils/test/test_CString.cpp index ab5d12c41a..a9d66530d1 100644 --- a/libs/utils/test/test_CString.cpp +++ b/libs/utils/test/test_CString.cpp @@ -22,6 +22,7 @@ #include #include #include +#include using namespace utils; @@ -242,7 +243,7 @@ TEST(CString, Concatenation) { } } -TEST(CString, Comparison) { +TEST(CString, ComparisonCString) { CString s1("abc"); CString s2("abc"); CString s3("def"); @@ -272,6 +273,106 @@ TEST(CString, Comparison) { EXPECT_TRUE(s1 > s4); } +TEST(CString, ComparisonStringView) { + const CString CS1("abc"); + const CString CS2("abc"); + const CString CS3("def"); + const CString CS4("ab"); + const std::string_view sv1("abc"); + const std::string_view sv2("abc"); + const std::string_view sv3("def"); + const std::string_view sv4("ab"); + + EXPECT_TRUE(CS1 == sv2); + EXPECT_TRUE(sv2 == CS1); + EXPECT_FALSE(CS1 == sv3); + EXPECT_FALSE(sv3 == CS1); + + EXPECT_TRUE(CS1 != sv3); + EXPECT_TRUE(sv3 != CS1); + EXPECT_FALSE(CS1 != sv2); + EXPECT_FALSE(sv2 != CS1); + + EXPECT_TRUE(CS1 < sv3); + EXPECT_TRUE(sv1 < CS3); + EXPECT_FALSE(CS3 < sv1); + EXPECT_FALSE(sv3 < CS1); + + EXPECT_TRUE(CS3 > sv1); + EXPECT_TRUE(sv3 > CS1); + EXPECT_FALSE(CS1 > sv3); + EXPECT_FALSE(sv1 > CS3); + + EXPECT_TRUE(CS1 <= sv2); + EXPECT_TRUE(sv1 <= CS2); + EXPECT_TRUE(CS1 <= sv3); + EXPECT_TRUE(sv1 <= CS3); + EXPECT_FALSE(CS3 <= sv1); + EXPECT_FALSE(sv3 <= CS1); + + EXPECT_TRUE(CS2 >= sv1); + EXPECT_TRUE(sv2 >= CS1); + EXPECT_TRUE(CS3 >= sv1); + EXPECT_TRUE(sv3 >= CS1); + EXPECT_FALSE(CS1 >= sv3); + EXPECT_FALSE(sv1 >= CS3); + + EXPECT_TRUE(CS4 < sv1); + EXPECT_TRUE(sv4 < CS1); + EXPECT_TRUE(CS1 > sv4); + EXPECT_TRUE(sv1 > CS4); +} + +TEST(CString, ComparisonStringLiteral) { + const CString CS1("abc"); + const CString CS2("abc"); + const CString CS3("def"); + const CString CS4("ab"); + const char* sv1("abc"); + const char* sv2("abc"); + const char* sv3("def"); + const char* sv4("ab"); + + EXPECT_TRUE(CS1 == sv2); + EXPECT_TRUE(sv2 == CS1); + EXPECT_FALSE(CS1 == sv3); + EXPECT_FALSE(sv3 == CS1); + + EXPECT_TRUE(CS1 != sv3); + EXPECT_TRUE(sv3 != CS1); + EXPECT_FALSE(CS1 != sv2); + EXPECT_FALSE(sv2 != CS1); + + EXPECT_TRUE(CS1 < sv3); + EXPECT_TRUE(sv1 < CS3); + EXPECT_FALSE(CS3 < sv1); + EXPECT_FALSE(sv3 < CS1); + + EXPECT_TRUE(CS3 > sv1); + EXPECT_TRUE(sv3 > CS1); + EXPECT_FALSE(CS1 > sv3); + EXPECT_FALSE(sv1 > CS3); + + EXPECT_TRUE(CS1 <= sv2); + EXPECT_TRUE(sv1 <= CS2); + EXPECT_TRUE(CS1 <= sv3); + EXPECT_TRUE(sv1 <= CS3); + EXPECT_FALSE(CS3 <= sv1); + EXPECT_FALSE(sv3 <= CS1); + + EXPECT_TRUE(CS2 >= sv1); + EXPECT_TRUE(sv2 >= CS1); + EXPECT_TRUE(CS3 >= sv1); + EXPECT_TRUE(sv3 >= CS1); + EXPECT_FALSE(CS1 >= sv3); + EXPECT_FALSE(sv1 >= CS3); + + EXPECT_TRUE(CS4 < sv1); + EXPECT_TRUE(sv4 < CS1); + EXPECT_TRUE(CS1 > sv4); + EXPECT_TRUE(sv1 > CS4); +} + TEST(CString, ElementAccess) { CString str("01234"); const CString cstr("const"); @@ -595,6 +696,31 @@ TEST(CString, ToString) { EXPECT_STREQ("-3.140000", to_string(-3.14f).c_str()); } +TEST(CString, HeterogeneousLookupForContainer) { + std::unordered_set us; + + const char* world("world"); + const std::string_view filament("filament"); + const CString unittest("unittest"); + + EXPECT_TRUE(us.insert("hello").second); + EXPECT_TRUE(us.insert(CString(world)).second); // or emplace(world) + EXPECT_TRUE(us.insert(CString(filament)).second); // or emplace(filament) + EXPECT_TRUE(us.insert(unittest).second); + + auto it = us.find("hello"); + EXPECT_NE(it, us.end()); + it = us.find(world); + EXPECT_NE(it, us.end()); + it = us.find(filament); + EXPECT_NE(it, us.end()); + it = us.find(unittest); + EXPECT_NE(it, us.end()); + + it = us.find("invalid key"); + EXPECT_EQ(it, us.end()); +} + TEST(FixedSizeString, EmptyString) { { FixedSizeString<32> str;