Repository: randombit/botan
Pull request #3323 (2023-02-25 15:20:17)

Fix clang-tidy warnings in recently introduced code
cc @reneme on the TLS session manager changes


Patch:
diff --git a/src/lib/tls/tls_session_manager.cpp b/src/lib/tls/tls_session_manager.cpp
index ac740fa5c..d2fa2444e 100644
--- a/src/lib/tls/tls_session_manager.cpp
+++ b/src/lib/tls/tls_session_manager.cpp
@@ -17,9 +17,10 @@ namespace Botan::TLS {
 Session_Manager::Session_Manager(RandomNumberGenerator& rng)
    : m_rng(rng) {}
 
-std::optional<Session_Handle> Session_Manager::establish(const Session& session,
-            std::optional<Session_ID> id,
-            bool tls12_no_ticket)
+std::optional<Session_Handle> Session_Manager::establish(
+   const Session& session,
+   const std::optional<Session_ID>& id,
+   bool tls12_no_ticket)
 {
    // By default, the session manager does not emit session tickets anyway
    BOTAN_UNUSED(tls12_no_ticket);
diff --git a/src/lib/tls/tls_session_manager.h b/src/lib/tls/tls_session_manager.h
index d8b6d9ea9..343d685b3 100644
--- a/src/lib/tls/tls_session_manager.h
+++ b/src/lib/tls/tls_session_manager.h
@@ -67,9 +67,10 @@ class BOTAN_PUBLIC_API(3, 0) Session_Manager
        * @return a Session_Handle containing either an ID or a ticket
        *         if the session was saved, otherwise std::nullopt
        */
-      virtual std::optional<Session_Handle> establish(const Session& session,
-            std::optional<Session_ID> id = std::nullopt,
-            bool tls12_no_ticket = false);
+      virtual std::optional<Session_Handle> establish(
+         const Session& session,
+         const std::optional<Session_ID>& id = std::nullopt,
+         bool tls12_no_ticket = false);
 
       /**
        * @brief Save a Session under a Session_Handle (TLS Client)
diff --git a/src/lib/tls/tls_session_manager_hybrid.cpp b/src/lib/tls/tls_session_manager_hybrid.cpp
index 63b4c1bb9..193c342cf 100644
--- a/src/lib/tls/tls_session_manager_hybrid.cpp
+++ b/src/lib/tls/tls_session_manager_hybrid.cpp
@@ -26,7 +26,9 @@ Session_Manager_Hybrid::Session_Manager_Hybrid(std::unique_ptr<Session_Manager>
    }
 
 std::optional<Session_Handle>
-Session_Manager_Hybrid::establish(const Session& session, std::optional<Session_ID> id, bool tls12_no_ticket)
+Session_Manager_Hybrid::establish(const Session& session,
+                                  const std::optional<Session_ID>& id,
+                                  bool tls12_no_ticket)
    {
    auto create_ticket = [&]() -> std::optional<Session_Handle>
       {
diff --git a/src/lib/tls/tls_session_manager_hybrid.h b/src/lib/tls/tls_session_manager_hybrid.h
index 48b22f918..d3ba56656 100644
--- a/src/lib/tls/tls_session_manager_hybrid.h
+++ b/src/lib/tls/tls_session_manager_hybrid.h
@@ -55,7 +55,8 @@ class BOTAN_PUBLIC_API(3, 0) Session_Manager_Hybrid final : public Session_Manag
                              RandomNumberGenerator& rng,
                              bool prefer_tickets = true);
 
-      std::optional<Session_Handle> establish(const Session& session, std::optional<Session_ID> id = std::nullopt,
+      std::optional<Session_Handle> establish(const Session& session,
+                                              const std::optional<Session_ID>& id = std::nullopt,
                                               bool tls12_no_ticket = false) override;
 
       std::optional<Session> retrieve(const Session_Handle& handle,
diff --git a/src/lib/tls/tls_session_manager_memory.cpp b/src/lib/tls/tls_session_manager_memory.cpp
index 962a0d107..fa9a9ad7f 100644
--- a/src/lib/tls/tls_session_manager_memory.cpp
+++ b/src/lib/tls/tls_session_manager_memory.cpp
@@ -106,9 +106,8 @@ size_t Session_Manager_In_Memory::remove_internal(const Session_Handle& handle)
          const auto before = m_sessions.size();
          std::erase_if(m_sessions, [&](const auto& item)
             {
-            const auto& [_, session_and_handle] = item;
-            const auto& [__, this_handle] = session_and_handle;
-
+            const auto& [_unused1, session_and_handle] = item;
+            const auto& [_unused2, this_handle] = session_and_handle;
             return this_handle.is_ticket() && this_handle.ticket().value() == ticket;
             });
          return before - m_sessions.size();
diff --git a/src/lib/tls/tls_session_manager_noop.h b/src/lib/tls/tls_session_manager_noop.h
index f365cedb0..21eb2b136 100644
--- a/src/lib/tls/tls_session_manager_noop.h
+++ b/src/lib/tls/tls_session_manager_noop.h
@@ -26,8 +26,10 @@ class BOTAN_PUBLIC_API(3, 0) Session_Manager_Noop final : public Session_Manager
    public:
       Session_Manager_Noop();
 
-      std::optional<Session_Handle> establish(const Session&, std::optional<Session_ID> = std::nullopt,
+      std::optional<Session_Handle> establish(const Session&,
+                                              const std::optional<Session_ID>& = std::nullopt,
                                               bool = false) override { return std::nullopt; }
+
       void store(const Session&, const Session_Handle&) override {}
       size_t remove(const Session_Handle&) override { return 0; }
       size_t remove_all() override { return 0; }
diff --git a/src/lib/tls/tls_session_manager_stateless.cpp b/src/lib/tls/tls_session_manager_stateless.cpp
index a37227193..9d92af4a2 100644
--- a/src/lib/tls/tls_session_manager_stateless.cpp
+++ b/src/lib/tls/tls_session_manager_stateless.cpp
@@ -20,8 +20,10 @@ Session_Manager_Stateless::Session_Manager_Stateless(Credentials_Manager& creds,
    : Session_Manager(rng)
    , m_credentials_manager(creds) {}
 
-std::optional<Session_Handle> Session_Manager_Stateless::establish(const Session& session, std::optional<Session_ID>,
-      bool tls12_no_ticket)
+std::optional<Session_Handle> Session_Manager_Stateless::establish(
+   const Session& session,
+   const std::optional<Session_ID>&,
+   bool tls12_no_ticket)
    {
    BOTAN_ASSERT(session.side() == Connection_Side::Server,
                 "Client tried to establish a session");
diff --git a/src/lib/tls/tls_session_manager_stateless.h b/src/lib/tls/tls_session_manager_stateless.h
index 63a0fe568..fa9aa119e 100644
--- a/src/lib/tls/tls_session_manager_stateless.h
+++ b/src/lib/tls/tls_session_manager_stateless.h
@@ -41,7 +41,10 @@ class BOTAN_PUBLIC_API(3,0) Session_Manager_Stateless : public Session_Manager
        */
       Session_Manager_Stateless(Credentials_Manager &credentials_manager, RandomNumberGenerator& rng);
 
-      std::optional<Session_Handle> establish(const Session& session, std::optional<Session_ID> id = std::nullopt, bool tls12_no_ticket = false) override;
+      std::optional<Session_Handle> establish(const Session& session,
+                                              const std::optional<Session_ID>& id = std::nullopt,
+                                              bool tls12_no_ticket = false) override;
+
       void store(const Session& session, const Session_Handle& handle) override;
 
       size_t remove(const Session_Handle&) override { return 0; }
diff --git a/src/scripts/dev_tools/run_clang_tidy.py b/src/scripts/dev_tools/run_clang_tidy.py
index 910e62a2e..727b8b651 100755
--- a/src/scripts/dev_tools/run_clang_tidy.py
+++ b/src/scripts/dev_tools/run_clang_tidy.py
@@ -53,7 +53,7 @@ disabled_needs_work = [
     'modernize-pass-by-value',
     'readability-convert-member-functions-to-static',
     'readability-implicit-bool-conversion', # maybe fix this
-    'readability-inconsistent-declaration-parameter-name', # should fix this
+    'readability-inconsistent-declaration-parameter-name', # should fix this, blocked by https://github.com/llvm/llvm-project/issues/60845
     'readability-qualified-auto',
     'readability-simplify-boolean-expr', # sometimes ok
     'readability-static-accessed-through-instance',


Test patch:
diff --git a/src/tests/test_ffi.cpp b/src/tests/test_ffi.cpp
index a5212d627..bd385949f 100644
--- a/src/tests/test_ffi.cpp
+++ b/src/tests/test_ffi.cpp
@@ -360,14 +360,19 @@ class FFI_Unit_Tests final : public Test
          const uint8_t input[totalSize + 1] = "Does this work?AAAAAAAAAAAAAAAzzzzzzzzzzzzzzz";
 
          // Allocate memory for the encoding and decoding output parameters.
+         std::vector<uint8_t> encoded_buf(N * blockSize);
+         std::vector<uint8_t> decoded_buf(K * blockSize);
+
          std::vector<uint8_t*> encoded(N);
+         for(size_t i = 0; i < N; ++i)
+            {
+            encoded[i] = &encoded_buf[i * blockSize];
+            }
          std::vector<uint8_t*> decoded(K);
-         for (size_t n = 0; n < N; ++n) {
-            encoded[n] = new uint8_t[blockSize];
-         };
-         for (size_t k = 0; k < K; ++k) {
-            decoded[k] = new uint8_t[blockSize];
-         };
+         for(size_t i = 0; i < K; ++i)
+            {
+            decoded[i] = &decoded_buf[i * blockSize];
+            }
 
          // First encode the complete input string into N blocks where K are
          // required for reconstruction.  The N encoded blocks will end up in
@@ -392,14 +397,6 @@ class FFI_Unit_Tests final : public Test
             }
          }
 
-         // Clean up the memory we allocated at the top.
-         for (size_t n = 0; n < N; ++n) {
-            delete[] encoded[n];
-         }
-         for (size_t k = 0; k < K; ++k) {
-            delete[] decoded[k];
-         }
-
          /* Exercise a couple basic failure cases, such as you encounter if
           * the caller supplies invalid parameters.  We don't try to
           * exhaustively prove invalid parameters are handled through this
@@ -409,10 +406,10 @@ class FFI_Unit_Tests final : public Test
           */
          TEST_FFI_FAIL("encode with out-of-bounds encoding parameters should have failed",
                        botan_zfec_encode,
-                       (0, 0, NULL, 0, NULL));
+                       (0, 0, nullptr, 0, nullptr));
          TEST_FFI_FAIL("decode with out-of-bounds encoding parameters should have failed",
                        botan_zfec_decode,
-                       (0, 0, NULL, NULL, 0, NULL));
+                       (0, 0, nullptr, nullptr, 0, nullptr));
 #endif
 
          return result;
diff --git a/src/tests/test_tls_rfc8448.cpp b/src/tests/test_tls_rfc8448.cpp
index e439b6001..2521ab426 100644
--- a/src/tests/test_tls_rfc8448.cpp
+++ b/src/tests/test_tls_rfc8448.cpp
@@ -546,7 +546,7 @@ class RFC8448_Session_Manager : public Botan::TLS::Session_Manager
    private:
       decltype(auto) find_by_handle(const Session_Handle& handle)
          {
-         return [=](const Session_with_Handle session)
+         return [=](const Session_with_Handle& session)
             {
             if(session.handle.id().has_value() && handle.id().has_value() &&
                session.handle.id().value() == handle.id().value())
@@ -572,7 +572,7 @@ class RFC8448_Session_Manager : public Botan::TLS::Session_Manager
          m_sessions.push_back({session, handle});
          }
 
-      std::optional<Session_Handle> establish(const Session& session, std::optional<Session_ID> id, bool) override
+      std::optional<Session_Handle> establish(const Session& session, const std::optional<Session_ID>& id, bool) override
          {
          m_sessions.emplace_back(Session_with_Handle{session, id.value_or(Session_ID())});
          return id;
diff --git a/src/tests/test_tls_session_manager.cpp b/src/tests/test_tls_session_manager.cpp
index ded1427a0..5d1799ec8 100644
--- a/src/tests/test_tls_session_manager.cpp
+++ b/src/tests/test_tls_session_manager.cpp
@@ -389,7 +389,7 @@ std::vector<Test::Result> test_session_manager_choose_ticket()
    Session_Manager_Callbacks cbs;
    Session_Manager_Policy plcy;
 
-   auto default_session = [&](std::string suite, Botan::TLS::Callbacks& mycbs)
+   auto default_session = [&](const std::string& suite, Botan::TLS::Callbacks& mycbs)
       {
       return Botan::TLS::Session(
             {}, std::nullopt, 0, std::chrono::seconds(1024),
@@ -614,7 +614,7 @@ std::vector<Test::Result> test_session_manager_hybrid()
    // managers. The `make_manager()` helper is passed into the test code and
    // transparently constructs a hybrid manager with the respective internal
    // stateful manager.
-   auto CHECK_all = [&](std::string name, auto lambda) -> std::vector<Test::Result>
+   auto CHECK_all = [&](const std::string& name, auto lambda) -> std::vector<Test::Result>
       {
       std::vector<std::pair<std::string, std::function<std::unique_ptr<Botan::TLS::Session_Manager>()>>>
          stateful_manager_factories =
@@ -728,6 +728,8 @@ class Temporary_Database_File
 
       Temporary_Database_File(const Temporary_Database_File&) = delete;
       Temporary_Database_File& operator=(const Temporary_Database_File&) = delete;
+      Temporary_Database_File(Temporary_Database_File&&) = delete;
+      Temporary_Database_File& operator=(Temporary_Database_File&&) = delete;
    };
 
 }
@@ -895,7 +897,7 @@ std::vector<Test::Result> tls_session_manager_expiry()
    Session_Manager_Callbacks cbs;
    Session_Manager_Policy plcy;
 
-   auto CHECK_all = [&](std::string name, auto lambda) -> std::vector<Test::Result>
+   auto CHECK_all = [&](const std::string& name, auto lambda) -> std::vector<Test::Result>
       {
       std::vector<std::pair<std::string, std::function<std::unique_ptr<Botan::TLS::Session_Manager>()>>>
          stateful_manager_factories =
@@ -908,6 +910,7 @@ std::vector<Test::Result> tls_session_manager_expiry()
             };
 
       std::vector<Test::Result> results;
+      results.reserve(stateful_manager_factories.size());
       using namespace std::placeholders;
       for(auto& [sub_name, factory] : stateful_manager_factories)
          {
@@ -930,7 +933,7 @@ std::vector<Test::Result> tls_session_manager_expiry()
          result.test_is_eq("session was deleted when it expired", mgr->remove_all(), size_t(0));
          }),
 
-      CHECK_all("expired sessions are not found", [&](std::string type, auto factory, auto& result)
+      CHECK_all("expired sessions are not found", [&](const std::string& type, auto factory, auto& result)
          {
          if(type == "Stateless")
             return; // this manager can neither store nor find anything
@@ -954,7 +957,7 @@ std::vector<Test::Result> tls_session_manager_expiry()
          result.test_is_eq("old session was deleted when it expired", mgr->remove_all(), size_t(1));
          }),
 
-      CHECK_all("session tickets are not reused", [&](std::string type, auto factory, auto& result)
+      CHECK_all("session tickets are not reused", [&](const std::string& type, auto factory, auto& result)
          {
          if(type == "Stateless")
             return; // this manager can neither store nor find anything
@@ -985,7 +988,7 @@ std::vector<Test::Result> tls_session_manager_expiry()
          result.confirm("found session is TLS 1.2", sessions_and_handles2.front().session.version().is_pre_tls_13());
          }),
 
-      CHECK_all("number of found tickets is capped", [&](std::string type, auto factory, auto& result)
+      CHECK_all("number of found tickets is capped", [&](const std::string& type, auto factory, auto& result)
          {
          if(type == "Stateless")
             return;  // this manager can neither store nor find anything
@@ -1014,7 +1017,7 @@ std::vector<Test::Result> 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 = [&](Botan::TLS::Session_Handle handle)
+         auto ticket = [&](const Botan::TLS::Session_Handle& handle)
             {
             return Botan::TLS::Ticket(handle.opaque_handle(), 0);
             };