From 22f75d8b6d7b2f8e2b7fbb78c8d54afa440d6e1d Mon Sep 17 00:00:00 2001 From: Victor Zverovich Date: Wed, 3 Sep 2014 08:03:05 -0700 Subject: [PATCH] Don't throw exceptions from error formatting functions. Gracefully fallback to a less descriptive message instead. --- format.cc | 88 +++++++++++++++++++++++++++++------------------ format.h | 15 ++++---- test/util-test.cc | 12 +++++-- 3 files changed, 73 insertions(+), 42 deletions(-) diff --git a/format.cc b/format.cc index 9e9d504b..38f899c8 100644 --- a/format.cc +++ b/format.cc @@ -136,13 +136,33 @@ struct IntChecker { const char RESET_COLOR[] = "\x1b[0m"; -typedef void (*FormatFunc)(fmt::Writer &, int , fmt::StringRef); +typedef void (*FormatFunc)(fmt::Writer &, int, fmt::StringRef); + +// TODO: test +void format_error_code(fmt::Writer &out, int error_code, + fmt::StringRef message) FMT_NOEXCEPT(true) { + // Report error code making sure that the output fits into + // INLINE_BUFFER_SIZE to avoid dynamic memory allocation and potential + // bad_alloc. + out.clear(); + static const char SEP[] = ": "; + static const char ERROR[] = "error "; + // SEP and ERROR add two terminating null characters, so subtract 1 as + // we need only one. + fmt::internal::IntTraits::MainType ec_value = error_code; + std::size_t error_code_size = + sizeof(SEP) + sizeof(ERROR) + fmt::internal::count_digits(ec_value) - 1; + if (message.size() < fmt::internal::INLINE_BUFFER_SIZE - error_code_size) + out << message << SEP; + out << ERROR << error_code; + assert(out.size() <= fmt::internal::INLINE_BUFFER_SIZE); +} void report_error(FormatFunc func, int error_code, fmt::StringRef message) FMT_NOEXCEPT(true) { try { fmt::Writer full_message; - func(full_message, error_code, message); // TODO: make sure this doesn't throw + func(full_message, error_code, message); std::fwrite(full_message.c_str(), full_message.size(), 1, stderr); std::fputc('\n', stderr); } catch (...) {} @@ -453,28 +473,31 @@ int fmt::internal::safe_strerror( } void fmt::internal::format_system_error( - fmt::Writer &out, int error_code, fmt::StringRef message) { - Array buffer; - buffer.resize(INLINE_BUFFER_SIZE); - char *system_message = 0; - for (;;) { - system_message = &buffer[0]; - int result = safe_strerror(error_code, system_message, buffer.size()); - if (result == 0) - break; - if (result != ERANGE) { - // Can't get error message, report error code instead. - out << message << ": error code = " << error_code; - return; + fmt::Writer &out, int error_code, + fmt::StringRef message) FMT_NOEXCEPT(true) { + try { + Array buffer; + buffer.resize(INLINE_BUFFER_SIZE); + char *system_message = 0; + for (;;) { + system_message = &buffer[0]; + int result = safe_strerror(error_code, system_message, buffer.size()); + if (result == 0) { + out << message << ": " << system_message; + return; + } + if (result != ERANGE) + break; // Can't get error message, report error code instead. + buffer.resize(buffer.size() * 2); } - buffer.resize(buffer.size() * 2); - } - out << message << ": " << system_message; + } catch (...) {} + format_error_code(out, error_code, message); } #ifdef _WIN32 void fmt::internal::format_windows_error( - fmt::Writer &out, int error_code, fmt::StringRef message) { + fmt::Writer &out, int error_code, + fmt::StringRef message) FMT_NOEXCEPT(true) { class String { private: LPWSTR str_; @@ -485,19 +508,20 @@ void fmt::internal::format_windows_error( LPWSTR *ptr() { return &str_; } LPCWSTR c_str() const { return str_; } }; - String system_message; - if (FormatMessageW(FORMAT_MESSAGE_ALLOCATE_BUFFER | - FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, 0, - error_code, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), - reinterpret_cast(system_message.ptr()), 0, 0)) { - UTF16ToUTF8 utf8_message; - if (!utf8_message.convert(system_message.c_str())) { - out << message << ": " << utf8_message; - return; + try { + String system_message; + if (FormatMessageW(FORMAT_MESSAGE_ALLOCATE_BUFFER | + FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, 0, + error_code, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), + reinterpret_cast(system_message.ptr()), 0, 0)) { + UTF16ToUTF8 utf8_message; + if (utf8_message.convert(system_message.c_str()) == ERROR_SUCCESS) { + out << message << ": " << utf8_message; + return; + } } - } - // Can't get error message, report error code instead. - out << message << ": error code = " << error_code; + } catch (...) {} + format_error_code(out, error_code, message); } #endif @@ -1190,14 +1214,12 @@ void fmt::BasicFormatter::format( void fmt::report_system_error( int error_code, fmt::StringRef message) FMT_NOEXCEPT(true) { - // FIXME: format_system_error may throw report_error(internal::format_system_error, error_code, message); } #ifdef _WIN32 void fmt::report_windows_error( int error_code, fmt::StringRef message) FMT_NOEXCEPT(true) { - // FIXME: format_windows_error may throw report_error(internal::format_windows_error, error_code, message); } #endif diff --git a/format.h b/format.h index 2f816292..abc6db7f 100644 --- a/format.h +++ b/format.h @@ -307,7 +307,7 @@ class Array { grow(capacity); } - void clear() { size_ = 0; } + void clear() FMT_NOEXCEPT(true) { size_ = 0; } void push_back(const T &value) { if (size_ == capacity_) @@ -528,7 +528,8 @@ class UTF16ToUTF8 { std::string str() const { return std::string(&buffer_[0], size()); } // Performs conversion returning a system error code instead of - // throwing exception on error. + // throwing exception on conversion error. This method may still throw + // in case of memory allocation error. int convert(WStringRef s); }; #endif @@ -545,12 +546,12 @@ class UTF16ToUTF8 { int safe_strerror(int error_code, char *&buffer, std::size_t buffer_size) FMT_NOEXCEPT(true); -void format_system_error( - fmt::Writer &out, int error_code, fmt::StringRef message); +void format_system_error(fmt::Writer &out, int error_code, + fmt::StringRef message) FMT_NOEXCEPT(true); #ifdef _WIN32 -void format_windows_error( - fmt::Writer &out, int error_code, fmt::StringRef message); +void format_windows_error(fmt::Writer &out, int error_code, + fmt::StringRef message) FMT_NOEXCEPT(true); #endif // Computes max(Arg, 1) at compile time. It is used to avoid errors about @@ -1518,7 +1519,7 @@ class BasicWriter { return *this; } - void clear() { buffer_.clear(); } + void clear() FMT_NOEXCEPT(true) { buffer_.clear(); } }; template diff --git a/test/util-test.cc b/test/util-test.cc index b0b80766..ec70da71 100644 --- a/test/util-test.cc +++ b/test/util-test.cc @@ -471,8 +471,11 @@ void check_throw_error(int error_code, FormatErrorMessage format) { TEST(UtilTest, FormatSystemError) { fmt::Writer message; fmt::internal::format_system_error(message, EDOM, "test"); - EXPECT_EQ(fmt::format("test: {}", - get_system_error(EDOM)), message.str()); + EXPECT_EQ(fmt::format("test: {}", get_system_error(EDOM)), message.str()); + message.clear(); + fmt::internal::format_system_error( + message, EDOM, fmt::StringRef("x", std::numeric_limits::max())); + EXPECT_EQ(fmt::format("error {}", EDOM), message.str()); } TEST(UtilTest, SystemError) { @@ -504,6 +507,11 @@ TEST(UtilTest, FormatWindowsError) { actual_message, ERROR_FILE_EXISTS, "test"); EXPECT_EQ(fmt::format("test: {}", utf8_message.str()), actual_message.str()); + actual_message.clear(); + fmt::internal::format_windows_error( + actual_message, ERROR_FILE_EXISTS, + fmt::StringRef("x", std::numeric_limits::max())); + EXPECT_EQ(fmt::format("error {}", ERROR_FILE_EXISTS), message.str()); } TEST(UtilTest, WindowsError) {