From b006e5b52b9c7c212369a7b5c0065a4451114e97 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Mon, 26 Aug 2024 18:33:48 -0600 Subject: [PATCH] fix: Don't let fmtlib exceptions crash the app When fmt arguments don't match the format string, fmt will throw an exception, or terminate if we disable exceptions (which we do). For gcc11+, we tried intercepting these, but let's do it for all platforms, and let's not terminate ourselves, but insted print and log the error. But, oof, to do this properly, I needed to move some error recording functionality from libOpenImageIO to libOpenImageIO_Util and make it owned more properly by strutil.cpp, so that this all works even when only using the util library. The logic isn't changing, it's just moving over to the other library. This all helps to address 4388 Unfortunately, the exception thrown by fmt doesn't tell us the bad format string itself. That would have really allowed to probably zero right in on it. But at least we know it's occurring, and one could put a breakpoint on pvt::log_fmt_error to catch it in the act and see where it's being called, revealing the bad line. Signed-off-by: Larry Gritz --- src/include/OpenImageIO/detail/fmt.h | 19 ++++--- src/include/OpenImageIO/strutil.h | 4 ++ src/include/imageio_pvt.h | 1 + src/libOpenImageIO/imageio.cpp | 53 ++----------------- src/libutil/strutil.cpp | 77 ++++++++++++++++++++++++++++ src/oiiotool/oiiotool.cpp | 6 +++ testsuite/oiiotool/ref/out.txt | 6 +++ testsuite/oiiotool/run.py | 3 ++ 8 files changed, 112 insertions(+), 57 deletions(-) diff --git a/src/include/OpenImageIO/detail/fmt.h b/src/include/OpenImageIO/detail/fmt.h index 74e6fae78c..6978311933 100644 --- a/src/include/OpenImageIO/detail/fmt.h +++ b/src/include/OpenImageIO/detail/fmt.h @@ -19,13 +19,18 @@ # define FMT_EXCEPTIONS 0 #endif -// Redefining FMT_THROW to something benign seems to avoid some UB or possibly -// gcc 11+ compiler bug triggered by the definition of FMT_THROW in fmt 10.1+ -// when FMT_EXCEPTIONS=0, which results in mangling SIMD math. This nugget -// below works around the problems for hard to understand reasons. -#if !defined(FMT_THROW) && !FMT_EXCEPTIONS && OIIO_GNUC_VERSION >= 110000 -# define FMT_THROW(x) \ - OIIO_ASSERT_MSG(0, "fmt exception: %s", (x).what()), std::terminate() +OIIO_NAMESPACE_BEGIN +namespace pvt { +OIIO_UTIL_API void +log_fmt_error(const char* message); +}; +OIIO_NAMESPACE_END + +// Redefining FMT_THROW to print and log the error. This should only occur if +// we've made a mistake and mismatched a format string and its arguments. +// Hopefully this will help us track it down. +#if !defined(FMT_THROW) && !FMT_EXCEPTIONS +# define FMT_THROW(x) OIIO::pvt::log_fmt_error((x).what()) #endif // Use the grisu fast floating point formatting for old fmt versions diff --git a/src/include/OpenImageIO/strutil.h b/src/include/OpenImageIO/strutil.h index e0c5ec302f..cb92333dcf 100644 --- a/src/include/OpenImageIO/strutil.h +++ b/src/include/OpenImageIO/strutil.h @@ -281,7 +281,11 @@ using sync::print; namespace pvt { +// Internal use only OIIO_UTIL_API void debug(string_view str); +OIIO_UTIL_API void append_error(string_view str); +OIIO_UTIL_API bool has_error(); +OIIO_UTIL_API std::string geterror(bool clear); } /// `debug(format, ...)` prints debugging message when attribute "debug" is diff --git a/src/include/imageio_pvt.h b/src/include/imageio_pvt.h index 08fe2bb52f..4b854bcded 100644 --- a/src/include/imageio_pvt.h +++ b/src/include/imageio_pvt.h @@ -39,6 +39,7 @@ extern std::string output_format_list; extern std::string extension_list; extern std::string library_list; extern OIIO_UTIL_API int oiio_print_debug; +extern OIIO_UTIL_API int oiio_print_uncaught_errors; extern int oiio_log_times; extern int openexr_core; extern int limit_channels; diff --git a/src/libOpenImageIO/imageio.cpp b/src/libOpenImageIO/imageio.cpp index e7712b19cc..ba859c607c 100644 --- a/src/libOpenImageIO/imageio.cpp +++ b/src/libOpenImageIO/imageio.cpp @@ -62,7 +62,6 @@ std::string output_format_list; // comma-separated list of writable formats std::string extension_list; // list of all extensions for all formats std::string library_list; // list of all libraries for all formats int oiio_log_times = Strutil::stoi(Sysutil::getenv("OPENIMAGEIO_LOG_TIMES")); -int oiio_print_uncaught_errors(1); std::vector oiio_missingcolor; } // namespace pvt @@ -285,51 +284,10 @@ openimageio_version() -// ErrorHolder houses a string, with the addition that when it is destroyed, -// it will disgorge any un-retrieved error messages, in an effort to help -// beginning users diagnose their problems if they have forgotten to call -// geterror(). -struct ErrorHolder { - std::string error_msg; - - ~ErrorHolder() - { - if (!error_msg.empty() && pvt::oiio_print_uncaught_errors) { - OIIO::print( - "OpenImageIO exited with a pending error message that was never\n" - "retrieved via OIIO::geterror(). This was the error message:\n{}\n", - error_msg); - } - } -}; - - - -// To avoid thread oddities, we have the storage area buffering error -// messages for append_error()/geterror() be thread-specific. -static thread_local ErrorHolder error_msg_holder; - - void pvt::append_error(string_view message) { - // Remove a single trailing newline - if (message.size() && message.back() == '\n') - message.remove_suffix(1); - std::string& error_msg(error_msg_holder.error_msg); - OIIO_ASSERT( - error_msg.size() < 1024 * 1024 * 16 - && "Accumulated error messages > 16MB. Try checking return codes!"); - // If we are appending to existing error messages, separate them with - // a single newline. - if (error_msg.size() && error_msg.back() != '\n') - error_msg += '\n'; - error_msg += std::string(message); - - // Remove a single trailing newline - if (message.size() && message.back() == '\n') - message.remove_suffix(1); - error_msg = std::string(message); + Strutil::pvt::append_error(message); } @@ -337,8 +295,7 @@ pvt::append_error(string_view message) bool has_error() { - std::string& error_msg(error_msg_holder.error_msg); - return !error_msg.empty(); + return Strutil::pvt::has_error(); } @@ -346,11 +303,7 @@ has_error() std::string geterror(bool clear) { - std::string& error_msg(error_msg_holder.error_msg); - std::string e = error_msg; - if (clear) - error_msg.clear(); - return e; + return Strutil::pvt::geterror(clear); } diff --git a/src/libutil/strutil.cpp b/src/libutil/strutil.cpp index 2435afd534..1a39b7798a 100644 --- a/src/libutil/strutil.cpp +++ b/src/libutil/strutil.cpp @@ -172,9 +172,86 @@ OIIO_UTIL_API int OIIO_UTIL_API int oiio_print_debug(oiio_debug_env ? Strutil::stoi(oiio_debug_env) : 1); #endif +OIIO_UTIL_API int oiio_print_uncaught_errors(1); } // namespace pvt + +// ErrorHolder houses a string, with the addition that when it is destroyed, +// it will disgorge any un-retrieved error messages, in an effort to help +// beginning users diagnose their problems if they have forgotten to call +// geterror(). +struct ErrorHolder { + std::string error_msg; + + ~ErrorHolder() + { + if (!error_msg.empty() && pvt::oiio_print_uncaught_errors) { + OIIO::print( + "OpenImageIO exited with a pending error message that was never\n" + "retrieved via OIIO::geterror(). This was the error message:\n{}\n", + error_msg); + } + } +}; + + +// To avoid thread oddities, we have the storage area buffering error +// messages for append_error()/geterror() be thread-specific. +static thread_local ErrorHolder error_msg_holder; + + +void +Strutil::pvt::append_error(string_view message) +{ + // Remove a single trailing newline + if (message.size() && message.back() == '\n') + message.remove_suffix(1); + std::string& error_msg(error_msg_holder.error_msg); + OIIO_ASSERT( + error_msg.size() < 1024 * 1024 * 16 + && "Accumulated error messages > 16MB. Try checking return codes!"); + // If we are appending to existing error messages, separate them with + // a single newline. + if (error_msg.size() && error_msg.back() != '\n') + error_msg += '\n'; + error_msg += std::string(message); + + // Remove a single trailing newline + if (message.size() && message.back() == '\n') + message.remove_suffix(1); + error_msg = std::string(message); +} + + +bool +Strutil::pvt::has_error() +{ + std::string& error_msg(error_msg_holder.error_msg); + return !error_msg.empty(); +} + + +std::string +Strutil::pvt::geterror(bool clear) +{ + std::string& error_msg(error_msg_holder.error_msg); + std::string e = error_msg; + if (clear) + error_msg.clear(); + return e; +} + + +void +pvt::log_fmt_error(const char* message) +{ + print("fmt exception: {}\n", message); + Strutil::pvt::append_error(std::string("fmt exception: ") + message); +} + + + void Strutil::pvt::debug(string_view message) { diff --git a/src/oiiotool/oiiotool.cpp b/src/oiiotool/oiiotool.cpp index 6abe16520e..1ef257037e 100644 --- a/src/oiiotool/oiiotool.cpp +++ b/src/oiiotool/oiiotool.cpp @@ -6372,6 +6372,12 @@ Oiiotool::getargs(int argc, char* argv[]) ap.arg("--crash") .hidden() .action(crash_me); + ap.arg("--test-bad-format") + .hidden() + .action([&](cspan){ + print("{}\n", Strutil::fmt::format("hey hey {:d} {}", + "foo", "bar", "oops")); + }); ap.separator("Commands that read images:"); ap.arg("-i %s:FILENAME") diff --git a/testsuite/oiiotool/ref/out.txt b/testsuite/oiiotool/ref/out.txt index e5532a1f56..e5b24d1dfc 100644 --- a/testsuite/oiiotool/ref/out.txt +++ b/testsuite/oiiotool/ref/out.txt @@ -57,6 +57,12 @@ half data[2][2][3] = { /* (0, 1): */ { 0.000000000, 0.000000000, 0.000000000 }, /* (1, 1): */ { 1.000000000, 1.000000000, 0.000000000 } }, }; +testing bad format +fmt exception: invalid format specifier +hey hey foo bar +OpenImageIO exited with a pending error message that was never +retrieved via OIIO::geterror(). This was the error message: +fmt exception: invalid format specifier Comparing "filled.tif" and "ref/filled.tif" PASS Comparing "autotrim.tif" and "ref/autotrim.tif" diff --git a/testsuite/oiiotool/run.py b/testsuite/oiiotool/run.py index 315406f7f0..ec5c661544 100755 --- a/testsuite/oiiotool/run.py +++ b/testsuite/oiiotool/run.py @@ -234,6 +234,9 @@ command += oiiotool ("-echo dumpdata: --dumpdata dump.exr") command += oiiotool ("-echo dumpdata:C --dumpdata:C=data dump.exr") +# Test printing uncaught errors (and finding bad fmt strings) +command += oiiotool ("--echo \"testing bad format\" --test-bad-format") + # To add more tests, just append more lines like the above and also add # the new 'feature.tif' (or whatever you call it) to the outputs list, # below.