-
Notifications
You must be signed in to change notification settings - Fork 655
Description
Description:
I encountered a situation where applications using OpenImageIO are terminated abruptly. This happens in the ImageBuf destructor when it attempts to print uncaught errors: imagebuf.cpp#L574-L582
Root Cause:
- Throwing in Destructor:
OIIO::printrelies onfmt. The bundled or externalfmtimplementation (specificallyfwrite_fullyinformat-inl.h) throws asystem_errorifstd::fwritewrites fewer bytes than expected.// include/OpenImageIO/detail/fmt/format-inl.h inline void fwrite_fully(const void* ptr, size_t count, FILE* stream) { size_t written = std::fwrite(ptr, 1, count, stream); if (written < count) FMT_THROW(system_error(errno, FMT_STRING("cannot write to file"))); }
- Terminate: Since
ImageBufImpl::~ImageBufImplis a destructor, if this exception is thrown (especially during stack unwinding from another exception),std::terminateis called, killing the entire application.// include/OpenImageIO/detail/fmt/format.h #ifndef FMT_THROW # if FMT_EXCEPTIONS # if FMT_MSC_VERSION || defined(__NVCC__) FMT_BEGIN_NAMESPACE namespace detail { template <typename Exception> inline void do_throw(const Exception& x) { // Silence unreachable code warnings in MSVC and NVCC because these // are nearly impossible to fix in a generic code. volatile bool b = true; if (b) throw x; } } // namespace detail FMT_END_NAMESPACE # define FMT_THROW(x) detail::do_throw(x) # else # define FMT_THROW(x) throw x # endif # else # define FMT_THROW(x) \ ::fmt::detail::assert_fail(__FILE__, __LINE__, (x).what()) # endif #endif
Issues for Discussion:
-
Error Handling Mix-up: I think the intention of the code in the destructor is to help devs debug and correct logic errors. It's reasonable to alert if there are unhandled errors. However, a failure in this diagnostic printing mechanism shouldn't escalate to an application crash.
-
FMT_THROW Configuration: There seems to be a mix of identifying errors via asserts and exceptions (see the code block I attached above). In addition, the usage of
FMT_THROWimplies exceptions are expected, butOIIOcode calling it fromnoexceptcontexts (like destructors) is dangerous. -
Reliability of
std::fwritecheck: Using the return value ofstd::fwriteto strictly enforce "all bytes written or die" could be too aggressive, especially for logging/diagnostic output.std::fwritemay write fewer items than requested if the underlying stream buffer is full (e.g., specific pipe buffer limits when redirecting stdout/stderr) or if the consumer of the pipe is slow or has disconnected.- A partial write is not necessarily a fatal error that warrants terminating the process, especially when simply trying to log a warning during destruction.