From a6239ead17efb9e07236c834a025e082e89f4de2 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Sun, 21 Apr 2024 17:28:02 -0700 Subject: [PATCH] fix: Error retrieval safeguards for recycled objects (#4239) The recent switch from boost thread_specific_ptr left a bug: if an ImageInput (say) is deleted without the caller clearing its error state, and then later another ImageInput is allocated and happens to get the same address as the first one, it can "inherit" the unretrieved error state left behind by the other. Fix by using a unique ID (initialized by an incrementing atomic counter) to index the object in the per-thread error store. I feel like we still don't have a foolproof replacement for the really nice lifetime management that thread_specific_ptr was doing for us. One could argue that unretrieved errors in an object are a "bug" in the user's program, but unretrieved bugs currently just accumulate in the map, even after the ImageInput is destroyed, until the thread terminates. --------- Signed-off-by: Larry Gritz --- src/libOpenImageIO/imageinput.cpp | 19 +++++++++++++------ src/libOpenImageIO/imageoutput.cpp | 16 +++++++++++----- src/libtexture/imagecache.cpp | 20 +++++++++----------- src/libtexture/texture_pvt.h | 7 ++++--- src/libtexture/texturesys.cpp | 15 +++++++++------ 5 files changed, 46 insertions(+), 31 deletions(-) diff --git a/src/libOpenImageIO/imageinput.cpp b/src/libOpenImageIO/imageinput.cpp index 26da944799..ee8a0dc1e0 100644 --- a/src/libOpenImageIO/imageinput.cpp +++ b/src/libOpenImageIO/imageinput.cpp @@ -27,13 +27,20 @@ using namespace pvt; // store an error message per thread, for a specific ImageInput -static thread_local tsl::robin_map - input_error_messages; +static thread_local tsl::robin_map input_error_messages; +static std::atomic_int64_t input_next_id(0); + class ImageInput::Impl { public: + Impl() + : m_id(++input_next_id) + { + } + // So we can lock this ImageInput for the thread-safe methods. std::recursive_mutex m_mutex; + uint64_t m_id; int m_threads = 0; // The IOProxy object we will use for all I/O operations. @@ -86,7 +93,7 @@ ImageInput::~ImageInput() // Erase any leftover errors from this thread // TODO: can we clear other threads' errors? // TODO: potentially unsafe due to the static destruction order fiasco - // input_error_messages.erase(this); + // input_error_messages.erase(m_impl->m_id); } @@ -1096,7 +1103,7 @@ ImageInput::append_error(string_view message) const { if (message.size() && message.back() == '\n') message.remove_suffix(1); - std::string& err_str = input_error_messages[this]; + std::string& err_str = input_error_messages[m_impl->m_id]; OIIO_DASSERT( err_str.size() < 1024 * 1024 * 16 && "Accumulated error messages > 16MB. Try checking return codes!"); @@ -1112,7 +1119,7 @@ ImageInput::append_error(string_view message) const bool ImageInput::has_error() const { - auto iter = input_error_messages.find(this); + auto iter = input_error_messages.find(m_impl->m_id); if (iter == input_error_messages.end()) return false; return iter.value().size() > 0; @@ -1124,7 +1131,7 @@ std::string ImageInput::geterror(bool clear) const { std::string e; - auto iter = input_error_messages.find(this); + auto iter = input_error_messages.find(m_impl->m_id); if (iter != input_error_messages.end()) { e = iter.value(); if (clear) diff --git a/src/libOpenImageIO/imageoutput.cpp b/src/libOpenImageIO/imageoutput.cpp index 6cd3f7ce16..75bfa3f6b3 100644 --- a/src/libOpenImageIO/imageoutput.cpp +++ b/src/libOpenImageIO/imageoutput.cpp @@ -30,16 +30,22 @@ using namespace pvt; // store an error message per thread, for a specific ImageInput -static thread_local tsl::robin_map - output_error_messages; +static thread_local tsl::robin_map output_error_messages; +static std::atomic_int64_t output_next_id(0); class ImageOutput::Impl { public: + Impl() + : m_id(++output_next_id) + { + } + // Unneeded? // // So we can lock this ImageOutput for the thread-safe methods. // std::recursive_mutex m_mutex; + uint64_t m_id; int m_threads = 0; // The IOProxy object we will use for all I/O operations. @@ -269,7 +275,7 @@ ImageOutput::append_error(string_view message) const { if (message.size() && message.back() == '\n') message.remove_suffix(1); - std::string& err_str = output_error_messages[this]; + std::string& err_str = output_error_messages[m_impl->m_id]; OIIO_DASSERT( err_str.size() < 1024 * 1024 * 16 && "Accumulated error messages > 16MB. Try checking return codes!"); @@ -698,7 +704,7 @@ ImageOutput::copy_tile_to_image_buffer(int x, int y, int z, TypeDesc format, bool ImageOutput::has_error() const { - auto iter = output_error_messages.find(this); + auto iter = output_error_messages.find(m_impl->m_id); if (iter == output_error_messages.end()) return false; return iter.value().size() > 0; @@ -710,7 +716,7 @@ std::string ImageOutput::geterror(bool clear) const { std::string e; - auto iter = output_error_messages.find(this); + auto iter = output_error_messages.find(m_impl->m_id); if (iter != output_error_messages.end()) { e = iter.value(); if (clear) diff --git a/src/libtexture/imagecache.cpp b/src/libtexture/imagecache.cpp index 9f5426bdd5..6e66a50c01 100644 --- a/src/libtexture/imagecache.cpp +++ b/src/libtexture/imagecache.cpp @@ -49,6 +49,7 @@ spin_mutex ImageCacheImpl::m_perthread_info_mutex; namespace { // anonymous +static thread_local tsl::robin_map imcache_error_messages; static std::shared_ptr shared_image_cache; static spin_mutex shared_image_cache_mutex; @@ -65,11 +66,8 @@ static ustring s_averagecolor("averagecolor"), s_averagealpha("averagealpha"); static ustring s_constantcolor("constantcolor"); static ustring s_constantalpha("constantalpha"); -static thread_local tsl::robin_map - imcache_error_messages; - // constantly increasing, so we can avoid issues if an ImageCache pointer is re-used after being freed -static std::atomic_int64_t imagecache_id_atomic = 0; +static std::atomic_int64_t imagecache_next_id = 0; static thread_local tsl::robin_map imagecache_per_thread_infos; @@ -1657,7 +1655,7 @@ ImageCacheTile::data(int x, int y, int z, int c) const ImageCacheImpl::ImageCacheImpl() { - imagecache_id = imagecache_id_atomic.fetch_add(1); + imagecache_id = imagecache_next_id.fetch_add(1); init(); } @@ -1725,7 +1723,7 @@ ImageCacheImpl::~ImageCacheImpl() // Erase any leftover errors from this thread // TODO: can we clear other threads' errors? // TODO: potentially unsafe due to the static destruction order fiasco - // imcache_error_messages.erase(this); + // imcache_error_messages.erase(imagecache_id); } @@ -2735,7 +2733,7 @@ ImageCacheImpl::get_image_info(ImageCacheFile* file, // for the file to be nonexistent or broken! if ((*(int*)data = (file && !file->broken())) == 0) { // eat any error generated by find_file - imcache_error_messages.erase(this); + imcache_error_messages.erase(imagecache_id); (void)OIIO::geterror(true); // Suppress global errors } return true; @@ -2763,7 +2761,7 @@ ImageCacheImpl::get_image_info(ImageCacheFile* file, if (file->broken()) { if (file->errors_should_issue()) { // eat any earlier messages - imcache_error_messages.erase(this); + imcache_error_messages.erase(imagecache_id); error("Invalid image file \"{}\": {}", file->filename(), file->broken_error_message()); } @@ -3878,7 +3876,7 @@ ImageCacheImpl::purge_perthread_microcaches() bool ImageCacheImpl::has_error() const { - auto iter = imcache_error_messages.find(this); + auto iter = imcache_error_messages.find(imagecache_id); if (iter == imcache_error_messages.end()) return false; return iter.value().size() > 0; @@ -3890,7 +3888,7 @@ std::string ImageCacheImpl::geterror(bool clear) const { std::string e; - auto iter = imcache_error_messages.find(this); + auto iter = imcache_error_messages.find(imagecache_id); if (iter != imcache_error_messages.end()) { e = iter.value(); if (clear) @@ -3906,7 +3904,7 @@ ImageCacheImpl::append_error(string_view message) const { if (message.size() && message.back() == '\n') message.remove_suffix(1); - std::string& err_str = imcache_error_messages[this]; + std::string& err_str = imcache_error_messages[imagecache_id]; OIIO_DASSERT( err_str.size() < 1024 * 1024 * 16 && "Accumulated error messages > 16MB. Try checking return codes!"); diff --git a/src/libtexture/texture_pvt.h b/src/libtexture/texture_pvt.h index 777489cfb4..dc352a6aae 100644 --- a/src/libtexture/texture_pvt.h +++ b/src/libtexture/texture_pvt.h @@ -551,9 +551,10 @@ class TextureSystemImpl final : public TextureSystem { float dsdy, float dtdy, float sblur, float tblur); ImageCacheImpl* m_imagecache = nullptr; - bool m_imagecache_owner = false; ///< True if we own the ImageCache - Imath::M44f m_Mw2c; ///< world-to-"common" matrix - Imath::M44f m_Mc2w; ///< common-to-world matrix + uint64_t m_id; // A unique ID for this TextureSystem + Imath::M44f m_Mw2c; ///< world-to-"common" matrix + Imath::M44f m_Mc2w; ///< common-to-world matrix + bool m_imagecache_owner = false; ///< True if we own the ImageCache bool m_gray_to_rgb; ///< automatically copy gray to rgb channels? bool m_flip_t; ///< Flip direction of t coord? int m_max_tile_channels; ///< narrow tile ID channel range when diff --git a/src/libtexture/texturesys.cpp b/src/libtexture/texturesys.cpp index 4dd530a3ef..578e3947e7 100644 --- a/src/libtexture/texturesys.cpp +++ b/src/libtexture/texturesys.cpp @@ -53,6 +53,9 @@ static spin_mutex shared_texturesys_mutex; static bool do_unit_test_texture = false; static float unit_test_texture_blur = 0.0f; +static thread_local tsl::robin_map txsys_error_messages; +static std::atomic_int64_t txsys_next_id(0); + static vfloat4 u8scale(1.0f / 255.0f); static vfloat4 u16scale(1.0f / 65535.0f); @@ -323,6 +326,7 @@ texture_type_name(TexFormat f) TextureSystemImpl::TextureSystemImpl(ImageCache* imagecache) + : m_id(++txsys_next_id) { m_imagecache = (ImageCacheImpl*)imagecache; init(); @@ -358,7 +362,7 @@ TextureSystemImpl::~TextureSystemImpl() // Erase any leftover errors from this thread // TODO: can we clear other threads' errors? // TODO: potentially unsafe due to the static destruction order fiasco - // txsys_error_messages.erase(this); + // txsys_error_messages.erase(m_id); } @@ -900,13 +904,12 @@ TextureSystemImpl::get_texels(TextureHandle* texture_handle_, return ok; } -static thread_local tsl::robin_map - txsys_error_messages; + bool TextureSystemImpl::has_error() const { - auto iter = txsys_error_messages.find(this); + auto iter = txsys_error_messages.find(m_id); if (iter == txsys_error_messages.end()) return false; return iter.value().size() > 0; @@ -918,7 +921,7 @@ std::string TextureSystemImpl::geterror(bool clear) const { std::string e; - auto iter = txsys_error_messages.find(this); + auto iter = txsys_error_messages.find(m_id); if (iter != txsys_error_messages.end()) { e = iter.value(); if (clear) @@ -934,7 +937,7 @@ TextureSystemImpl::append_error(string_view message) const { if (message.size() && message.back() == '\n') message.remove_suffix(1); - std::string& err_str = txsys_error_messages[this]; + std::string& err_str = txsys_error_messages[m_id]; OIIO_DASSERT( err_str.size() < 1024 * 1024 * 16 && "Accumulated error messages > 16MB. Try checking return codes!");