diff --git a/.clang-tidy b/.clang-tidy index c13ca78ac2..88dd6f4e57 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -50,13 +50,7 @@ Checks: "-*, performance-*, -performance-avoid-endl, -performance-enum-size, - -performance-inefficient-algorithm, - -performance-inefficient-string-concatenation, - -performance-no-int-to-ptr, - -performance-noexcept-destructor, -performance-noexcept-move-constructor, - -performance-noexcept-swap, - -performance-type-promotion-in-math-fn, -performance-unnecessary-copy-initialization, -performance-unnecessary-value-param, diff --git a/include/xrpl/basics/IntrusivePointer.ipp b/include/xrpl/basics/IntrusivePointer.ipp index 8344a3e613..67d43b05d6 100644 --- a/include/xrpl/basics/IntrusivePointer.ipp +++ b/include/xrpl/basics/IntrusivePointer.ipp @@ -641,6 +641,9 @@ template T* SharedWeakUnion::unsafeGetRawPtr() const { + // tp_ packs a raw pointer together with a strength bit; recovering the + // pointer inherently requires an integer-to-pointer cast. + // NOLINTNEXTLINE(performance-no-int-to-ptr) return reinterpret_cast(tp_ & kPtrMask); } diff --git a/include/xrpl/shamap/detail/TaggedPointer.ipp b/include/xrpl/shamap/detail/TaggedPointer.ipp index 6606c49a6b..9275f3d15a 100644 --- a/include/xrpl/shamap/detail/TaggedPointer.ipp +++ b/include/xrpl/shamap/detail/TaggedPointer.ipp @@ -507,6 +507,9 @@ TaggedPointer::operator=(TaggedPointer&& other) [[nodiscard]] inline std::pair TaggedPointer::decode() const { + // tp_ packs a raw pointer together with the tag bits; recovering the + // pointer inherently requires an integer-to-pointer cast. + // NOLINTNEXTLINE(performance-no-int-to-ptr) return {tp_ & kTagMask, reinterpret_cast(tp_ & kPtrMask)}; } @@ -535,6 +538,9 @@ TaggedPointer::getHashesAndChildren() const [[nodiscard]] inline SHAMapHash* TaggedPointer::getHashes() const { + // tp_ packs a raw pointer together with the tag bits; recovering the + // pointer inherently requires an integer-to-pointer cast. + // NOLINTNEXTLINE(performance-no-int-to-ptr) return reinterpret_cast(tp_ & kPtrMask); }; diff --git a/src/libxrpl/json/json_reader.cpp b/src/libxrpl/json/json_reader.cpp index f37cca506b..f9134e6629 100644 --- a/src/libxrpl/json/json_reader.cpp +++ b/src/libxrpl/json/json_reader.cpp @@ -5,12 +5,13 @@ #include #include +#include #include #include -#include #include #include #include +#include namespace json { // Implementation of class Reader @@ -605,36 +606,17 @@ bool Reader::decodeDouble(Token& token) { double value = 0; - int const bufferSize = 32; - int count = 0; - int const length = int(token.end - token.start); - // Sanity check to avoid buffer overflow exploits. - if (length < 0) - { - return addError("Unable to parse token length", token); - } - // Avoid using a string constant for the format control string given to - // sscanf, as this can cause hard to debug crashes on OS X. See here for - // more info: - // - // http://developer.apple.com/library/mac/#DOCUMENTATION/DeveloperTools/gcc-4.0.1/gcc/Incompatibilities.html - char format[] = "%lf"; - if (length <= bufferSize) - { - Char buffer[bufferSize + 1]; - memcpy(buffer, token.start, length); - buffer[length] = 0; - // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg) - count = sscanf(buffer, format, &value); - } - else - { - std::string const buffer(token.start, token.end); - // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg) - count = sscanf(buffer.c_str(), format, &value); - } - if (count != 1) + auto const [ptr, ec] = std::from_chars(token.start, token.end, value); + + // Reject anything from_chars could not turn into a finite double: + // - ec != std::errc{}: no valid conversion, or an out-of-range magnitude + // (e.g. 1e400). + // - ptr != token.end: readNumber() is permissive about which characters + // it collects into a token (it will, for example, keep a '+' mid-token), + // but from_chars() will stop at the first character it cannot parse. + if (ec != std::errc{} || ptr != token.end) return addError("'" + std::string(token.start, token.end) + "' is not a number.", token); + currentValue() = value; return true; } diff --git a/src/libxrpl/json/json_writer.cpp b/src/libxrpl/json/json_writer.cpp index 853dd9a5ac..fdfa5d3cf0 100644 --- a/src/libxrpl/json/json_writer.cpp +++ b/src/libxrpl/json/json_writer.cpp @@ -4,13 +4,14 @@ #include #include -#include +#include #include #include #include #include #include #include +#include #include namespace json { @@ -76,20 +77,18 @@ valueToString(UInt value) std::string valueToString(double value) { - // Allocate a buffer that is more than large enough to store the 16 digits - // of precision requested below. + // Format with 16 significant digits. + // We need not request the alternative representation that always has a + // decimal point because JSON doesn't distinguish the concepts of reals and integers. + // A double never needs more than 32 characters in this form, + // so to_chars cannot actually run out of room here. char buffer[32]; - // Print into the buffer. We need not request the alternative representation - // that always has a decimal point because JSON doesn't distinguish the - // concepts of reals and integers. -#if defined(_MSC_VER) && defined(__STDC_SECURE_LIB__) // Use secure version with visual studio 2005 - // to avoid warning. - sprintf_s(buffer, sizeof(buffer), "%.16g", value); -#else - // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg) - snprintf(buffer, sizeof(buffer), "%.16g", value); -#endif - return buffer; + auto const [ptr, ec] = + std::to_chars(buffer, buffer + sizeof(buffer), value, std::chars_format::general, 16); + XRPL_ASSERT(ec == std::errc{}, "json::valueToString(double) : conversion fits buffer"); + if (ec != std::errc{}) + return {}; + return std::string(buffer, ptr); } std::string diff --git a/src/libxrpl/protocol/STParsedJSON.cpp b/src/libxrpl/protocol/STParsedJSON.cpp index d3342ef728..33ca5424d1 100644 --- a/src/libxrpl/protocol/STParsedJSON.cpp +++ b/src/libxrpl/protocol/STParsedJSON.cpp @@ -69,6 +69,17 @@ toUnsigned(U2 value) return static_cast(value); } +static std::string +joinName(std::string const& jsonName, std::string const& fieldName) +{ + std::string result; + result.reserve(jsonName.size() + 1 + fieldName.size()); + result += jsonName; + result += '.'; + result += fieldName; + return result; +} + // LCOV_EXCL_START static inline std::string makeName(std::string const& object, std::string const& field) @@ -76,7 +87,7 @@ makeName(std::string const& object, std::string const& field) if (field.empty()) return object; - return object + "." + field; + return joinName(object, field); } static inline json::Value @@ -1036,8 +1047,8 @@ parseObject( try { - auto ret = - parseObject(jsonName + "." + fieldName, value, field, depth + 1, error); + auto ret = parseObject( + joinName(jsonName, fieldName), value, field, depth + 1, error); if (!ret) return std::nullopt; data.emplaceBack(std::move(*ret)); @@ -1054,8 +1065,8 @@ parseObject( case STI_ARRAY: try { - auto array = - parseArray(jsonName + "." + fieldName, value, field, depth + 1, error); + auto array = parseArray( + joinName(jsonName, fieldName), value, field, depth + 1, error); if (!array.has_value()) return std::nullopt; data.emplaceBack(std::move(*array)); diff --git a/src/libxrpl/rdb/SociDB.cpp b/src/libxrpl/rdb/SociDB.cpp index 06e58d373f..08c826b210 100644 --- a/src/libxrpl/rdb/SociDB.cpp +++ b/src/libxrpl/rdb/SociDB.cpp @@ -213,6 +213,12 @@ public: if (auto [conn, keepAlive] = getConnection(); conn) { (void)keepAlive; + // The checkpointer is identified to the C callback by an integer id + // (resolved via checkpointerFromId) rather than a raw `this`, so it + // cannot dangle if the checkpointer is destroyed. Passing the id + // through sqlite's void* user-data requires an integer-to-pointer + // cast. + // NOLINTNEXTLINE(performance-no-int-to-ptr) sqlite_api::sqlite3_wal_hook(conn, &sqliteWALHook, reinterpret_cast(id_)); } } diff --git a/src/test/core/Config_test.cpp b/src/test/core/Config_test.cpp index 38c14b8ac5..17c48b1ccc 100644 --- a/src/test/core/Config_test.cpp +++ b/src/test/core/Config_test.cpp @@ -1475,6 +1475,7 @@ r.ripple.com:51235 std::string toLoad(R"xrpldConfig( [amendment_majority_time] )xrpldConfig"); + // NOLINTNEXTLINE(performance-inefficient-string-concatenation) toLoad += std::to_string(val) + space + unit; space = space.empty() ? " " : ""; diff --git a/src/tests/libxrpl/json/Value.cpp b/src/tests/libxrpl/json/Value.cpp index a1cd8160d7..a58a5df9fd 100644 --- a/src/tests/libxrpl/json/Value.cpp +++ b/src/tests/libxrpl/json/Value.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -592,6 +593,57 @@ TEST(json_value, bad_json) EXPECT_TRUE(r.parse(s, j)); } +namespace { + +std::optional +parseValue(std::string const& doc) +{ + json::Value j; + json::Reader r; + if (!r.parse("{\"v\":" + doc + "}", j)) + return std::nullopt; + return j["v"]; +} + +} // namespace + +TEST(json_value, parse_double_valid) +{ + // 1e300 is large but still representable, so it parses (unlike the out-of-range cases below). + for (auto const& [text, expected] : + {std::pair{"2.5", 2.5}, + std::pair{"-3.25e2", -325.0}, + std::pair{"0.0", 0.0}, + std::pair{"1E3", 1000.0}, + std::pair{"-0.5e-1", -0.05}, + std::pair{"1e300", 1e300}}) + { + auto const v = parseValue(text); + ASSERT_TRUE(v.has_value()) << text; + // NOLINTBEGIN(bugprone-unchecked-optional-access) + EXPECT_TRUE(v->isDouble()) << text; + EXPECT_EQ(v->asDouble(), expected) << text; + // NOLINTEND(bugprone-unchecked-optional-access) + } +} + +TEST(json_value, parse_double_out_of_range) +{ + // Magnitudes with no finite double representation are rejected. + for (char const* oor : {"1e400", "-1e400", "0.001e500", "1e-400", "-1e-400", "123e-500"}) + EXPECT_FALSE(parseValue(oor).has_value()) << oor; +} + +TEST(json_value, parse_double_malformed) +{ + // readNumber() collects any run of digits and '.eE+-' into a single Double + // token, so these malformed tokens reach decodeDouble. Each has a valid + // leading prefix that from_chars would accept on its own; requiring the + // entire token be consumed rejects them instead of silently truncating. + for (char const* bad : {"1+2", "1-2", "1.2.3", "1e5e6", "1..2", "++5", "1e", "1e+", ".", "-"}) + EXPECT_FALSE(parseValue(bad).has_value()) << bad; +} + TEST(json_value, edge_cases) { std::uint32_t const maxUInt = std::numeric_limits::max();