From 4ecf6e3f6d120907c0e32059a923d15cd6fcfedc Mon Sep 17 00:00:00 2001 From: Tudor Bosman Date: Fri, 15 May 2015 19:29:05 -0700 Subject: [PATCH] Make folly::format no longer crash on invalid format strings Summary: Yes, ideally we'd detect this at compile time, but if we can't, causing a SEV1 is not the best way to do it. format() now behaves like formatChecked(); the old API is maintained for backwards compatibility, but deprecated. Test Plan: format_test Reviewed By: simpkins@fb.com Subscribers: trunkagent, dpittman, jdelong, sdmich, net-systems@, dmitri, folly-diffs@, yfeldblum, andrii, mssarang, chalfant FB internal diff: D2075829 Tasks: 7095768 Signature: t1:2075829:1431713985:b3fdec4820104b4ddc4be0b6af999db174a692d9 --- folly/Format-inl.h | 45 -------------- folly/Format.h | 123 ++++++++++---------------------------- folly/test/FormatTest.cpp | 59 +++++++++++------- 3 files changed, 67 insertions(+), 160 deletions(-) diff --git a/folly/Format-inl.h b/folly/Format-inl.h index 9f6b26a7..c7ebb9f3 100644 --- a/folly/Format-inl.h +++ b/folly/Format-inl.h @@ -154,55 +154,10 @@ BaseFormatter::BaseFormatter(StringPiece str, "Exactly one argument required in container mode"); } -template -void BaseFormatter::handleFormatStrError() - const { - if (crashOnError_) { - LOG(FATAL) << "folly::format: bad format string \"" << str_ << "\": " << - folly::exceptionStr(std::current_exception()); - } - throw; -} - template template void BaseFormatter::operator()(Output& out) const { - // Catch BadFormatArg and range_error exceptions, and call - // handleFormatStrError(). - // - // These exception types indicate a problem with the format string. Most - // format strings are string literals specified by the programmer. If they - // have a problem, this is usually a programmer bug. We want to crash to - // ensure that these are found early on during development. - // - // BadFormatArg is thrown by the Format.h code, while range_error is thrown - // by Conv.h, which is used in several places in our format string - // processing. - // - // (Note: This behavior is slightly dangerous. If the Output object throws a - // BadFormatArg or a range_error, we will also crash the program, even if it - // wasn't an issue with the format string. This seems highly unlikely - // though, and none of our current Output objects can throw these errors.) - // - // We also throw out_of_range errors if the format string references an - // argument that isn't present (or a key that isn't present in one of the - // argument containers). However, at the moment we don't crash on these - // errors, as it is likely that the container is dynamic at runtime. - try { - appendOutput(out); - } catch (const BadFormatArg& ex) { - handleFormatStrError(); - } catch (const std::range_error& ex) { - handleFormatStrError(); - } -} - -template -template -void BaseFormatter::appendOutput(Output& out) - const { - // Copy raw string (without format specifiers) to output; // not as simple as we'd like, as we still need to translate "}}" to "}" // and throw if we see any lone "}" diff --git a/folly/Format.h b/folly/Format.h index c88cd273..7ffc1527 100644 --- a/folly/Format.h +++ b/folly/Format.h @@ -71,19 +71,6 @@ class FormatterTag {}; template class BaseFormatter { public: - /* - * Change whether or not Formatter should crash or throw exceptions if the - * format string is invalid. - * - * Crashing is desirable for literal format strings that are fixed at compile - * time. Errors in the format string are generally programmer bugs, and - * should be caught early on in development. Crashing helps ensure these - * problems are noticed. - */ - void setCrashOnError(bool crash) { - crashOnError_ = crash; - } - /** * Append to output. out(StringPiece sp) may be called (more than once) */ @@ -129,10 +116,6 @@ class BaseFormatter { typename std::decay::type>...> ValueTuple; static constexpr size_t valueCount = std::tuple_size::value; - FOLLY_NORETURN void handleFormatStrError() const; - template - void appendOutput(Output& out) const; - template typename std::enable_if::type doFormatFrom(size_t i, FormatArg& arg, Callback& cb) const { @@ -155,7 +138,6 @@ class BaseFormatter { } StringPiece str_; - bool crashOnError_{true}; protected: explicit BaseFormatter(StringPiece str, Args&&... args); @@ -196,12 +178,8 @@ class Formatter : public BaseFormatter, template friend Formatter format(StringPiece fmt, A&&... arg); - template - friend Formatter formatChecked(StringPiece fmt, A&&... arg); template friend Formatter vformat(StringPiece fmt, C&& container); - template - friend Formatter vformatChecked(StringPiece fmt, C&& container); }; /** @@ -228,16 +206,6 @@ void writeTo(FILE* fp, * std::string formatted = format("{} {}", 23, 42).str(); * LOG(INFO) << format("{} {}", 23, 42); * writeTo(stdout, format("{} {}", 23, 42)); - * - * Note that format() will crash the program if the format string is invalid. - * Normally, the format string is a fixed string literal specified by the - * programmer. Invalid format strings are normally programmer bugs, and should - * be caught early on during development. Crashing helps ensure these bugs are - * found. - * - * Use formatChecked() if you have a dynamic format string (for example, a user - * supplied value). formatChecked() will throw an exception rather than - * crashing the program. */ template Formatter format(StringPiece fmt, Args&&... args) { @@ -254,30 +222,6 @@ inline std::string sformat(StringPiece fmt, Args&&... args) { return format(fmt, std::forward(args)...).str(); } -/** - * Create a formatter object from a dynamic format string. - * - * This is identical to format(), but throws an exception if the format string - * is invalid, rather than aborting the program. This allows it to be used - * with user-specified format strings which are not guaranteed to be well - * formed. - */ -template -Formatter formatChecked(StringPiece fmt, Args&&... args) { - Formatter f(fmt, std::forward(args)...); - f.setCrashOnError(false); - return f; -} - -/** - * Like formatChecked(), but immediately returns the formatted string instead of - * an intermediate format object. - */ -template -inline std::string sformatChecked(StringPiece fmt, Args&&... args) { - return formatChecked(fmt, std::forward(args)...).str(); -} - /** * Create a formatter object that takes one argument (of container type) * and uses that container to get argument values from. @@ -306,31 +250,6 @@ inline std::string svformat(StringPiece fmt, Container&& container) { return vformat(fmt, std::forward(container)).str(); } -/** - * Create a formatter object from a dynamic format string. - * - * This is identical to vformat(), but throws an exception if the format string - * is invalid, rather than aborting the program. This allows it to be used - * with user-specified format strings which are not guaranteed to be well - * formed. - */ -template -Formatter vformatChecked(StringPiece fmt, - Container&& container) { - Formatter f(fmt, std::forward(container)); - f.setCrashOnError(false); - return f; -} - -/** - * Like vformatChecked(), but immediately returns the formatted string instead - * of an intermediate format object. - */ -template -inline std::string svformatChecked(StringPiece fmt, Container&& container) { - return vformatChecked(fmt, std::forward(container)).str(); -} - /** * Wrap a sequence or associative container so that out-of-range lookups * return a default value rather than throwing an exception. @@ -370,12 +289,6 @@ format(Str* out, StringPiece fmt, Args&&... args) { format(fmt, std::forward(args)...).appendTo(*out); } -template -typename std::enable_if::value>::type -formatChecked(Str* out, StringPiece fmt, Args&&... args) { - formatChecked(fmt, std::forward(args)...).appendTo(*out); -} - /** * Append vformatted output to a string. */ @@ -385,12 +298,6 @@ vformat(Str* out, StringPiece fmt, Container&& container) { vformat(fmt, std::forward(container)).appendTo(*out); } -template -typename std::enable_if::value>::type -vformatChecked(Str* out, StringPiece fmt, Container&& container) { - vformatChecked(fmt, std::forward(container)).appendTo(*out); -} - /** * Utilities for all format value specializations. */ @@ -468,6 +375,36 @@ struct IsFormatter< type> : public std::true_type {}; } // folly::detail +// Deprecated API. formatChecked() et. al. now behave identically to their +// non-Checked counterparts. +template +Formatter formatChecked(StringPiece fmt, Args&&... args) { + return format(fmt, std::forward(args)...); +} +template +inline std::string sformatChecked(StringPiece fmt, Args&&... args) { + return formatChecked(fmt, std::forward(args)...).str(); +} +template +Formatter vformatChecked(StringPiece fmt, + Container&& container) { + return vformat(fmt, std::forward(container)); +} +template +inline std::string svformatChecked(StringPiece fmt, Container&& container) { + return vformatChecked(fmt, std::forward(container)).str(); +} +template +typename std::enable_if::value>::type +formatChecked(Str* out, StringPiece fmt, Args&&... args) { + formatChecked(fmt, std::forward(args)...).appendTo(*out); +} +template +typename std::enable_if::value>::type +vformatChecked(Str* out, StringPiece fmt, Container&& container) { + vformatChecked(fmt, std::forward(container)).appendTo(*out); +} + } // namespace folly #include diff --git a/folly/test/FormatTest.cpp b/folly/test/FormatTest.cpp index 5a751cc1..5ad38ef6 100644 --- a/folly/test/FormatTest.cpp +++ b/folly/test/FormatTest.cpp @@ -419,11 +419,37 @@ struct Opaque { } // namespace +#define EXPECT_THROW_STR(code, type, str) \ + do { \ + bool caught = false; \ + try { \ + code; \ + } catch (const type& e) { \ + caught = true; \ + EXPECT_TRUE(strstr(e.what(), (str)) != nullptr) << \ + "Expected message [" << (str) << "], actual message [" << \ + e.what(); \ + } catch (const std::exception& e) { \ + caught = true; \ + ADD_FAILURE() << "Caught different exception type; expected " #type \ + ", caught " << folly::demangle(typeid(e)); \ + } catch (...) { \ + caught = true; \ + ADD_FAILURE() << "Caught unknown exception type; expected " #type; \ + } \ + if (!caught) { \ + ADD_FAILURE() << "Expected exception " #type ", caught nothing"; \ + } \ + } while (false) + +#define EXPECT_FORMAT_ERROR(code, str) \ + EXPECT_THROW_STR(code, folly::BadFormatArg, (str)) + TEST(Format, Unformatted) { Opaque o; EXPECT_NE("", sformat("{}", &o)); - EXPECT_DEATH(sformat("{0[0]}", &o), "No formatter available for this type"); - EXPECT_THROW(sformatChecked("{0[0]}", &o), std::invalid_argument); + EXPECT_FORMAT_ERROR(sformat("{0[0]}", &o), + "No formatter available for this type"); } TEST(Format, Nested) { @@ -438,39 +464,28 @@ TEST(Format, OutOfBounds) { std::vector ints{1, 2, 3, 4, 5}; EXPECT_EQ("1 3 5", sformat("{0[0]} {0[2]} {0[4]}", ints)); EXPECT_THROW(sformat("{[5]}", ints), std::out_of_range); - EXPECT_THROW(sformatChecked("{[5]}", ints), std::out_of_range); std::map map{{"hello", 0}, {"world", 1}}; EXPECT_EQ("hello = 0", sformat("hello = {[hello]}", map)); EXPECT_THROW(sformat("{[nope]}", map), std::out_of_range); EXPECT_THROW(svformat("{nope}", map), std::out_of_range); - EXPECT_THROW(svformatChecked("{nope}", map), std::out_of_range); } TEST(Format, BogusFormatString) { // format() will crash the program if the format string is invalid. - EXPECT_DEATH(sformat("}"), "single '}' in format string"); - EXPECT_DEATH(sformat("foo}bar"), "single '}' in format string"); - EXPECT_DEATH(sformat("foo{bar"), "missing ending '}'"); - EXPECT_DEATH(sformat("{[test]"), "missing ending '}'"); - EXPECT_DEATH(sformat("{-1.3}"), "argument index must be non-negative"); - EXPECT_DEATH(sformat("{1.3}", 0, 1, 2), "index not allowed"); - EXPECT_DEATH(sformat("{0} {} {1}", 0, 1, 2), + EXPECT_FORMAT_ERROR(sformat("}"), "single '}' in format string"); + EXPECT_FORMAT_ERROR(sformat("foo}bar"), "single '}' in format string"); + EXPECT_FORMAT_ERROR(sformat("foo{bar"), "missing ending '}'"); + EXPECT_FORMAT_ERROR(sformat("{[test]"), "missing ending '}'"); + EXPECT_FORMAT_ERROR(sformat("{-1.3}"), "argument index must be non-negative"); + EXPECT_FORMAT_ERROR(sformat("{1.3}", 0, 1, 2), "index not allowed"); + EXPECT_FORMAT_ERROR(sformat("{0} {} {1}", 0, 1, 2), "may not have both default and explicit arg indexes"); - // formatChecked() should throw exceptions rather than crashing the program - EXPECT_THROW(sformatChecked("}"), std::invalid_argument); - EXPECT_THROW(sformatChecked("foo}bar"), std::invalid_argument); - EXPECT_THROW(sformatChecked("foo{bar"), std::invalid_argument); - EXPECT_THROW(sformatChecked("{[test]"), std::invalid_argument); - EXPECT_THROW(sformatChecked("{-1.3}"), std::invalid_argument); - EXPECT_THROW(sformatChecked("{1.3}", 0, 1, 2), std::invalid_argument); - EXPECT_THROW(sformatChecked("{0} {} {1}", 0, 1, 2), std::invalid_argument); - // This one fails in detail::enforceWhitespace(), which throws // std::range_error - EXPECT_DEATH(sformat("{0[test}"), "Non-whitespace: \\["); - EXPECT_THROW(sformatChecked("{0[test}"), std::exception); + EXPECT_THROW_STR(sformat("{0[test}"), std::range_error, + "Non-whitespace: ["); } template -- 2.34.1