From 9bfa4150776b1373827cd1104de852ec91350376 Mon Sep 17 00:00:00 2001 From: bsimmers Date: Tue, 27 Aug 2013 16:27:09 -0700 Subject: [PATCH] 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 --- folly/Format-inl.h | 97 ++++++++++++++++++--------------------- folly/Format.cpp | 34 +++++++------- folly/FormatArg.h | 33 +++++++------ folly/test/FormatTest.cpp | 24 ++++------ 4 files changed, 89 insertions(+), 99 deletions(-) diff --git a/folly/Format-inl.h b/folly/Format-inl.h index 70672d47..620557a6 100644 --- a/folly/Format-inl.h +++ b/folly/Format-inl.h @@ -168,10 +168,7 @@ void Formatter::operator()(Output& out) const { out(StringPiece(p, q)); p = q; - if (p == end || *p != '}') { - throw std::invalid_argument( - "folly::format: single '}' in format string"); - } + CHECK(p != end && *p == '}') << "single '}' in format string"; ++p; } }; @@ -188,10 +185,7 @@ void Formatter::operator()(Output& out) const { outputString(StringPiece(p, q)); p = q + 1; - if (p == end) { - throw std::invalid_argument( - "folly::format: '}' at end of format string"); - } + CHECK(p != end) << "'{' at end of format string"; // "{{" -> "{" if (*p == '{') { @@ -202,9 +196,7 @@ void Formatter::operator()(Output& out) const { // Format string q = static_cast(memchr(p, '}', end - p)); - if (q == end) { - throw std::invalid_argument("folly::format: missing ending '}'"); - } + CHECK(q != end) << "missing ending '}'"; FormatArg arg(StringPiece(p, q)); p = q + 1; @@ -226,17 +218,16 @@ void Formatter::operator()(Output& out) const { try { argIndex = to(piece); } 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; } } - if (hasDefaultArgIndex && hasExplicitArgIndex) { - throw std::invalid_argument( - "folly::format: may not have both default and explicit arg indexes"); - } + CHECK(!hasDefaultArgIndex || !hasExplicitArgIndex) + << "may not have both default and explicit arg indexes"; doFormat(argIndex, arg, out); } @@ -400,8 +391,8 @@ class FormatValue< uval = val_; sign = '\0'; - arg.enforce(arg.sign == FormatArg::Sign::DEFAULT, - "sign specifications not allowed for unsigned values"); + CHECK(arg.sign == FormatArg::Sign::DEFAULT) + << arg.errorStr("sign specifications not allowed for unsigned values"); } // max of: @@ -428,9 +419,9 @@ class FormatValue< switch (presentation) { case 'n': // TODO(tudorb): locale awareness? case 'd': - arg.enforce(!arg.basePrefix, - "base prefix not allowed with '", presentation, - "' specifier"); + CHECK(!arg.basePrefix) + << arg.errorStr("base prefix not allowed with '", presentation, + "' specifier"); if (arg.thousandsSeparator) { useSprintf("%'ju"); } else { @@ -440,21 +431,21 @@ class FormatValue< } break; case 'c': - arg.enforce(!arg.basePrefix, - "base prefix not allowed with '", presentation, - "' specifier"); - arg.enforce(!arg.thousandsSeparator, - "thousands separator (',') not allowed with '", - presentation, "' specifier"); + CHECK(!arg.basePrefix) + << arg.errorStr("base prefix not allowed with '", presentation, + "' specifier"); + CHECK(!arg.thousandsSeparator) + << arg.errorStr("thousands separator (',') not allowed with '", + presentation, "' specifier"); valBufBegin = valBuf + 3; *valBufBegin = static_cast(uval); valBufEnd = valBufBegin + 1; break; case 'o': case 'O': - arg.enforce(!arg.thousandsSeparator, - "thousands separator (',') not allowed with '", - presentation, "' specifier"); + CHECK(!arg.thousandsSeparator) + << arg.errorStr("thousands separator (',') not allowed with '", + presentation, "' specifier"); valBufEnd = valBuf + valBufSize - 1; valBufBegin = valBuf + detail::uintToOctal(valBuf, valBufSize - 1, uval); if (arg.basePrefix) { @@ -463,9 +454,9 @@ class FormatValue< } break; case 'x': - arg.enforce(!arg.thousandsSeparator, - "thousands separator (',') not allowed with '", - presentation, "' specifier"); + CHECK(!arg.thousandsSeparator) + << arg.errorStr("thousands separator (',') not allowed with '", + presentation, "' specifier"); valBufEnd = valBuf + valBufSize - 1; valBufBegin = valBuf + detail::uintToHexLower(valBuf, valBufSize - 1, uval); @@ -476,9 +467,9 @@ class FormatValue< } break; case 'X': - arg.enforce(!arg.thousandsSeparator, - "thousands separator (',') not allowed with '", - presentation, "' specifier"); + CHECK(!arg.thousandsSeparator) + << arg.errorStr("thousands separator (',') not allowed with '", + presentation, "' specifier"); valBufEnd = valBuf + valBufSize - 1; valBufBegin = valBuf + detail::uintToHexUpper(valBuf, valBufSize - 1, uval); @@ -490,9 +481,9 @@ class FormatValue< break; case 'b': case 'B': - arg.enforce(!arg.thousandsSeparator, - "thousands separator (',') not allowed with '", - presentation, "' specifier"); + CHECK(!arg.thousandsSeparator) + << arg.errorStr("thousands separator (',') not allowed with '", + presentation, "' specifier"); valBufEnd = valBuf + valBufSize - 1; valBufBegin = valBuf + detail::uintToBinary(valBuf, valBufSize - 1, uval); @@ -503,7 +494,7 @@ class FormatValue< } break; default: - arg.error("invalid specifier '", presentation, "'"); + LOG(FATAL) << arg.errorStr("invalid specifier '", presentation, "'"); } if (sign) { @@ -645,7 +636,7 @@ class FormatValue { } break; default: - arg.error("invalid specifier '", arg.presentation, "'"); + LOG(FATAL) << arg.errorStr("invalid specifier '", arg.presentation, "'"); } int len = builder.position(); @@ -702,9 +693,9 @@ class FormatValue< void format(FormatArg& arg, FormatCallback& cb) const { if (arg.keyEmpty()) { arg.validate(FormatArg::Type::OTHER); - arg.enforce(arg.presentation == FormatArg::kDefaultPresentation || - arg.presentation == 's', - "invalid specifier '", arg.presentation, "'"); + CHECK(arg.presentation == FormatArg::kDefaultPresentation || + arg.presentation == 's') + << arg.errorStr("invalid specifier '", arg.presentation, "'"); format_value::formatString(val_, arg, cb); } else { FormatValue(val_.at(arg.splitIntKey())).format(arg, cb); @@ -724,8 +715,8 @@ class FormatValue { template void format(FormatArg& arg, FormatCallback& cb) const { arg.validate(FormatArg::Type::OTHER); - arg.enforce(arg.presentation == FormatArg::kDefaultPresentation, - "invalid specifier '", arg.presentation, "'"); + CHECK(arg.presentation == FormatArg::kDefaultPresentation) + << arg.errorStr("invalid specifier '", arg.presentation, "'"); format_value::formatString("(null)", arg, cb); } }; @@ -775,8 +766,8 @@ class FormatValue< } else { // Print as a pointer, in hex. arg.validate(FormatArg::Type::OTHER); - arg.enforce(arg.presentation == FormatArg::kDefaultPresentation, - "invalid specifier '", arg.presentation, "'"); + CHECK(arg.presentation == FormatArg::kDefaultPresentation) + << arg.errorStr("invalid specifier '", arg.presentation, "'"); arg.basePrefix = true; arg.presentation = 'x'; if (arg.align == FormatArg::Align::DEFAULT) { @@ -796,7 +787,7 @@ class TryFormatValue { public: template 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> { FormatValue::type>(val_.second).format(arg, cb); break; default: - arg.error("invalid index for pair"); + LOG(FATAL) << arg.errorStr("invalid index for pair"); } } @@ -1055,7 +1046,7 @@ class FormatValue> { template void format(FormatArg& arg, FormatCallback& cb) const { 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); } @@ -1065,7 +1056,7 @@ class FormatValue> { template typename std::enable_if::type 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 diff --git a/folly/Format.cpp b/folly/Format.cpp index 385a9bb8..169eca08 100644 --- a/folly/Format.cpp +++ b/folly/Format.cpp @@ -70,7 +70,7 @@ void FormatArg::initSlow() { } if (*p == '0') { - enforce(align == Align::DEFAULT, "alignment specified twice"); + CHECK(align == Align::DEFAULT) << errorStr("alignment specified twice"); fill = '0'; align = Align::PAD_AFTER_SIGN; if (++p == end) return; @@ -105,31 +105,31 @@ void FormatArg::initSlow() { if (++p == end) return; } - error("extra characters in format string"); + LOG(FATAL) << "extra characters in format string"; } void FormatArg::validate(Type type) const { - enforce(keyEmpty(), "index not allowed"); + CHECK(keyEmpty()) << "index not allowed"; switch (type) { case Type::INTEGER: - enforce(precision == kDefaultPrecision, - "precision not allowed on integers"); + CHECK(precision == kDefaultPrecision) + << errorStr("precision not allowed on integers"); break; case Type::FLOAT: - enforce(!basePrefix, - "base prefix ('#') specifier only allowed on integers"); - enforce(!thousandsSeparator, - "thousands separator (',') only allowed on integers"); + CHECK(!basePrefix) + << errorStr("base prefix ('#') specifier only allowed on integers"); + CHECK(!thousandsSeparator) + << errorStr("thousands separator (',') only allowed on integers"); break; case Type::OTHER: - enforce(align != Align::PAD_AFTER_SIGN, - "'='alignment only allowed on numbers"); - enforce(sign == Sign::DEFAULT, - "sign specifier only allowed on numbers"); - enforce(!basePrefix, - "base prefix ('#') specifier only allowed on integers"); - enforce(!thousandsSeparator, - "thousands separator (',') only allowed on integers"); + CHECK(align != Align::PAD_AFTER_SIGN) + << errorStr("'='alignment only allowed on numbers"); + CHECK(sign == Sign::DEFAULT) + << errorStr("sign specifier only allowed on numbers"); + CHECK(!basePrefix) + << errorStr("base prefix ('#') specifier only allowed on integers"); + CHECK(!thousandsSeparator) + << errorStr("thousands separator (',') only allowed on integers"); break; } } diff --git a/folly/FormatArg.h b/folly/FormatArg.h index 124d621d..d48aad7a 100644 --- a/folly/FormatArg.h +++ b/folly/FormatArg.h @@ -71,8 +71,11 @@ struct FormatArg { } } + template + std::string errorStr(Args&&... args) const; template void error(Args&&... args) const FOLLY_NORETURN; + /** * Full argument string, as passed in to the constructor. */ @@ -185,16 +188,21 @@ struct FormatArg { NextKeyMode nextKeyMode_; }; +template +inline std::string FormatArg::errorStr(Args&&... args) const { + return to( + "invalid format argument {", fullArgString, "}: ", + std::forward(args)...); +} + template inline void FormatArg::error(Args&&... args) const { - throw std::invalid_argument(to( - "folly::format: invalid format argument {", fullArgString, "}: ", - std::forward(args)...)); + throw std::invalid_argument(errorStr(std::forward(args)...)); } template inline StringPiece FormatArg::splitKey() { - enforce(nextKeyMode_ != NextKeyMode::INT, "integer key expected"); + CHECK(nextKeyMode_ != NextKeyMode::INT) << errorStr("integer key expected"); return doSplitKey(); } @@ -202,16 +210,12 @@ template inline StringPiece FormatArg::doSplitKey() { if (nextKeyMode_ == NextKeyMode::STRING) { nextKeyMode_ = NextKeyMode::NONE; - if (!emptyOk) { // static - enforce(!nextKey_.empty(), "non-empty key required"); - } + CHECK(emptyOk || !nextKey_.empty()) << errorStr("non-empty key required"); return nextKey_; } if (key_.empty()) { - if (!emptyOk) { // static - error("non-empty key required"); - } + CHECK(emptyOk) << errorStr("non-empty key required"); return StringPiece(); } @@ -221,7 +225,7 @@ inline StringPiece FormatArg::doSplitKey() { if (e[-1] == ']') { --e; p = static_cast(memchr(b, '[', e - b)); - enforce(p, "unmatched ']'"); + CHECK(p) << errorStr("unmatched ']'"); } else { p = static_cast(memchr(b, '.', e - b)); } @@ -231,9 +235,8 @@ inline StringPiece FormatArg::doSplitKey() { p = e; key_.clear(); } - if (!emptyOk) { // static - enforce(b != p, "non-empty key required"); - } + CHECK(emptyOk || b != p) << errorStr("non-empty key required"); + return StringPiece(b, p); } @@ -245,7 +248,7 @@ inline int FormatArg::splitIntKey() { try { return to(doSplitKey()); } catch (const std::out_of_range& e) { - error("integer key required"); + LOG(FATAL) << errorStr("integer key required"); return 0; // unreached } } diff --git a/folly/test/FormatTest.cpp b/folly/test/FormatTest.cpp index 96de6bf6..0c41d7ae 100644 --- a/folly/test/FormatTest.cpp +++ b/folly/test/FormatTest.cpp @@ -275,20 +275,6 @@ TEST(Format, Custom) { 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) { EXPECT_EQ("1 2 3 4", fstr("{} {} {}", 1, 2, format("{} {}", 3, 4))); // @@ -297,6 +283,16 @@ TEST(Format, Nested) { EXPECT_EQ("1 2 3 4", fstr("{} {} {}", 1, 2, saved)); } +TEST(Format, OutOfBounds) { + std::vector 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 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[]) { testing::InitGoogleTest(&argc, argv); google::ParseCommandLineFlags(&argc, &argv, true); -- 2.34.1