Address cert-oop54-cpp & concurrency-mt-unsafe warnings

.clang-tidy

@@ -1,5 +1,5 @@
 FormatStyle: file
-HeaderFilterRegex: 'silkworm/(api|core|infra|node|sentry|rpc|sync)/.+\.hpp$'
+HeaderFilterRegex: 'silkworm/(capi|core|infra|node|rpc|sentry|sync|wasm)/.+\.hpp$'
 WarningsAsErrors: '*'
 Checks: >
   abseil-*,
@@ -12,11 +12,9 @@ Checks: >
   -bugprone-unused-raii,
   cert-*,
   -cert-err58-cpp,
-  -cert-oop54-cpp,
   -clang-analyzer-*,
   clang-diagnostic-*,
   concurrency-*,
-  -concurrency-mt-unsafe,
   cppcoreguidelines-*,
   -cppcoreguidelines-avoid-c-arrays,
   -cppcoreguidelines-avoid-const-or-ref-data-members,

silkworm/core/common/small_map.hpp

@@ -59,6 +59,9 @@
         }
     }
     constexpr SmallMap& operator=(const SmallMap& other) {
+        if (this == &other) {
+            return *this;
+        }
         size_ = other.size_;
         for (size_t i{0}; i < max_size; ++i) {
             data_[i] = other.data_[i];

silkworm/core/concurrency/resettable_once_flag.hpp

@@ -45,6 +45,9 @@
         }
     }
     ResettableOnceFlag& operator=(const ResettableOnceFlag& other) {
+        if (this == &other) {
+            return *this;
+        }
         const uint32_t other_flag{other.flag_.load(std::memory_order_acquire)};
         if (other_flag == absl::base_internal::kOnceDone) {
             flag_.store(absl::base_internal::kOnceDone, std::memory_order_release);

silkworm/infra/common/directories.cpp

@@ -138,7 +138,8 @@ DataDirectory DataDirectory::from_chaindata(const std::filesystem::path& chainda
 
 std::filesystem::path silkworm::DataDirectory::get_default_storage_path() {
     std::string base_dir_str{};
-    const char* env{std::getenv("XDG_DATA_HOME")};
+    // C++11 guarantees some thread safety for std::getenv
+    const char* env{std::getenv("XDG_DATA_HOME")};  // NOLINT(concurrency-mt-unsafe)
     if (env) {
         // Got storage path from docker
         base_dir_str.assign(env);
@@ -148,7 +149,7 @@ std::filesystem::path silkworm::DataDirectory::get_default_storage_path() {
 #else
         std::string env_name{"HOME"};
 #endif
-        env = std::getenv(env_name.c_str());
+        env = std::getenv(env_name.c_str());  // NOLINT(concurrency-mt-unsafe)
         if (!env) {
             // We don't actually know where to store data
             // fallback to current directory

silkworm/infra/common/memory_mapped_file.cpp

@@ -33,6 +33,7 @@
 #include <gsl/util>
 
 #include "ensure.hpp"
+#include "safe_strerror.hpp"
 
 namespace silkworm {
 
@@ -137,7 +138,7 @@
 void MemoryMappedFile::map_existing(bool read_only) {
     FileDescriptor fd = ::open(path_.c_str(), read_only ? O_RDONLY : O_RDWR);
     if (fd == -1) {
-        throw std::runtime_error{"open failed for: " + path_.string() + " error: " + strerror(errno)};
+        throw std::runtime_error{"open failed for: " + path_.string() + " error: " + safe_strerror(errno)};
     }
     [[maybe_unused]] auto _ = gsl::finally([fd]() { ::close(fd); });
 
@@ -163,7 +164,7 @@
 
     const auto address = ::mmap(nullptr, length_, read_only ? PROT_READ : (PROT_READ | PROT_WRITE), flags, fd, 0);
     if (address == MAP_FAILED) {
-        throw std::runtime_error{"mmap failed for: " + path_.string() + " error: " + strerror(errno)};
+        throw std::runtime_error{"mmap failed for: " + path_.string() + " error: " + safe_strerror(errno)};
     }
 
     return address;
@@ -173,7 +174,7 @@
     if (address_ != nullptr) {
         const int result = ::munmap(address_, length_);
         if (result == -1) {
-            throw std::runtime_error{"munmap failed for: " + path_.string() + " error: " + strerror(errno)};
+            throw std::runtime_error{"munmap failed for: " + path_.string() + " error: " + safe_strerror(errno)};
         }
     }
 }
@@ -183,7 +184,7 @@
     if (result == -1) {
         // Ignore not implemented in kernel error because it still works (from Erigon)
         if (errno != ENOSYS) {
-            throw std::runtime_error{"madvise failed for: " + path_.string() + " error: " + strerror(errno)};
+            throw std::runtime_error{"madvise failed for: " + path_.string() + " error: " + safe_strerror(errno)};
         }
     }
 }

silkworm/node/etl/util.cpp → silkworm/infra/common/safe_strerror.cpp

@@ -14,13 +14,15 @@
    limitations under the License.
 */
 
-#include "util.hpp"
+#include "safe_strerror.hpp"
 
-namespace silkworm::etl {
+#include <cstring>
 
-std::string errno2str(int err_code) {
-    char msg[64];
-#if defined(_WIN32) || defined(_WIN64)
+namespace silkworm {
+
+std::string safe_strerror(int err_code) {
+    char msg[256];
+#if defined(_WIN32)
     if (strerror_s(msg, sizeof(msg), err_code) != 0) {
         (void)strncpy_s(msg, "Unknown error", _TRUNCATE);
     }
@@ -33,4 +35,4 @@ std::string errno2str(int err_code) {
     return {msg};
 }
 
-}  // namespace silkworm::etl
+}  // namespace silkworm

silkworm/infra/common/safe_strerror.hpp

@@ -0,0 +1,27 @@
+/*
+   Copyright 2024 The Silkworm Authors
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+#pragma once
+
+#include <string>
+
+namespace silkworm {
+
+//! \brief Converts a system error code into its message.
+//! \remarks Thread-safe version of strerror.
+std::string safe_strerror(int err_code);
+
+}  // namespace silkworm

silkworm/infra/concurrency/parallel_group_test.cpp

@@ -35,7 +35,7 @@ using namespace boost::asio::experimental;
 using namespace silkworm::concurrency;
 using namespace std::chrono_literals;
 
-awaitable<void> sleep(std::chrono::milliseconds duration) {
+awaitable<void> my_sleep(std::chrono::milliseconds duration) {
     auto executor = co_await this_coro::executor;
     steady_timer timer(executor);
     timer.expires_after(duration);
@@ -47,7 +47,7 @@ awaitable<void> noop() {
 }
 
 awaitable<void> throw_op() {
-    co_await sleep(1ms);
+    co_await my_sleep(1ms);
     throw std::runtime_error("throw_op");
 }
 
@@ -67,7 +67,7 @@ awaitable<void> co_spawn_cancellation_handler_bug() {
     auto strand = make_strand(executor);
 
     try {
-        co_await (sleep(1s) && spawn_throw_op(strand) && spawn_noop_loop(strand));
+        co_await (my_sleep(1s) && spawn_throw_op(strand) && spawn_noop_loop(strand));
     } catch (std::runtime_error&) {
     }
 }

silkworm/node/bittorrent/client.cpp

@@ -30,6 +30,7 @@
 #include <libtorrent/write_resume_data.hpp>
 #include <magic_enum.hpp>
 
+#include <silkworm/core/common/assert.hpp>
 #include <silkworm/core/common/base.hpp>
 #include <silkworm/core/common/util.hpp>
 #include <silkworm/infra/common/ensure.hpp>
@@ -294,10 +295,16 @@
     // When we receive the finished alert, we request to save resume data for the torrent
     if (const auto* tfa = lt::alert_cast<lt::torrent_finished_alert>(alert)) {
         const auto& status = tfa->handle.status();
+        std::tm completed_calendar_time{};
+#ifdef _MSC_VER
+        SILKWORM_ASSERT(gmtime_s(&completed_calendar_time, &status.completed_time) == 0);
+#else
+        SILKWORM_ASSERT(gmtime_r(&status.completed_time, &completed_calendar_time) != nullptr);
+#endif
         SILK_TRACE << "Torrent: " << tfa->torrent_name() << " finished download_rate: " << (status.download_rate / 1000) << " kB/s"
                    << " download_payload_rate: " << (status.download_payload_rate / 1000) << " kB/s"
                    << " in " << (status.completed_time - status.added_time) << " sec at "
-                   << std::put_time(std::gmtime(&status.completed_time), "%c %Z");
+                   << std::put_time(&completed_calendar_time, "%c %Z");
 
         tfa->handle.save_resume_data(lt::torrent_handle::save_info_dict | lt::torrent_handle::flush_disk_cache);
         ++outstanding_resume_requests_;

silkworm/node/etl/file_provider.cpp

@@ -19,6 +19,7 @@
 #include <filesystem>
 
 #include <silkworm/core/common/bytes_to_string.hpp>
+#include <silkworm/infra/common/safe_strerror.hpp>
 
 namespace silkworm::etl {
 
@@ -45,7 +46,7 @@
     file_.open(file_name_, std::ios_base::out | std::ios_base::binary | std::ios_base::trunc);
     if (!file_.is_open()) {
         reset();
-        throw etl_error(errno2str(errno));
+        throw etl_error(safe_strerror(errno));
     }
 
     for (const auto& entry : entries) {
@@ -56,7 +57,7 @@
             !file_.write(byte_ptr_cast(entry.value.data()), static_cast<std::streamsize>(entry.value.size()))) {
             auto err{errno};
             reset();
-            throw etl_error(errno2str(err));
+            throw etl_error(safe_strerror(err));
         }
     }
 
@@ -69,7 +70,7 @@
     if (!file_.is_open()) {
         auto err{errno};
         reset();
-        throw etl_error(errno2str(err));
+        throw etl_error(safe_strerror(err));
     }
 }
 
@@ -90,7 +91,7 @@
         !file_.read(byte_ptr_cast(entry.value.data()), head.lengths[1])) {
         auto err{errno};
         reset();
-        throw etl_error(errno2str(err));
+        throw etl_error(safe_strerror(err));
     }
 
     return std::make_pair(entry, id_);

silkworm/node/etl/util.hpp

@@ -23,10 +23,6 @@
 
 namespace silkworm::etl {
 
-//! \brief Converts a system error code into its message
-//! \remarks Removes the deprecation strerror
-std::string errno2str(int err_code);
-
 class etl_error : public std::runtime_error {
   public:
     using std::runtime_error::runtime_error;

silkworm/sentry/discovery/disc_v4/ping/ping_check.cpp

@@ -33,7 +33,7 @@
 
 namespace silkworm::sentry::discovery::disc_v4::ping {
 
-static const std::chrono::hours kPongValidityPeriod{24};
+constexpr std::chrono::hours kPongValidityPeriod{24};
 
 static std::chrono::time_point<std::chrono::system_clock> pong_expiration(std::chrono::time_point<std::chrono::system_clock> last_pong_time) {
     return last_pong_time + kPongValidityPeriod;

silkworm/sentry/rlpx/peer.cpp

@@ -85,8 +85,8 @@ static bool is_fatal_network_error(const boost::system::system_error& ex) {
            (code == boost::asio::error::broken_pipe);
 }
 
-static const std::chrono::milliseconds kPeerDisconnectTimeout = 2s;
-static const std::chrono::milliseconds kPeerPingInterval = 15s;
+constexpr std::chrono::milliseconds kPeerDisconnectTimeout = 2s;
+constexpr std::chrono::milliseconds kPeerPingInterval = 15s;
 
 class PingTimeoutError : public std::runtime_error {
   public: