Repository: randombit/botan
Pull request #3573 (2023-06-02 11:22:20)

Enable some additional clang-tidy warnings
We have exceptions where we violate the check, but for those cases add NOLINT annotations. This at least helps prevent creating more instances of the problem in the future, and makes existing violations easy to find.

randombit: @reneme It seems like the `Server_Hello_13` tag types can be const without issue but I wanted to check with you

Patch:
diff --git a/src/lib/ffi/ffi.cpp b/src/lib/ffi/ffi.cpp
index 66614dd46..8aa26b362 100644
--- a/src/lib/ffi/ffi.cpp
+++ b/src/lib/ffi/ffi.cpp
@@ -17,6 +17,7 @@
 
 namespace Botan_FFI {
 
+// NOLINTNEXTLINE(*-avoid-non-const-global-variables)
 thread_local std::string g_last_exception_what;
 
 int ffi_error_exception_thrown(const char* func_name, const char* exn, int rc) {
diff --git a/src/lib/prov/pkcs11/p11.cpp b/src/lib/prov/pkcs11/p11.cpp
index 0c9a64b54..d399ebafa 100644
--- a/src/lib/prov/pkcs11/p11.cpp
+++ b/src/lib/prov/pkcs11/p11.cpp
@@ -17,7 +17,7 @@
 
 namespace Botan::PKCS11 {
 
-// NOLINTNEXTLINE(*-no-int-to-ptr)
+// NOLINTNEXTLINE(*-no-int-to-ptr,*-avoid-non-const-global-variables)
 ReturnValue* ThrowException = reinterpret_cast<ReturnValue*>(-1);
 
 /// @param function_result Return value of the PKCS11 module function
diff --git a/src/lib/tls/msg_client_hello.cpp b/src/lib/tls/msg_client_hello.cpp
index 870998040..79ed0cc15 100644
--- a/src/lib/tls/msg_client_hello.cpp
+++ b/src/lib/tls/msg_client_hello.cpp
@@ -154,15 +154,16 @@ class Client_Hello_Internal {
       Extensions& extensions() { return m_extensions; }
 
    public:
-      Protocol_Version m_legacy_version;
-      Session_ID m_session_id;
-      std::vector<uint8_t> m_random;
-      std::vector<uint16_t> m_suites;
-      std::vector<uint8_t> m_comp_methods;
-      Extensions m_extensions;
-
-      std::vector<uint8_t> m_hello_cookie;       // DTLS only
-      std::vector<uint8_t> m_cookie_input_bits;  // DTLS only
+      Protocol_Version m_legacy_version;    // NOLINT(*-non-private-member-variables-in-classes)
+      Session_ID m_session_id;              // NOLINT(*-non-private-member-variables-in-classes)
+      std::vector<uint8_t> m_random;        // NOLINT(*-non-private-member-variables-in-classes)
+      std::vector<uint16_t> m_suites;       // NOLINT(*-non-private-member-variables-in-classes)
+      std::vector<uint8_t> m_comp_methods;  // NOLINT(*-non-private-member-variables-in-classes)
+      Extensions m_extensions;              // NOLINT(*-non-private-member-variables-in-classes)
+
+      // These fields are only for DTLS:
+      std::vector<uint8_t> m_hello_cookie;       // NOLINT(*-non-private-member-variables-in-classes)
+      std::vector<uint8_t> m_cookie_input_bits;  // NOLINT(*-non-private-member-variables-in-classes)
 };
 
 Client_Hello::Client_Hello(Client_Hello&&) noexcept = default;
diff --git a/src/lib/tls/msg_server_hello.cpp b/src/lib/tls/msg_server_hello.cpp
index 808c9929b..ea36a145a 100644
--- a/src/lib/tls/msg_server_hello.cpp
+++ b/src/lib/tls/msg_server_hello.cpp
@@ -423,9 +423,9 @@ std::vector<uint8_t> Server_Hello_Done::serialize() const { return std::vector<u
 
 #if defined(BOTAN_HAS_TLS_13)
 
-Server_Hello_13::Server_Hello_Tag Server_Hello_13::as_server_hello;
-Server_Hello_13::Hello_Retry_Request_Tag Server_Hello_13::as_hello_retry_request;
-Server_Hello_13::Hello_Retry_Request_Creation_Tag Server_Hello_13::as_new_hello_retry_request;
+const Server_Hello_13::Server_Hello_Tag Server_Hello_13::as_server_hello;
+const Server_Hello_13::Hello_Retry_Request_Tag Server_Hello_13::as_hello_retry_request;
+const Server_Hello_13::Hello_Retry_Request_Creation_Tag Server_Hello_13::as_new_hello_retry_request;
 
 std::variant<Hello_Retry_Request, Server_Hello_13> Server_Hello_13::create(const Client_Hello_13& ch,
                                                                            bool hello_retry_request_allowed,
diff --git a/src/lib/tls/tls13/tls_extensions_key_share.cpp b/src/lib/tls/tls13/tls_extensions_key_share.cpp
index c10473784..fe0cd1178 100644
--- a/src/lib/tls/tls13/tls_extensions_key_share.cpp
+++ b/src/lib/tls/tls13/tls_extensions_key_share.cpp
@@ -385,6 +385,7 @@ class Key_Share::Key_Share_Impl {
 
       Key_Share_Impl(Key_Share_Type ks) : key_share(std::move(ks)) {}
 
+      // NOLINTNEXTLINE(*-non-private-member-variables-in-classes)
       Key_Share_Type key_share;
 };
 
diff --git a/src/lib/tls/tls13/tls_extensions_psk.cpp b/src/lib/tls/tls13/tls_extensions_psk.cpp
index 503111d61..54f11a2e8 100644
--- a/src/lib/tls/tls13/tls_extensions_psk.cpp
+++ b/src/lib/tls/tls13/tls_extensions_psk.cpp
@@ -50,6 +50,7 @@ class PSK::PSK_Internal {
 
       PSK_Internal(std::vector<Client_PSK> clt_psks) : psk(std::move(clt_psks)) {}
 
+      // NOLINTNEXTLINE(*-non-private-member-variables-in-classes)
       std::variant<std::vector<Client_PSK>, Server_PSK> psk;
 };
 
diff --git a/src/lib/tls/tls_extensions_cert_status_req.cpp b/src/lib/tls/tls_extensions_cert_status_req.cpp
index 0c0573bea..1cf2a19cc 100644
--- a/src/lib/tls/tls_extensions_cert_status_req.cpp
+++ b/src/lib/tls/tls_extensions_cert_status_req.cpp
@@ -65,9 +65,9 @@ class RFC6066_Certificate_Status_Request {
          };
       }
 
-      std::vector<uint8_t> ocsp_names;
-      std::vector<std::vector<uint8_t>> ocsp_keys;
-      std::vector<uint8_t> extension_bytes;
+      std::vector<uint8_t> ocsp_names;              // NOLINT(*-non-private-member-variables-in-classes)
+      std::vector<std::vector<uint8_t>> ocsp_keys;  // NOLINT(*-non-private-member-variables-in-classes)
+      std::vector<uint8_t> extension_bytes;         // NOLINT(*-non-private-member-variables-in-classes)
 };
 
 }  // namespace
@@ -80,7 +80,7 @@ class Certificate_Status_Request_Internal {
    public:
       Certificate_Status_Request_Internal(Contents c) : content(std::move(c)) {}
 
-      Contents content;
+      Contents content;  // NOLINT(*-non-private-member-variables-in-classes)
 };
 
 Certificate_Status_Request::Certificate_Status_Request(TLS_Data_Reader& reader,
diff --git a/src/lib/tls/tls_messages.h b/src/lib/tls/tls_messages.h
index 05c620684..8997916f2 100644
--- a/src/lib/tls/tls_messages.h
+++ b/src/lib/tls/tls_messages.h
@@ -394,13 +394,13 @@ class Hello_Retry_Request;
 
 class BOTAN_UNSTABLE_API Server_Hello_13 : public Server_Hello {
    protected:
-      static struct Server_Hello_Tag {
+      static const struct Server_Hello_Tag {
       } as_server_hello;
 
-      static struct Hello_Retry_Request_Tag {
+      static const struct Hello_Retry_Request_Tag {
       } as_hello_retry_request;
 
-      static struct Hello_Retry_Request_Creation_Tag {
+      static const struct Hello_Retry_Request_Creation_Tag {
       } as_new_hello_retry_request;
 
       // These constructors are meant for instantiating Server Hellos
diff --git a/src/lib/utils/locking_allocator/locking_allocator.cpp b/src/lib/utils/locking_allocator/locking_allocator.cpp
index b60339d00..32706b71f 100644
--- a/src/lib/utils/locking_allocator/locking_allocator.cpp
+++ b/src/lib/utils/locking_allocator/locking_allocator.cpp
@@ -66,9 +66,10 @@ mlock_allocator::~mlock_allocator() {
 
 namespace {
 
+// NOLINTNEXTLINE(*-avoid-non-const-global-variables)
 BOTAN_EARLY_INIT(101) mlock_allocator g_mlock_allocator;
 
-}
+}  // namespace
 
 mlock_allocator& mlock_allocator::instance() { return g_mlock_allocator; }
 
diff --git a/src/lib/utils/os_utils.cpp b/src/lib/utils/os_utils.cpp
index 8f2f7d6b7..d39d2800a 100644
--- a/src/lib/utils/os_utils.cpp
+++ b/src/lib/utils/os_utils.cpp
@@ -618,6 +618,7 @@ void OS::page_named(void* page, size_t size) {
 
 namespace {
 
+// NOLINTNEXTLINE(*-avoid-non-const-global-variables)
 ::sigjmp_buf g_sigill_jmp_buf;
 
 void botan_sigill_handler(int /*unused*/) { siglongjmp(g_sigill_jmp_buf, /*non-zero return value*/ 1); }
diff --git a/src/scripts/dev_tools/run_clang_tidy.py b/src/scripts/dev_tools/run_clang_tidy.py
index 026a181da..367b5b05d 100755
--- a/src/scripts/dev_tools/run_clang_tidy.py
+++ b/src/scripts/dev_tools/run_clang_tidy.py
@@ -44,12 +44,11 @@ disabled_checks_non_lib = [
     'performance-no-automatic-move',
 ]
 
-# these are ones that ideally we would be clean for, but
-# currently are not
+# these are ones that we might want to be clean for in the future,
+# but currently are not
 disabled_needs_work = [
-    'misc-non-private-member-variables-in-classes',
     '*-named-parameter',
-    '*-member-init', # seems bad
+    '*-member-init', # should definitely fix this one
     'bugprone-lambda-function-name', # should be an easy fix
     'bugprone-unchecked-optional-access', # clang-tidy seems buggy (many false positives)
     'cert-err58-cpp', # many false positives eg __m128i
@@ -62,10 +61,10 @@ disabled_needs_work = [
     'misc-redundant-expression', # BigInt seems to confuse clang-tidy
     'misc-misplaced-const',
     'misc-confusable-identifiers',
-    'modernize-avoid-bind', # used a lot in pkcs11
+    'modernize-avoid-bind',
     'modernize-pass-by-value',
     'readability-convert-member-functions-to-static',
-    'readability-implicit-bool-conversion', # maybe fix this
+    'readability-implicit-bool-conversion',
     '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
@@ -88,7 +87,6 @@ disabled_not_interested = [
     'bugprone-branch-clone', # doesn't interact well with feature macros
     'bugprone-easily-swappable-parameters',
     'bugprone-implicit-widening-of-multiplication-result',
-    'cppcoreguidelines-avoid-non-const-global-variables',
     'cppcoreguidelines-non-private-member-variables-in-classes', # pk split keys
     'cppcoreguidelines-pro-bounds-pointer-arithmetic',
     'cppcoreguidelines-pro-bounds-constant-array-index',


Test patch:
diff --git a/src/tests/test_pkcs11_low_level.cpp b/src/tests/test_pkcs11_low_level.cpp
index 71205af6d..985d86101 100644
--- a/src/tests/test_pkcs11_low_level.cpp
+++ b/src/tests/test_pkcs11_low_level.cpp
@@ -629,7 +629,7 @@ const std::string label = "A data object";
 const std::string data = "Sample data";
 const Bbool btrue = True;
 
-std::array<Attribute, 4> dtemplate = {
+const std::array<Attribute, 4> data_template = {
    {{static_cast<CK_ATTRIBUTE_TYPE>(AttributeType::Class),
      const_cast<ObjectClass*>(&object_class),
      sizeof(object_class)},
@@ -643,6 +643,8 @@ std::array<Attribute, 4> dtemplate = {
 
 ObjectHandle create_simple_data_object(const RAII_LowLevel& p11_low_level) {
    ObjectHandle object_handle;
+
+   auto dtemplate = data_template;
    p11_low_level.get()->C_CreateObject(
       p11_low_level.get_session_handle(), dtemplate.data(), static_cast<Ulong>(dtemplate.size()), &object_handle);
    return object_handle;
@@ -654,6 +656,8 @@ Test::Result test_c_create_object_c_destroy_object() {
 
    ObjectHandle object_handle(0);
 
+   auto dtemplate = data_template;
+
    auto create_bind = std::bind(&LowLevel::C_CreateObject,
                                 *p11_low_level.get(),
                                 session_handle,
diff --git a/src/tests/tests.cpp b/src/tests/tests.cpp
index b6dea5198..8aa7249ba 100644
--- a/src/tests/tests.cpp
+++ b/src/tests/tests.cpp
@@ -681,7 +681,9 @@ std::vector<uint8_t> Test::read_binary_data_file(const std::string& path) {
 
 // static member variables of Test
 
+// NOLINTNEXTLINE(*-avoid-non-const-global-variables)
 Test_Options Test::m_opts;
+// NOLINTNEXTLINE(*-avoid-non-const-global-variables)
 std::shared_ptr<Botan::RandomNumberGenerator> Test::m_test_rng;
 
 //static