Commit 9bfa4150 authored by bsimmers's avatar bsimmers Committed by Jordan DeLong

abort instead of throwing an exception on bad format strings

Summary:
Errors that happen during format are almost always programmer
error in the form of a bad format string. Abort instead of throwing in
these cases.

Test Plan: automated tests, added a couple new tests

Reviewed By: tudorb@fb.com

FB internal diff: D947470
parent 38220af4
...@@ -168,10 +168,7 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const { ...@@ -168,10 +168,7 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const {
out(StringPiece(p, q)); out(StringPiece(p, q));
p = q; p = q;
if (p == end || *p != '}') { CHECK(p != end && *p == '}') << "single '}' in format string";
throw std::invalid_argument(
"folly::format: single '}' in format string");
}
++p; ++p;
} }
}; };
...@@ -188,10 +185,7 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const { ...@@ -188,10 +185,7 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const {
outputString(StringPiece(p, q)); outputString(StringPiece(p, q));
p = q + 1; p = q + 1;
if (p == end) { CHECK(p != end) << "'{' at end of format string";
throw std::invalid_argument(
"folly::format: '}' at end of format string");
}
// "{{" -> "{" // "{{" -> "{"
if (*p == '{') { if (*p == '{') {
...@@ -202,9 +196,7 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const { ...@@ -202,9 +196,7 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const {
// Format string // Format string
q = static_cast<const char*>(memchr(p, '}', end - p)); q = static_cast<const char*>(memchr(p, '}', end - p));
if (q == end) { CHECK(q != end) << "missing ending '}'";
throw std::invalid_argument("folly::format: missing ending '}'");
}
FormatArg arg(StringPiece(p, q)); FormatArg arg(StringPiece(p, q));
p = q + 1; p = q + 1;
...@@ -226,17 +218,16 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const { ...@@ -226,17 +218,16 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const {
try { try {
argIndex = to<int>(piece); argIndex = to<int>(piece);
} catch (const std::out_of_range& e) { } catch (const std::out_of_range& e) {
arg.error("argument index must be integer"); LOG(FATAL) << "argument index must be integer";
} }
arg.enforce(argIndex >= 0, "argument index must be non-negative"); CHECK(argIndex >= 0)
<< arg.errorStr("argument index must be non-negative");
hasExplicitArgIndex = true; hasExplicitArgIndex = true;
} }
} }
if (hasDefaultArgIndex && hasExplicitArgIndex) { CHECK(!hasDefaultArgIndex || !hasExplicitArgIndex)
throw std::invalid_argument( << "may not have both default and explicit arg indexes";
"folly::format: may not have both default and explicit arg indexes");
}
doFormat(argIndex, arg, out); doFormat(argIndex, arg, out);
} }
...@@ -400,8 +391,8 @@ class FormatValue< ...@@ -400,8 +391,8 @@ class FormatValue<
uval = val_; uval = val_;
sign = '\0'; sign = '\0';
arg.enforce(arg.sign == FormatArg::Sign::DEFAULT, CHECK(arg.sign == FormatArg::Sign::DEFAULT)
"sign specifications not allowed for unsigned values"); << arg.errorStr("sign specifications not allowed for unsigned values");
} }
// max of: // max of:
...@@ -428,9 +419,9 @@ class FormatValue< ...@@ -428,9 +419,9 @@ class FormatValue<
switch (presentation) { switch (presentation) {
case 'n': // TODO(tudorb): locale awareness? case 'n': // TODO(tudorb): locale awareness?
case 'd': case 'd':
arg.enforce(!arg.basePrefix, CHECK(!arg.basePrefix)
"base prefix not allowed with '", presentation, << arg.errorStr("base prefix not allowed with '", presentation,
"' specifier"); "' specifier");
if (arg.thousandsSeparator) { if (arg.thousandsSeparator) {
useSprintf("%'ju"); useSprintf("%'ju");
} else { } else {
...@@ -440,21 +431,21 @@ class FormatValue< ...@@ -440,21 +431,21 @@ class FormatValue<
} }
break; break;
case 'c': case 'c':
arg.enforce(!arg.basePrefix, CHECK(!arg.basePrefix)
"base prefix not allowed with '", presentation, << arg.errorStr("base prefix not allowed with '", presentation,
"' specifier"); "' specifier");
arg.enforce(!arg.thousandsSeparator, CHECK(!arg.thousandsSeparator)
"thousands separator (',') not allowed with '", << arg.errorStr("thousands separator (',') not allowed with '",
presentation, "' specifier"); presentation, "' specifier");
valBufBegin = valBuf + 3; valBufBegin = valBuf + 3;
*valBufBegin = static_cast<char>(uval); *valBufBegin = static_cast<char>(uval);
valBufEnd = valBufBegin + 1; valBufEnd = valBufBegin + 1;
break; break;
case 'o': case 'o':
case 'O': case 'O':
arg.enforce(!arg.thousandsSeparator, CHECK(!arg.thousandsSeparator)
"thousands separator (',') not allowed with '", << arg.errorStr("thousands separator (',') not allowed with '",
presentation, "' specifier"); presentation, "' specifier");
valBufEnd = valBuf + valBufSize - 1; valBufEnd = valBuf + valBufSize - 1;
valBufBegin = valBuf + detail::uintToOctal(valBuf, valBufSize - 1, uval); valBufBegin = valBuf + detail::uintToOctal(valBuf, valBufSize - 1, uval);
if (arg.basePrefix) { if (arg.basePrefix) {
...@@ -463,9 +454,9 @@ class FormatValue< ...@@ -463,9 +454,9 @@ class FormatValue<
} }
break; break;
case 'x': case 'x':
arg.enforce(!arg.thousandsSeparator, CHECK(!arg.thousandsSeparator)
"thousands separator (',') not allowed with '", << arg.errorStr("thousands separator (',') not allowed with '",
presentation, "' specifier"); presentation, "' specifier");
valBufEnd = valBuf + valBufSize - 1; valBufEnd = valBuf + valBufSize - 1;
valBufBegin = valBuf + detail::uintToHexLower(valBuf, valBufSize - 1, valBufBegin = valBuf + detail::uintToHexLower(valBuf, valBufSize - 1,
uval); uval);
...@@ -476,9 +467,9 @@ class FormatValue< ...@@ -476,9 +467,9 @@ class FormatValue<
} }
break; break;
case 'X': case 'X':
arg.enforce(!arg.thousandsSeparator, CHECK(!arg.thousandsSeparator)
"thousands separator (',') not allowed with '", << arg.errorStr("thousands separator (',') not allowed with '",
presentation, "' specifier"); presentation, "' specifier");
valBufEnd = valBuf + valBufSize - 1; valBufEnd = valBuf + valBufSize - 1;
valBufBegin = valBuf + detail::uintToHexUpper(valBuf, valBufSize - 1, valBufBegin = valBuf + detail::uintToHexUpper(valBuf, valBufSize - 1,
uval); uval);
...@@ -490,9 +481,9 @@ class FormatValue< ...@@ -490,9 +481,9 @@ class FormatValue<
break; break;
case 'b': case 'b':
case 'B': case 'B':
arg.enforce(!arg.thousandsSeparator, CHECK(!arg.thousandsSeparator)
"thousands separator (',') not allowed with '", << arg.errorStr("thousands separator (',') not allowed with '",
presentation, "' specifier"); presentation, "' specifier");
valBufEnd = valBuf + valBufSize - 1; valBufEnd = valBuf + valBufSize - 1;
valBufBegin = valBuf + detail::uintToBinary(valBuf, valBufSize - 1, valBufBegin = valBuf + detail::uintToBinary(valBuf, valBufSize - 1,
uval); uval);
...@@ -503,7 +494,7 @@ class FormatValue< ...@@ -503,7 +494,7 @@ class FormatValue<
} }
break; break;
default: default:
arg.error("invalid specifier '", presentation, "'"); LOG(FATAL) << arg.errorStr("invalid specifier '", presentation, "'");
} }
if (sign) { if (sign) {
...@@ -645,7 +636,7 @@ class FormatValue<double> { ...@@ -645,7 +636,7 @@ class FormatValue<double> {
} }
break; break;
default: default:
arg.error("invalid specifier '", arg.presentation, "'"); LOG(FATAL) << arg.errorStr("invalid specifier '", arg.presentation, "'");
} }
int len = builder.position(); int len = builder.position();
...@@ -702,9 +693,9 @@ class FormatValue< ...@@ -702,9 +693,9 @@ class FormatValue<
void format(FormatArg& arg, FormatCallback& cb) const { void format(FormatArg& arg, FormatCallback& cb) const {
if (arg.keyEmpty()) { if (arg.keyEmpty()) {
arg.validate(FormatArg::Type::OTHER); arg.validate(FormatArg::Type::OTHER);
arg.enforce(arg.presentation == FormatArg::kDefaultPresentation || CHECK(arg.presentation == FormatArg::kDefaultPresentation ||
arg.presentation == 's', arg.presentation == 's')
"invalid specifier '", arg.presentation, "'"); << arg.errorStr("invalid specifier '", arg.presentation, "'");
format_value::formatString(val_, arg, cb); format_value::formatString(val_, arg, cb);
} else { } else {
FormatValue<char>(val_.at(arg.splitIntKey())).format(arg, cb); FormatValue<char>(val_.at(arg.splitIntKey())).format(arg, cb);
...@@ -724,8 +715,8 @@ class FormatValue<std::nullptr_t> { ...@@ -724,8 +715,8 @@ class FormatValue<std::nullptr_t> {
template <class FormatCallback> template <class FormatCallback>
void format(FormatArg& arg, FormatCallback& cb) const { void format(FormatArg& arg, FormatCallback& cb) const {
arg.validate(FormatArg::Type::OTHER); arg.validate(FormatArg::Type::OTHER);
arg.enforce(arg.presentation == FormatArg::kDefaultPresentation, CHECK(arg.presentation == FormatArg::kDefaultPresentation)
"invalid specifier '", arg.presentation, "'"); << arg.errorStr("invalid specifier '", arg.presentation, "'");
format_value::formatString("(null)", arg, cb); format_value::formatString("(null)", arg, cb);
} }
}; };
...@@ -775,8 +766,8 @@ class FormatValue< ...@@ -775,8 +766,8 @@ class FormatValue<
} else { } else {
// Print as a pointer, in hex. // Print as a pointer, in hex.
arg.validate(FormatArg::Type::OTHER); arg.validate(FormatArg::Type::OTHER);
arg.enforce(arg.presentation == FormatArg::kDefaultPresentation, CHECK(arg.presentation == FormatArg::kDefaultPresentation)
"invalid specifier '", arg.presentation, "'"); << arg.errorStr("invalid specifier '", arg.presentation, "'");
arg.basePrefix = true; arg.basePrefix = true;
arg.presentation = 'x'; arg.presentation = 'x';
if (arg.align == FormatArg::Align::DEFAULT) { if (arg.align == FormatArg::Align::DEFAULT) {
...@@ -796,7 +787,7 @@ class TryFormatValue { ...@@ -796,7 +787,7 @@ class TryFormatValue {
public: public:
template <class FormatCallback> template <class FormatCallback>
static void formatOrFail(T& value, FormatArg& arg, FormatCallback& cb) { static void formatOrFail(T& value, FormatArg& arg, FormatCallback& cb) {
arg.error("No formatter available for this type"); LOG(FATAL) << arg.errorStr("No formatter available for this type");
} }
}; };
...@@ -1037,7 +1028,7 @@ class FormatValue<std::pair<A, B>> { ...@@ -1037,7 +1028,7 @@ class FormatValue<std::pair<A, B>> {
FormatValue<typename std::decay<B>::type>(val_.second).format(arg, cb); FormatValue<typename std::decay<B>::type>(val_.second).format(arg, cb);
break; break;
default: default:
arg.error("invalid index for pair"); LOG(FATAL) << arg.errorStr("invalid index for pair");
} }
} }
...@@ -1055,7 +1046,7 @@ class FormatValue<std::tuple<Args...>> { ...@@ -1055,7 +1046,7 @@ class FormatValue<std::tuple<Args...>> {
template <class FormatCallback> template <class FormatCallback>
void format(FormatArg& arg, FormatCallback& cb) const { void format(FormatArg& arg, FormatCallback& cb) const {
int key = arg.splitIntKey(); int key = arg.splitIntKey();
arg.enforce(key >= 0, "tuple index must be non-negative"); CHECK(key >= 0) << arg.errorStr("tuple index must be non-negative");
doFormat(key, arg, cb); doFormat(key, arg, cb);
} }
...@@ -1065,7 +1056,7 @@ class FormatValue<std::tuple<Args...>> { ...@@ -1065,7 +1056,7 @@ class FormatValue<std::tuple<Args...>> {
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 {
arg.enforce("tuple index out of range, max=", i); LOG(FATAL) << arg.errorStr("tuple index out of range, max=", i);
} }
template <size_t K, class Callback> template <size_t K, class Callback>
......
...@@ -70,7 +70,7 @@ void FormatArg::initSlow() { ...@@ -70,7 +70,7 @@ void FormatArg::initSlow() {
} }
if (*p == '0') { if (*p == '0') {
enforce(align == Align::DEFAULT, "alignment specified twice"); CHECK(align == Align::DEFAULT) << errorStr("alignment specified twice");
fill = '0'; fill = '0';
align = Align::PAD_AFTER_SIGN; align = Align::PAD_AFTER_SIGN;
if (++p == end) return; if (++p == end) return;
...@@ -105,31 +105,31 @@ void FormatArg::initSlow() { ...@@ -105,31 +105,31 @@ void FormatArg::initSlow() {
if (++p == end) return; if (++p == end) return;
} }
error("extra characters in format string"); LOG(FATAL) << "extra characters in format string";
} }
void FormatArg::validate(Type type) const { void FormatArg::validate(Type type) const {
enforce(keyEmpty(), "index not allowed"); CHECK(keyEmpty()) << "index not allowed";
switch (type) { switch (type) {
case Type::INTEGER: case Type::INTEGER:
enforce(precision == kDefaultPrecision, CHECK(precision == kDefaultPrecision)
"precision not allowed on integers"); << errorStr("precision not allowed on integers");
break; break;
case Type::FLOAT: case Type::FLOAT:
enforce(!basePrefix, CHECK(!basePrefix)
"base prefix ('#') specifier only allowed on integers"); << errorStr("base prefix ('#') specifier only allowed on integers");
enforce(!thousandsSeparator, CHECK(!thousandsSeparator)
"thousands separator (',') only allowed on integers"); << errorStr("thousands separator (',') only allowed on integers");
break; break;
case Type::OTHER: case Type::OTHER:
enforce(align != Align::PAD_AFTER_SIGN, CHECK(align != Align::PAD_AFTER_SIGN)
"'='alignment only allowed on numbers"); << errorStr("'='alignment only allowed on numbers");
enforce(sign == Sign::DEFAULT, CHECK(sign == Sign::DEFAULT)
"sign specifier only allowed on numbers"); << errorStr("sign specifier only allowed on numbers");
enforce(!basePrefix, CHECK(!basePrefix)
"base prefix ('#') specifier only allowed on integers"); << errorStr("base prefix ('#') specifier only allowed on integers");
enforce(!thousandsSeparator, CHECK(!thousandsSeparator)
"thousands separator (',') only allowed on integers"); << errorStr("thousands separator (',') only allowed on integers");
break; break;
} }
} }
......
...@@ -71,8 +71,11 @@ struct FormatArg { ...@@ -71,8 +71,11 @@ struct FormatArg {
} }
} }
template <typename... Args>
std::string errorStr(Args&&... args) const;
template <typename... Args> template <typename... Args>
void error(Args&&... args) const FOLLY_NORETURN; void error(Args&&... args) const FOLLY_NORETURN;
/** /**
* Full argument string, as passed in to the constructor. * Full argument string, as passed in to the constructor.
*/ */
...@@ -185,16 +188,21 @@ struct FormatArg { ...@@ -185,16 +188,21 @@ struct FormatArg {
NextKeyMode nextKeyMode_; NextKeyMode nextKeyMode_;
}; };
template <typename... Args>
inline std::string FormatArg::errorStr(Args&&... args) const {
return to<std::string>(
"invalid format argument {", fullArgString, "}: ",
std::forward<Args>(args)...);
}
template <typename... Args> template <typename... Args>
inline void FormatArg::error(Args&&... args) const { inline void FormatArg::error(Args&&... args) const {
throw std::invalid_argument(to<std::string>( throw std::invalid_argument(errorStr(std::forward<Args>(args)...));
"folly::format: invalid format argument {", fullArgString, "}: ",
std::forward<Args>(args)...));
} }
template <bool emptyOk> template <bool emptyOk>
inline StringPiece FormatArg::splitKey() { inline StringPiece FormatArg::splitKey() {
enforce(nextKeyMode_ != NextKeyMode::INT, "integer key expected"); CHECK(nextKeyMode_ != NextKeyMode::INT) << errorStr("integer key expected");
return doSplitKey<emptyOk>(); return doSplitKey<emptyOk>();
} }
...@@ -202,16 +210,12 @@ template <bool emptyOk> ...@@ -202,16 +210,12 @@ template <bool emptyOk>
inline StringPiece FormatArg::doSplitKey() { inline StringPiece FormatArg::doSplitKey() {
if (nextKeyMode_ == NextKeyMode::STRING) { if (nextKeyMode_ == NextKeyMode::STRING) {
nextKeyMode_ = NextKeyMode::NONE; nextKeyMode_ = NextKeyMode::NONE;
if (!emptyOk) { // static CHECK(emptyOk || !nextKey_.empty()) << errorStr("non-empty key required");
enforce(!nextKey_.empty(), "non-empty key required");
}
return nextKey_; return nextKey_;
} }
if (key_.empty()) { if (key_.empty()) {
if (!emptyOk) { // static CHECK(emptyOk) << errorStr("non-empty key required");
error("non-empty key required");
}
return StringPiece(); return StringPiece();
} }
...@@ -221,7 +225,7 @@ inline StringPiece FormatArg::doSplitKey() { ...@@ -221,7 +225,7 @@ inline StringPiece FormatArg::doSplitKey() {
if (e[-1] == ']') { if (e[-1] == ']') {
--e; --e;
p = static_cast<const char*>(memchr(b, '[', e - b)); p = static_cast<const char*>(memchr(b, '[', e - b));
enforce(p, "unmatched ']'"); CHECK(p) << errorStr("unmatched ']'");
} else { } else {
p = static_cast<const char*>(memchr(b, '.', e - b)); p = static_cast<const char*>(memchr(b, '.', e - b));
} }
...@@ -231,9 +235,8 @@ inline StringPiece FormatArg::doSplitKey() { ...@@ -231,9 +235,8 @@ inline StringPiece FormatArg::doSplitKey() {
p = e; p = e;
key_.clear(); key_.clear();
} }
if (!emptyOk) { // static CHECK(emptyOk || b != p) << errorStr("non-empty key required");
enforce(b != p, "non-empty key required");
}
return StringPiece(b, p); return StringPiece(b, p);
} }
...@@ -245,7 +248,7 @@ inline int FormatArg::splitIntKey() { ...@@ -245,7 +248,7 @@ inline int FormatArg::splitIntKey() {
try { try {
return to<int>(doSplitKey<true>()); return to<int>(doSplitKey<true>());
} catch (const std::out_of_range& e) { } catch (const std::out_of_range& e) {
error("integer key required"); LOG(FATAL) << errorStr("integer key required");
return 0; // unreached return 0; // unreached
} }
} }
......
...@@ -275,20 +275,6 @@ TEST(Format, Custom) { ...@@ -275,20 +275,6 @@ TEST(Format, Custom) {
EXPECT_NE("", fstr("{}", &kv)); EXPECT_NE("", fstr("{}", &kv));
} }
namespace {
struct Opaque {
int k;
};
} // namespace
TEST(Format, Unformatted) {
Opaque o;
EXPECT_NE("", fstr("{}", &o));
EXPECT_THROW(fstr("{0[0]}", &o), std::invalid_argument);
}
TEST(Format, Nested) { TEST(Format, Nested) {
EXPECT_EQ("1 2 3 4", fstr("{} {} {}", 1, 2, format("{} {}", 3, 4))); EXPECT_EQ("1 2 3 4", fstr("{} {} {}", 1, 2, format("{} {}", 3, 4)));
// //
...@@ -297,6 +283,16 @@ TEST(Format, Nested) { ...@@ -297,6 +283,16 @@ TEST(Format, Nested) {
EXPECT_EQ("1 2 3 4", fstr("{} {} {}", 1, 2, saved)); EXPECT_EQ("1 2 3 4", fstr("{} {} {}", 1, 2, saved));
} }
TEST(Format, OutOfBounds) {
std::vector<int> ints{1, 2, 3, 4, 5};
EXPECT_EQ("1 3 5", fstr("{0[0]} {0[2]} {0[4]}", ints));
EXPECT_THROW(fstr("{[5]}", ints), std::out_of_range);
std::map<std::string, int> map{{"hello", 0}, {"world", 1}};
EXPECT_EQ("hello = 0", fstr("hello = {[hello]}", map));
EXPECT_THROW(fstr("{[nope]}", map), std::out_of_range);
}
int main(int argc, char *argv[]) { int main(int argc, char *argv[]) {
testing::InitGoogleTest(&argc, argv); testing::InitGoogleTest(&argc, argv);
google::ParseCommandLineFlags(&argc, &argv, true); google::ParseCommandLineFlags(&argc, &argv, true);
......
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