Skip to content

fix: Error retrieval safeguards for recycled objects #4239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lgritz merged 3 commits into from
Apr 22, 2024
Merged

fix: Error retrieval safeguards for recycled objects #4239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
76cb77d
fix: Error retrieval safeguards for recycled objects
lgritz Apr 21, 2024
6a443e0
Revision: unique counter in objects with per-thread error state
lgritz Apr 21, 2024
c30f34f
Fix mixup between imagecache_id and the next atomic id counter
lgritz Apr 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions src/libOpenImageIO/imageinput.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}


Expand Down Expand Up @@ -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!");
Expand All @@ -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;
Expand All @@ -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)
Expand Down
16 changes: 11 additions & 5 deletions src/libOpenImageIO/imageoutput.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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!");
Expand Down Expand Up @@ -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;
Expand All @@ -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)
Expand Down
20 changes: 9 additions & 11 deletions src/libtexture/imagecache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;

Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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);
}


Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}
Expand Down Expand Up @@ -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;
Expand All @@ -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)
Expand All @@ -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!");
Expand Down
7 changes: 4 additions & 3 deletions src/libtexture/texture_pvt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 9 additions & 6 deletions src/libtexture/texturesys.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -323,6 +326,7 @@ texture_type_name(TexFormat f)


TextureSystemImpl::TextureSystemImpl(ImageCache* imagecache)
: m_id(++txsys_next_id)
{
m_imagecache = (ImageCacheImpl*)imagecache;
init();
Expand Down Expand Up @@ -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);
}


Expand Down Expand Up @@ -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;
Expand All @@ -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)
Expand All @@ -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!");
Expand Down