From 12c19fb383862d9393f065ae9d834eab77f84dc5 Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Tue, 14 Jan 2025 03:40:23 -0500 Subject: [PATCH] Address various warnings new in clang-tidy 19 --- src/.clang-tidy | 11 +++- src/cli/tls_client.cpp | 2 +- .../noekeon/noekeon_simd/noekeon_simd.cpp | 4 ++ .../serpent/serpent_avx512/serpent_avx512.cpp | 4 ++ src/lib/block/threefish_512/threefish_512.cpp | 4 ++ src/lib/ffi/ffi.cpp | 4 ++ .../classic_mceliece/cmce_field_ordering.cpp | 4 ++ src/lib/pubkey/mce/mceliece_key.cpp | 8 +-- src/lib/tls/tls13/tls_handshake_layer_13.cpp | 1 + src/lib/tls/tls13_pqc/hybrid_public_key.cpp | 1 + src/lib/tls/tls_ciphersuite.cpp | 2 +- src/lib/x509/x509_dn.cpp | 2 +- src/scripts/dev_tools/run_clang_tidy.py | 5 ++ src/tests/test_kyber.cpp | 2 +- src/tests/test_rng_behavior.cpp | 8 ++- src/tests/test_tls_session_manager.cpp | 66 ++++++++++--------- src/tests/tests.cpp | 4 +- 17 files changed, 86 insertions(+), 46 deletions(-) diff --git a/src/.clang-tidy b/src/.clang-tidy index dd3b15bc0ea..db5dcc431b0 100644 --- a/src/.clang-tidy +++ b/src/.clang-tidy @@ -1,5 +1,5 @@ --- -# This file was automatically generated by src/scripts/dev_tools/run_clang_tidy.py --regenerate-inline-config-file +# This file was automatically generated by ./src/scripts/dev_tools/run_clang_tidy.py --regenerate-inline-config-file # # All manual edits to this file will be lost. Edit the script # then regenerate this configuration file. @@ -19,6 +19,7 @@ Checks: > -*-member-init, -bugprone-lambda-function-name, -bugprone-unchecked-optional-access, + -bugprone-empty-catch, -cert-err58-cpp, -cppcoreguidelines-avoid-const-or-ref-data-members, -cppcoreguidelines-init-variables, @@ -27,11 +28,14 @@ Checks: > -cppcoreguidelines-slicing, -hicpp-explicit-conversions, -misc-const-correctness, + -misc-include-cleaner, -misc-redundant-expression, -misc-misplaced-const, -misc-confusable-identifiers, -modernize-avoid-bind, -modernize-pass-by-value, + -modernize-use-ranges, + -performance-avoid-endl, -readability-convert-member-functions-to-static, -readability-implicit-bool-conversion, -readability-inconsistent-declaration-parameter-name, @@ -52,6 +56,7 @@ Checks: > -bugprone-branch-clone, -bugprone-easily-swappable-parameters, -bugprone-implicit-widening-of-multiplication-result, + -bugprone-suspicious-stringview-data-usage, -cppcoreguidelines-avoid-do-while, -cppcoreguidelines-non-private-member-variables-in-classes, -cppcoreguidelines-pro-bounds-pointer-arithmetic, @@ -68,15 +73,19 @@ Checks: > -modernize-use-trailing-return-type, -modernize-return-braced-init-list, -modernize-use-default-member-init, + -modernize-use-designated-initializers, -modernize-use-nodiscard, -modernize-use-using, -portability-simd-intrinsics, + -readability-avoid-return-with-void-value, -readability-container-data-pointer, -readability-function-cognitive-complexity, -readability-identifier-length, -readability-isolate-declaration, + -readability-math-missing-parentheses, -readability-non-const-parameter, -readability-redundant-access-specifiers, -readability-suspicious-call-argument, + -readability-use-std-min-max, -readability-use-anyofallof, --- diff --git a/src/cli/tls_client.cpp b/src/cli/tls_client.cpp index 4bd669481d2..52a2964192a 100644 --- a/src/cli/tls_client.cpp +++ b/src/cli/tls_client.cpp @@ -71,7 +71,7 @@ class Callbacks : public Botan::TLS::Callbacks { if(result.successful_validation()) { output() << "Certificate validation status: " << result.result_string() << "\n"; - auto status = result.all_statuses(); + const auto& status = result.all_statuses(); if(!status.empty() && status[0].contains(Botan::Certificate_Status_Code::OCSP_RESPONSE_GOOD)) { output() << "Valid OCSP response for this server\n"; diff --git a/src/lib/block/noekeon/noekeon_simd/noekeon_simd.cpp b/src/lib/block/noekeon/noekeon_simd/noekeon_simd.cpp index 7727a7edf4b..2a85a836bf0 100644 --- a/src/lib/block/noekeon/noekeon_simd/noekeon_simd.cpp +++ b/src/lib/block/noekeon/noekeon_simd/noekeon_simd.cpp @@ -11,6 +11,8 @@ namespace Botan { +namespace { + /* * Noekeon's Theta Operation */ @@ -55,6 +57,8 @@ inline void gamma(SIMD_4x32& A0, SIMD_4x32& A1, SIMD_4x32& A2, SIMD_4x32& A3) { A0 ^= A2 & A1; } +} // namespace + /* * Noekeon Encryption */ diff --git a/src/lib/block/serpent/serpent_avx512/serpent_avx512.cpp b/src/lib/block/serpent/serpent_avx512/serpent_avx512.cpp index e02a0a56422..ee1a83a8852 100644 --- a/src/lib/block/serpent/serpent_avx512/serpent_avx512.cpp +++ b/src/lib/block/serpent/serpent_avx512/serpent_avx512.cpp @@ -10,6 +10,8 @@ namespace Botan { +namespace { + BOTAN_FORCE_INLINE void SBoxE0(SIMD_16x32& a, SIMD_16x32& b, SIMD_16x32& c, SIMD_16x32& d) { const SIMD_16x32 t0 = SIMD_16x32::ternary_fn<0xb9>(b, d, c); const SIMD_16x32 t1 = SIMD_16x32::ternary_fn<0xe2>(a, b, d); @@ -261,6 +263,8 @@ BOTAN_FORCE_INLINE void SBoxD7(SIMD_16x32& a, SIMD_16x32& b, SIMD_16x32& c, SIMD d = o3; } +} // namespace + BOTAN_AVX512_FN void Serpent::avx512_encrypt_16(const uint8_t in[16 * 16], uint8_t out[16 * 16]) const { using namespace Botan::Serpent_F; diff --git a/src/lib/block/threefish_512/threefish_512.cpp b/src/lib/block/threefish_512/threefish_512.cpp index 6bcc8396c10..9657f02bd39 100644 --- a/src/lib/block/threefish_512/threefish_512.cpp +++ b/src/lib/block/threefish_512/threefish_512.cpp @@ -15,6 +15,8 @@ namespace Botan { namespace Threefish_F { +namespace { + template BOTAN_FORCE_INLINE void e_round( uint64_t& X0, uint64_t& X1, uint64_t& X2, uint64_t& X3, uint64_t& X4, uint64_t& X5, uint64_t& X6, uint64_t& X7) { @@ -142,6 +144,8 @@ BOTAN_FORCE_INLINE void d8_rounds(uint64_t& X0, key.d_add(R2, X0, X1, X2, X3, X4, X5, X6, X7); } +} // namespace + } // namespace Threefish_F void Threefish_512::skein_feedfwd(const secure_vector& M, const secure_vector& T) { diff --git a/src/lib/ffi/ffi.cpp b/src/lib/ffi/ffi.cpp index b118149b3ac..8cede931f75 100644 --- a/src/lib/ffi/ffi.cpp +++ b/src/lib/ffi/ffi.cpp @@ -21,9 +21,13 @@ namespace Botan_FFI { +namespace { + // NOLINTNEXTLINE(*-avoid-non-const-global-variables) thread_local std::string g_last_exception_what; +} // namespace + int ffi_error_exception_thrown(const char* func_name, const char* exn, int rc) { g_last_exception_what.assign(exn); diff --git a/src/lib/pubkey/classic_mceliece/cmce_field_ordering.cpp b/src/lib/pubkey/classic_mceliece/cmce_field_ordering.cpp index c6007db4c8f..4f05056ec55 100644 --- a/src/lib/pubkey/classic_mceliece/cmce_field_ordering.cpp +++ b/src/lib/pubkey/classic_mceliece/cmce_field_ordering.cpp @@ -22,6 +22,8 @@ namespace Botan { namespace CMCE_CT { +namespace { + template requires(sizeof(T1) <= 8 && sizeof(T2) <= 8) void cond_swap_pair(CT::Mask cond_mask, std::pair& a, std::pair& b) { @@ -66,6 +68,8 @@ T min(const T& a, const T& b) { return mask.select(a, b); } +} // namespace + } // namespace CMCE_CT namespace { diff --git a/src/lib/pubkey/mce/mceliece_key.cpp b/src/lib/pubkey/mce/mceliece_key.cpp index 502618c6908..97771f060a8 100644 --- a/src/lib/pubkey/mce/mceliece_key.cpp +++ b/src/lib/pubkey/mce/mceliece_key.cpp @@ -86,8 +86,8 @@ std::vector McEliece_PublicKey::public_key_bits() const { DER_Encoder(output) .start_sequence() .start_sequence() - .encode(static_cast(get_code_length())) - .encode(static_cast(get_t())) + .encode(get_code_length()) + .encode(get_t()) .end_cons() .encode(m_public_matrix, ASN1_Type::OctetString) .end_cons(); @@ -121,8 +121,8 @@ secure_vector McEliece_PrivateKey::private_key_bits() const { DER_Encoder enc; enc.start_sequence() .start_sequence() - .encode(static_cast(get_code_length())) - .encode(static_cast(get_t())) + .encode(get_code_length()) + .encode(get_t()) .end_cons() .encode(m_public_matrix, ASN1_Type::OctetString) .encode(m_g[0].encode(), ASN1_Type::OctetString); // g as octet string diff --git a/src/lib/tls/tls13/tls_handshake_layer_13.cpp b/src/lib/tls/tls13/tls_handshake_layer_13.cpp index b17a15ffb18..77afca56885 100644 --- a/src/lib/tls/tls13/tls_handshake_layer_13.cpp +++ b/src/lib/tls/tls13/tls_handshake_layer_13.cpp @@ -150,6 +150,7 @@ const T& get(const std::reference_wrapper& v) { template const T& get(const T& v) { + // NOLINTNEXTLINE(bugprone-return-const-ref-from-parameter) return v; } diff --git a/src/lib/tls/tls13_pqc/hybrid_public_key.cpp b/src/lib/tls/tls13_pqc/hybrid_public_key.cpp index 3119f5e0ce7..d8f0160e104 100644 --- a/src/lib/tls/tls13_pqc/hybrid_public_key.cpp +++ b/src/lib/tls/tls13_pqc/hybrid_public_key.cpp @@ -149,6 +149,7 @@ std::unique_ptr Hybrid_KEM_PublicKey::load_for_group( BufferSlicer public_value_slicer(concatenated_public_values); std::vector> pks; + pks.reserve(alg_ids.size()); for(size_t idx = 0; idx < alg_ids.size(); ++idx) { pks.emplace_back(load_public_key(alg_ids[idx], public_value_slicer.take(public_value_lengths[idx]))); } diff --git a/src/lib/tls/tls_ciphersuite.cpp b/src/lib/tls/tls_ciphersuite.cpp index 76e5521110b..cfa8b9d98fe 100644 --- a/src/lib/tls/tls_ciphersuite.cpp +++ b/src/lib/tls/tls_ciphersuite.cpp @@ -148,7 +148,7 @@ bool Ciphersuite::is_usable() const { return false; } - const auto mode = cipher_and_mode[1]; + const auto& mode = cipher_and_mode[1]; #if !defined(BOTAN_HAS_AEAD_CCM) if(mode == "CCM" || mode == "CCM-8") diff --git a/src/lib/x509/x509_dn.cpp b/src/lib/x509/x509_dn.cpp index 1f7a9e21c47..1a69871782e 100644 --- a/src/lib/x509/x509_dn.cpp +++ b/src/lib/x509/x509_dn.cpp @@ -406,7 +406,7 @@ std::string X509_DN::to_string() const { } std::ostream& operator<<(std::ostream& out, const X509_DN& dn) { - auto info = dn.dn_info(); + const auto& info = dn.dn_info(); for(size_t i = 0; i != info.size(); ++i) { out << to_short_form(info[i].first) << "=\""; diff --git a/src/scripts/dev_tools/run_clang_tidy.py b/src/scripts/dev_tools/run_clang_tidy.py index 2b9bc9b83d6..7239e93121a 100755 --- a/src/scripts/dev_tools/run_clang_tidy.py +++ b/src/scripts/dev_tools/run_clang_tidy.py @@ -60,6 +60,7 @@ 'misc-confusable-identifiers', 'modernize-avoid-bind', 'modernize-pass-by-value', + 'modernize-use-ranges', # limited by compiler support currently 'performance-avoid-endl', 'readability-convert-member-functions-to-static', 'readability-implicit-bool-conversion', @@ -85,6 +86,7 @@ 'bugprone-branch-clone', # doesn't interact well with feature macros 'bugprone-easily-swappable-parameters', 'bugprone-implicit-widening-of-multiplication-result', + 'bugprone-suspicious-stringview-data-usage', # triggers on every use of string_view::data ?? 'cppcoreguidelines-avoid-do-while', 'cppcoreguidelines-non-private-member-variables-in-classes', # pk split keys 'cppcoreguidelines-pro-bounds-pointer-arithmetic', @@ -101,6 +103,7 @@ 'modernize-use-trailing-return-type', # fine, but we're not using it everywhere 'modernize-return-braced-init-list', # thanks I hate it 'modernize-use-default-member-init', + 'modernize-use-designated-initializers', 'modernize-use-nodiscard', 'modernize-use-using', # fine not great 'portability-simd-intrinsics', @@ -109,9 +112,11 @@ 'readability-function-cognitive-complexity', 'readability-identifier-length', # lol, lmao 'readability-isolate-declaration', + 'readability-math-missing-parentheses', 'readability-non-const-parameter', 'readability-redundant-access-specifiers', # reneme likes doing this 'readability-suspicious-call-argument', + 'readability-use-std-min-max', 'readability-use-anyofallof', # not more readable ] diff --git a/src/tests/test_kyber.cpp b/src/tests/test_kyber.cpp index 3ea8217cae6..3b920d2319f 100644 --- a/src/tests/test_kyber.cpp +++ b/src/tests/test_kyber.cpp @@ -330,8 +330,8 @@ void test_decompress(Test::Result& result) { result.start_timer(); - const auto twotothed = (uint32_t(1) << d); using from_t = std::conditional_t; + const from_t twotothed = static_cast(from_t(1) << d); for(from_t y = 0; y < twotothed; ++y) { const uint32_t c = Kyber_Algos::decompress(y); diff --git a/src/tests/test_rng_behavior.cpp b/src/tests/test_rng_behavior.cpp index 87744c3c70b..1d1b12da4ad 100644 --- a/src/tests/test_rng_behavior.cpp +++ b/src/tests/test_rng_behavior.cpp @@ -500,8 +500,10 @@ class HMAC_DRBG_Unit_Tests final : public Stateful_RNG_Tests { std::vector security_strengths{128, 192, 256, 256, 256, 256}; for(size_t i = 0; i < approved_hash_fns.size(); ++i) { - std::string hash_fn = approved_hash_fns[i]; - std::string mac_name = "HMAC(" + hash_fn + ")"; + const auto& hash_fn = approved_hash_fns[i]; + const size_t expected_security_level = security_strengths[i]; + + const std::string mac_name = "HMAC(" + hash_fn + ")"; auto mac = Botan::MessageAuthenticationCode::create(mac_name); if(!mac) { result.note_missing(mac_name); @@ -509,7 +511,7 @@ class HMAC_DRBG_Unit_Tests final : public Stateful_RNG_Tests { } Botan::HMAC_DRBG rng(std::move(mac)); - result.test_eq(hash_fn + " security level", rng.security_level(), security_strengths[i]); + result.test_eq(hash_fn + " security level", rng.security_level(), expected_security_level); } return result; diff --git a/src/tests/test_tls_session_manager.cpp b/src/tests/test_tls_session_manager.cpp index a9fd20e59fe..39d4120f4aa 100644 --- a/src/tests/test_tls_session_manager.cpp +++ b/src/tests/test_tls_session_manager.cpp @@ -335,6 +335,7 @@ std::vector test_session_manager_in_memory() { // fill the Session_Manager up fully std::vector handles; + handles.reserve(mgr->capacity()); for(size_t i = 0; i < mgr->capacity(); ++i) { handles.push_back( mgr->establish(default_session(Botan::TLS::Connection_Side::Server, cbs), random_id(*rng)).value()); @@ -707,7 +708,7 @@ std::vector test_session_manager_hybrid() { return Test::flatten_result_lists({ CHECK_all("ticket vs ID preference in establishment", - [&](auto make_manager, auto& result) { + [&](const auto& make_manager, auto& result) { auto mgr_prefers_tickets = make_manager(true); auto ticket1 = mgr_prefers_tickets.establish(default_session(Botan::TLS::Connection_Side::Server, cbs)); @@ -729,7 +730,7 @@ std::vector test_session_manager_hybrid() { }), CHECK_all("ticket vs ID preference in retrieval", - [&](auto make_manager, auto& result) { + [&](const auto& make_manager, auto& result) { auto mgr_prefers_tickets = make_manager(true); auto mgr_prefers_ids = make_manager(false); @@ -751,7 +752,7 @@ std::vector test_session_manager_hybrid() { }), CHECK_all("no session tickets if hybrid manager cannot create them", - [&](auto make_manager, auto& result) { + [&](const auto& make_manager, auto& result) { Botan::TLS::Session_Manager_Hybrid empty_mgr( std::make_unique(rng, 10), std::make_shared(), @@ -1026,7 +1027,7 @@ std::vector tls_session_manager_expiry() { return Test::flatten_result_lists({ CHECK_all("sessions expire", - [&](auto, auto factory, auto& result) { + [&](const auto&, const auto& factory, auto& result) { auto mgr = factory(); auto handle = mgr->establish(default_session(Botan::TLS::Connection_Side::Server, cbs)); @@ -1038,7 +1039,7 @@ std::vector tls_session_manager_expiry() { }), CHECK_all("expired sessions are not found", - [&](const std::string& type, auto factory, auto& result) { + [&](const std::string& type, const auto& factory, auto& result) { if(type == "Stateless") { return; // this manager can neither store nor find anything } @@ -1065,7 +1066,7 @@ std::vector tls_session_manager_expiry() { CHECK_all( "session tickets are not reused", - [&](const std::string& type, auto factory, auto& result) { + [&](const std::string& type, const auto& factory, auto& result) { if(type == "Stateless") { return; // this manager can neither store nor find anything } @@ -1103,7 +1104,7 @@ std::vector tls_session_manager_expiry() { }), CHECK_all("number of found tickets is capped", - [&](const std::string& type, auto factory, auto& result) { + [&](const std::string& type, const auto& factory, auto& result) { if(type == "Stateless") { return; // this manager can neither store nor find anything } @@ -1133,31 +1134,32 @@ std::vector tls_session_manager_expiry() { }), #if defined(BOTAN_HAS_TLS_13) - CHECK_all("expired tickets are not selected for PSK resumption", [&](auto, auto factory, auto& result) { - auto ticket = [&](const Botan::TLS::Session_Handle& handle) { - return Botan::TLS::PskIdentity(handle.opaque_handle().get(), 0); - }; - - auto mgr = factory(); - - auto old_handle = mgr->establish( - default_session(Botan::TLS::Connection_Side::Server, cbs, Botan::TLS::Version_Code::TLS_V13)); - cbs.tick(); - auto new_handle = mgr->establish( - default_session(Botan::TLS::Connection_Side::Server, cbs, Botan::TLS::Version_Code::TLS_V13)); - result.require("both sessions are stored", old_handle.has_value() && new_handle.has_value()); - - auto session_and_index = mgr->choose_from_offered_tickets( - std::vector{ticket(old_handle.value()), ticket(new_handle.value())}, "SHA-256", cbs, plcy); - result.require("a ticket was chosen", session_and_index.has_value()); - result.test_is_eq("the new ticket was chosen", session_and_index->second, uint16_t(1)); - - cbs.tick(); - - auto nothing = mgr->choose_from_offered_tickets( - std::vector{ticket(new_handle.value()), ticket(old_handle.value())}, "SHA-256", cbs, plcy); - result.require("all tickets are expired", !nothing.has_value()); - }), + CHECK_all("expired tickets are not selected for PSK resumption", + [&](const auto&, const auto& factory, auto& result) { + auto ticket = [&](const Botan::TLS::Session_Handle& handle) { + return Botan::TLS::PskIdentity(handle.opaque_handle().get(), 0); + }; + + auto mgr = factory(); + + auto old_handle = mgr->establish( + default_session(Botan::TLS::Connection_Side::Server, cbs, Botan::TLS::Version_Code::TLS_V13)); + cbs.tick(); + auto new_handle = mgr->establish( + default_session(Botan::TLS::Connection_Side::Server, cbs, Botan::TLS::Version_Code::TLS_V13)); + result.require("both sessions are stored", old_handle.has_value() && new_handle.has_value()); + + auto session_and_index = mgr->choose_from_offered_tickets( + std::vector{ticket(old_handle.value()), ticket(new_handle.value())}, "SHA-256", cbs, plcy); + result.require("a ticket was chosen", session_and_index.has_value()); + result.test_is_eq("the new ticket was chosen", session_and_index->second, uint16_t(1)); + + cbs.tick(); + + auto nothing = mgr->choose_from_offered_tickets( + std::vector{ticket(new_handle.value()), ticket(old_handle.value())}, "SHA-256", cbs, plcy); + result.require("all tickets are expired", !nothing.has_value()); + }), #endif }); } diff --git a/src/tests/tests.cpp b/src/tests/tests.cpp index 766b431b3c1..28025535791 100644 --- a/src/tests/tests.cpp +++ b/src/tests/tests.cpp @@ -1078,7 +1078,7 @@ std::string Text_Based_Test::get_next_line() { } if(line[0] == '#') { - if(line.compare(0, 6, "#test ") == 0) { + if(line.starts_with("#test ")) { return line; } else { continue; @@ -1136,7 +1136,7 @@ std::vector Text_Based_Test::run() { break; } - if(line.compare(0, 6, "#test ") == 0) { + if(line.starts_with("#test ")) { std::vector pragma_tokens = Botan::split_on(line.substr(6), ' '); if(pragma_tokens.empty()) {