fix: Don't let fmtlib exceptions crash the app (AcademySoftwareFounda…

…tion#4400)

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 AcademySoftwareFoundation#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 <[email protected]>
Signed-off-by: Zach Lewis <[email protected]>

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

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

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;

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<float> oiio_missingcolor;
 }  // namespace pvt
 
@@ -285,72 +284,26 @@ 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);
 }
 
 
 
 bool
 has_error()
 {
-    std::string& error_msg(error_msg_holder.error_msg);
-    return !error_msg.empty();
+    return Strutil::pvt::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);
 }
 
 

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)
 {

src/oiiotool/oiiotool.cpp

@@ -6395,6 +6395,12 @@ Oiiotool::getargs(int argc, char* argv[])
     ap.arg("--crash")
       .hidden()
       .action(crash_me);
+    ap.arg("--test-bad-format")
+      .hidden()
+      .action([&](cspan<const char*>){
+                  print("{}\n", Strutil::fmt::format("hey hey {:d} {}",
+                                                     "foo", "bar", "oops"));
+              });
 
     ap.separator("Commands that read images:");
     ap.arg("-i %s:FILENAME")

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"

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.