fix: Error retrieval safeguards for recycled objects (#4239)

lgritz · web-flow · commit a6239ead17ef · 2024-04-21T17:28:02.000-07:00
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 &lt;lg@larrygritz.com&gt;
diff --git 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<const ImageInput*, std::string>
-    input_error_messages;
+static thread_local tsl::robin_map<uint64_t, std::string> 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
@@ -30,16 +30,22 @@ using namespace pvt;
 
 
 // store an error message per thread, for a specific ImageInput
-static thread_local tsl::robin_map<const ImageOutput*, std::string>
-    output_error_messages;
+static thread_local tsl::robin_map<uint64_t, std::string> 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
@@ -49,6 +49,7 @@ spin_mutex ImageCacheImpl::m_perthread_info_mutex;
 
 namespace {  // anonymous
 
+static thread_local tsl::robin_map<uint64_t, std::string> imcache_error_messages;
 static std::shared_ptr<ImageCacheImpl> 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<const ImageCacheImpl*, std::string>
-    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<uint64_t, ImageCachePerThreadInfo*>
     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
@@ -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
@@ -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<int64_t, std::string> 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<const TextureSystemImpl*, std::string>
-    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!");
