Commit 4ecf6e3f authored by Tudor Bosman's avatar Tudor Bosman Committed by Viswanath Sivakumar

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
parent 1ae1c1ff
...@@ -154,55 +154,10 @@ BaseFormatter<Derived, containerMode, Args...>::BaseFormatter(StringPiece str, ...@@ -154,55 +154,10 @@ BaseFormatter<Derived, containerMode, Args...>::BaseFormatter(StringPiece str,
"Exactly one argument required in container mode"); "Exactly one argument required in container mode");
} }
template <class Derived, bool containerMode, class... Args>
void BaseFormatter<Derived, containerMode, Args...>::handleFormatStrError()
const {
if (crashOnError_) {
LOG(FATAL) << "folly::format: bad format string \"" << str_ << "\": " <<
folly::exceptionStr(std::current_exception());
}
throw;
}
template <class Derived, bool containerMode, class... Args> template <class Derived, bool containerMode, class... Args>
template <class Output> template <class Output>
void BaseFormatter<Derived, containerMode, Args...>::operator()(Output& out) void BaseFormatter<Derived, containerMode, Args...>::operator()(Output& out)
const { 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 <class Derived, bool containerMode, class... Args>
template <class Output>
void BaseFormatter<Derived, containerMode, Args...>::appendOutput(Output& out)
const {
// Copy raw string (without format specifiers) to output; // Copy raw string (without format specifiers) to output;
// not as simple as we'd like, as we still need to translate "}}" to "}" // not as simple as we'd like, as we still need to translate "}}" to "}"
// and throw if we see any lone "}" // and throw if we see any lone "}"
......
...@@ -71,19 +71,6 @@ class FormatterTag {}; ...@@ -71,19 +71,6 @@ class FormatterTag {};
template <class Derived, bool containerMode, class... Args> template <class Derived, bool containerMode, class... Args>
class BaseFormatter { class BaseFormatter {
public: 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) * Append to output. out(StringPiece sp) may be called (more than once)
*/ */
...@@ -129,10 +116,6 @@ class BaseFormatter { ...@@ -129,10 +116,6 @@ class BaseFormatter {
typename std::decay<Args>::type>...> ValueTuple; typename std::decay<Args>::type>...> ValueTuple;
static constexpr size_t valueCount = std::tuple_size<ValueTuple>::value; static constexpr size_t valueCount = std::tuple_size<ValueTuple>::value;
FOLLY_NORETURN void handleFormatStrError() const;
template <class Output>
void appendOutput(Output& out) const;
template <size_t K, class Callback> template <size_t K, class Callback>
typename std::enable_if<K == valueCount>::type typename std::enable_if<K == valueCount>::type
doFormatFrom(size_t i, FormatArg& arg, Callback& cb) const { doFormatFrom(size_t i, FormatArg& arg, Callback& cb) const {
...@@ -155,7 +138,6 @@ class BaseFormatter { ...@@ -155,7 +138,6 @@ class BaseFormatter {
} }
StringPiece str_; StringPiece str_;
bool crashOnError_{true};
protected: protected:
explicit BaseFormatter(StringPiece str, Args&&... args); explicit BaseFormatter(StringPiece str, Args&&... args);
...@@ -196,12 +178,8 @@ class Formatter : public BaseFormatter<Formatter<containerMode, Args...>, ...@@ -196,12 +178,8 @@ class Formatter : public BaseFormatter<Formatter<containerMode, Args...>,
template <class... A> template <class... A>
friend Formatter<false, A...> format(StringPiece fmt, A&&... arg); friend Formatter<false, A...> format(StringPiece fmt, A&&... arg);
template <class... A>
friend Formatter<false, A...> formatChecked(StringPiece fmt, A&&... arg);
template <class C> template <class C>
friend Formatter<true, C> vformat(StringPiece fmt, C&& container); friend Formatter<true, C> vformat(StringPiece fmt, C&& container);
template <class C>
friend Formatter<true, C> vformatChecked(StringPiece fmt, C&& container);
}; };
/** /**
...@@ -228,16 +206,6 @@ void writeTo(FILE* fp, ...@@ -228,16 +206,6 @@ void writeTo(FILE* fp,
* std::string formatted = format("{} {}", 23, 42).str(); * std::string formatted = format("{} {}", 23, 42).str();
* LOG(INFO) << format("{} {}", 23, 42); * LOG(INFO) << format("{} {}", 23, 42);
* writeTo(stdout, 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 <class... Args> template <class... Args>
Formatter<false, Args...> format(StringPiece fmt, Args&&... args) { Formatter<false, Args...> format(StringPiece fmt, Args&&... args) {
...@@ -254,30 +222,6 @@ inline std::string sformat(StringPiece fmt, Args&&... args) { ...@@ -254,30 +222,6 @@ inline std::string sformat(StringPiece fmt, Args&&... args) {
return format(fmt, std::forward<Args>(args)...).str(); return format(fmt, std::forward<Args>(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 <class... Args>
Formatter<false, Args...> formatChecked(StringPiece fmt, Args&&... args) {
Formatter<false, Args...> f(fmt, std::forward<Args>(args)...);
f.setCrashOnError(false);
return f;
}
/**
* Like formatChecked(), but immediately returns the formatted string instead of
* an intermediate format object.
*/
template <class... Args>
inline std::string sformatChecked(StringPiece fmt, Args&&... args) {
return formatChecked(fmt, std::forward<Args>(args)...).str();
}
/** /**
* Create a formatter object that takes one argument (of container type) * Create a formatter object that takes one argument (of container type)
* and uses that container to get argument values from. * and uses that container to get argument values from.
...@@ -306,31 +250,6 @@ inline std::string svformat(StringPiece fmt, Container&& container) { ...@@ -306,31 +250,6 @@ inline std::string svformat(StringPiece fmt, Container&& container) {
return vformat(fmt, std::forward<Container>(container)).str(); return vformat(fmt, std::forward<Container>(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 <class Container>
Formatter<true, Container> vformatChecked(StringPiece fmt,
Container&& container) {
Formatter<true, Container> f(fmt, std::forward<Container>(container));
f.setCrashOnError(false);
return f;
}
/**
* Like vformatChecked(), but immediately returns the formatted string instead
* of an intermediate format object.
*/
template <class Container>
inline std::string svformatChecked(StringPiece fmt, Container&& container) {
return vformatChecked(fmt, std::forward<Container>(container)).str();
}
/** /**
* Wrap a sequence or associative container so that out-of-range lookups * Wrap a sequence or associative container so that out-of-range lookups
* return a default value rather than throwing an exception. * return a default value rather than throwing an exception.
...@@ -370,12 +289,6 @@ format(Str* out, StringPiece fmt, Args&&... args) { ...@@ -370,12 +289,6 @@ format(Str* out, StringPiece fmt, Args&&... args) {
format(fmt, std::forward<Args>(args)...).appendTo(*out); format(fmt, std::forward<Args>(args)...).appendTo(*out);
} }
template <class Str, class... Args>
typename std::enable_if<IsSomeString<Str>::value>::type
formatChecked(Str* out, StringPiece fmt, Args&&... args) {
formatChecked(fmt, std::forward<Args>(args)...).appendTo(*out);
}
/** /**
* Append vformatted output to a string. * Append vformatted output to a string.
*/ */
...@@ -385,12 +298,6 @@ vformat(Str* out, StringPiece fmt, Container&& container) { ...@@ -385,12 +298,6 @@ vformat(Str* out, StringPiece fmt, Container&& container) {
vformat(fmt, std::forward<Container>(container)).appendTo(*out); vformat(fmt, std::forward<Container>(container)).appendTo(*out);
} }
template <class Str, class Container>
typename std::enable_if<IsSomeString<Str>::value>::type
vformatChecked(Str* out, StringPiece fmt, Container&& container) {
vformatChecked(fmt, std::forward<Container>(container)).appendTo(*out);
}
/** /**
* Utilities for all format value specializations. * Utilities for all format value specializations.
*/ */
...@@ -468,6 +375,36 @@ struct IsFormatter< ...@@ -468,6 +375,36 @@ struct IsFormatter<
type> : public std::true_type {}; type> : public std::true_type {};
} // folly::detail } // folly::detail
// Deprecated API. formatChecked() et. al. now behave identically to their
// non-Checked counterparts.
template <class... Args>
Formatter<false, Args...> formatChecked(StringPiece fmt, Args&&... args) {
return format(fmt, std::forward<Args>(args)...);
}
template <class... Args>
inline std::string sformatChecked(StringPiece fmt, Args&&... args) {
return formatChecked(fmt, std::forward<Args>(args)...).str();
}
template <class Container>
Formatter<true, Container> vformatChecked(StringPiece fmt,
Container&& container) {
return vformat(fmt, std::forward<Container>(container));
}
template <class Container>
inline std::string svformatChecked(StringPiece fmt, Container&& container) {
return vformatChecked(fmt, std::forward<Container>(container)).str();
}
template <class Str, class... Args>
typename std::enable_if<IsSomeString<Str>::value>::type
formatChecked(Str* out, StringPiece fmt, Args&&... args) {
formatChecked(fmt, std::forward<Args>(args)...).appendTo(*out);
}
template <class Str, class Container>
typename std::enable_if<IsSomeString<Str>::value>::type
vformatChecked(Str* out, StringPiece fmt, Container&& container) {
vformatChecked(fmt, std::forward<Container>(container)).appendTo(*out);
}
} // namespace folly } // namespace folly
#include <folly/Format-inl.h> #include <folly/Format-inl.h>
......
...@@ -419,11 +419,37 @@ struct Opaque { ...@@ -419,11 +419,37 @@ struct Opaque {
} // namespace } // 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) { TEST(Format, Unformatted) {
Opaque o; Opaque o;
EXPECT_NE("", sformat("{}", &o)); EXPECT_NE("", sformat("{}", &o));
EXPECT_DEATH(sformat("{0[0]}", &o), "No formatter available for this type"); EXPECT_FORMAT_ERROR(sformat("{0[0]}", &o),
EXPECT_THROW(sformatChecked("{0[0]}", &o), std::invalid_argument); "No formatter available for this type");
} }
TEST(Format, Nested) { TEST(Format, Nested) {
...@@ -438,39 +464,28 @@ TEST(Format, OutOfBounds) { ...@@ -438,39 +464,28 @@ TEST(Format, OutOfBounds) {
std::vector<int> ints{1, 2, 3, 4, 5}; std::vector<int> ints{1, 2, 3, 4, 5};
EXPECT_EQ("1 3 5", sformat("{0[0]} {0[2]} {0[4]}", ints)); EXPECT_EQ("1 3 5", sformat("{0[0]} {0[2]} {0[4]}", ints));
EXPECT_THROW(sformat("{[5]}", ints), std::out_of_range); EXPECT_THROW(sformat("{[5]}", ints), std::out_of_range);
EXPECT_THROW(sformatChecked("{[5]}", ints), std::out_of_range);
std::map<std::string, int> map{{"hello", 0}, {"world", 1}}; std::map<std::string, int> map{{"hello", 0}, {"world", 1}};
EXPECT_EQ("hello = 0", sformat("hello = {[hello]}", map)); EXPECT_EQ("hello = 0", sformat("hello = {[hello]}", map));
EXPECT_THROW(sformat("{[nope]}", map), std::out_of_range); EXPECT_THROW(sformat("{[nope]}", map), std::out_of_range);
EXPECT_THROW(svformat("{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) { TEST(Format, BogusFormatString) {
// format() will crash the program if the format string is invalid. // format() will crash the program if the format string is invalid.
EXPECT_DEATH(sformat("}"), "single '}' in format string"); EXPECT_FORMAT_ERROR(sformat("}"), "single '}' in format string");
EXPECT_DEATH(sformat("foo}bar"), "single '}' in format string"); EXPECT_FORMAT_ERROR(sformat("foo}bar"), "single '}' in format string");
EXPECT_DEATH(sformat("foo{bar"), "missing ending '}'"); EXPECT_FORMAT_ERROR(sformat("foo{bar"), "missing ending '}'");
EXPECT_DEATH(sformat("{[test]"), "missing ending '}'"); EXPECT_FORMAT_ERROR(sformat("{[test]"), "missing ending '}'");
EXPECT_DEATH(sformat("{-1.3}"), "argument index must be non-negative"); EXPECT_FORMAT_ERROR(sformat("{-1.3}"), "argument index must be non-negative");
EXPECT_DEATH(sformat("{1.3}", 0, 1, 2), "index not allowed"); EXPECT_FORMAT_ERROR(sformat("{1.3}", 0, 1, 2), "index not allowed");
EXPECT_DEATH(sformat("{0} {} {1}", 0, 1, 2), EXPECT_FORMAT_ERROR(sformat("{0} {} {1}", 0, 1, 2),
"may not have both default and explicit arg indexes"); "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 // This one fails in detail::enforceWhitespace(), which throws
// std::range_error // std::range_error
EXPECT_DEATH(sformat("{0[test}"), "Non-whitespace: \\["); EXPECT_THROW_STR(sformat("{0[test}"), std::range_error,
EXPECT_THROW(sformatChecked("{0[test}"), std::exception); "Non-whitespace: [");
} }
template <bool containerMode, class... Args> template <bool containerMode, class... Args>
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment