fix: coverity warnings (#3632)

* fix: uninitialized field natpmp

* fix: unchecked downcast result

* fix: potential tr_error leak

* fix: potential tr_error leak

* fix: potential tr_error leak

* fix: potential nullptr dereference in test fixtures

* fix: handshake fields not initialized in constructor

* chore: cast unchecked return value to void

* chore: cast unchecked return value to void

* chore: cast unchecked return value to void

* chore: make single-arg constructors explicit

* fix: code smell make variable a pointer-to-const

* fix: code smell make variable a pointer-to-const

* chore: make single-arg constructors explicit

* chore: silence sonarcloud false warning on commented-out-code

* chore: fix code smell use std::array

* chore: make EvTimer::handleTimer() const

* chore: mark tr_bandwidth remove_child as noexcept

gtk/Application.cc

@@ -630,13 +630,9 @@ namespace
 
 std::string get_application_id(std::string const& config_dir)
 {
-    struct stat sb;
-    ::stat(config_dir.c_str(), &sb);
-
-    std::ostringstream id;
-    id << "com.transmissionbt.transmission_" << sb.st_dev << '_' << sb.st_ino;
-
-    return id.str();
+    struct stat sb = {};
+    (void)::stat(config_dir.c_str(), &sb);
+    return fmt::format("com.transmissionbt.transmission_{}_{}", sb.st_dev, sb.st_ino);
 }
 
 } // namespace

gtk/FaviconCache.cc

@@ -69,7 +69,7 @@ Glib::RefPtr<Gdk::Pixbuf> favicon_load_from_cache(std::string const& host)
     }
     catch (Glib::Error const&)
     {
-        g_remove(filename.c_str());
+        (void)g_remove(filename.c_str());
         return {};
     }
 }

libtransmission/.clang-tidy

@@ -20,6 +20,7 @@ Checks: >
   cppcoreguidelines-slicing,
   cppcoreguidelines-special-member-functions,
   cppcoreguidelines-virtual-class-destructor,
+  google-explicit-constructor,
   misc-*,
   -misc-no-recursion,
   -misc-non-private-member-variables-in-classes,

libtransmission/bandwidth.cc

@@ -90,18 +90,15 @@ tr_bandwidth::tr_bandwidth(tr_bandwidth* parent)
 ****
 ***/
 
-static void remove_child(std::vector<tr_bandwidth*>& v, tr_bandwidth* remove_me)
+static void remove_child(std::vector<tr_bandwidth*>& v, tr_bandwidth* remove_me) noexcept
 {
-    auto it = std::find(std::begin(v), std::end(v), remove_me);
-    if (it == std::end(v))
-    {
-        return;
-    }
-
     // the list isn't sorted -- so instead of erase()ing `it`,
     // do the cheaper option of overwriting it with the final item
-    *it = v.back();
-    v.resize(v.size() - 1);
+    if (auto it = std::find(std::begin(v), std::end(v), remove_me); it != std::end(v))
+    {
+        *it = v.back();
+        v.resize(v.size() - 1);
+    }
 }
 
 void tr_bandwidth::setParent(tr_bandwidth* new_parent)

libtransmission/file-posix.cc

@@ -358,7 +358,7 @@ std::string tr_sys_path_resolve(std::string_view path, tr_error** error)
     auto const szpath = tr_pathbuf{ path };
     auto buf = std::array<char, PATH_MAX>{};
 
-    if (auto* const ret = realpath(szpath, std::data(buf)); ret != nullptr)
+    if (auto const* const ret = realpath(szpath, std::data(buf)); ret != nullptr)
     {
         return ret;
     }
@@ -1192,7 +1192,7 @@ std::string tr_sys_dir_get_current(tr_error** error)
 
     for (;;)
     {
-        if (char* const ret = getcwd(std::data(buf), std::size(buf)); ret != nullptr)
+        if (char const* const ret = getcwd(std::data(buf), std::size(buf)); ret != nullptr)
         {
             return ret;
         }

libtransmission/handshake.cc

@@ -113,9 +113,10 @@ enum handshake_state_t
 
 struct tr_handshake
 {
-    tr_handshake(std::shared_ptr<tr_handshake_mediator> mediator_in)
+    tr_handshake(std::shared_ptr<tr_handshake_mediator> mediator_in, tr_encryption_mode encryption_mode_in)
         : mediator{ std::move(mediator_in) }
         , dh{ mediator->privateKey() }
+        , encryption_mode{ encryption_mode_in }
     {
     }
 
@@ -143,7 +144,7 @@ struct tr_handshake
     bool haveSentBitTorrentHandshake = false;
     tr_peerIo* io = nullptr;
     DH dh = {};
-    handshake_state_t state;
+    handshake_state_t state = AWAITING_HANDSHAKE;
     tr_encryption_mode encryption_mode;
     uint16_t pad_c_len = {};
     uint16_t pad_d_len = {};
@@ -1137,9 +1138,8 @@ tr_handshake* tr_handshakeNew(
     tr_handshake_done_func done_func,
     void* done_func_user_data)
 {
-    auto* const handshake = new tr_handshake{ std::move(mediator) };
+    auto* const handshake = new tr_handshake{ std::move(mediator), encryption_mode };
     handshake->io = io;
-    handshake->encryption_mode = encryption_mode;
     handshake->done_func = done_func;
     handshake->done_func_user_data = done_func_user_data;
     handshake->timeout_timer = handshake->mediator->createTimer();

libtransmission/makemeta.cc

@@ -66,7 +66,7 @@ void walkTree(std::string_view const top, std::string_view const subpath, std::s
     tr_sys_path_native_separators(std::data(path));
     tr_error* error = nullptr;
     auto const info = tr_sys_path_get_info(path, 0, &error);
-    if (!info)
+    if (error != nullptr)
     {
         tr_logAddWarn(fmt::format(
             _("Skipping '{path}': {error} ({error_code})"),
@@ -74,6 +74,9 @@ void walkTree(std::string_view const top, std::string_view const subpath, std::s
             fmt::arg("error", error->message),
             fmt::arg("error_code", error->code)));
         tr_error_free(error);
+    }
+    if (!info)
+    {
         return;
     }
 

libtransmission/natpmp_local.h

@@ -56,7 +56,7 @@ private:
 
     void setCommandTime();
 
-    natpmp_t natpmp_;
+    natpmp_t natpmp_ = {};
 
     tr_port public_port_ = {};
     tr_port private_port_ = {};

libtransmission/peer-mgr.cc

@@ -84,7 +84,7 @@ private:
     }
 
 public:
-    tr_handshake_mediator_impl(tr_session& session)
+    explicit tr_handshake_mediator_impl(tr_session& session)
         : session_{ session }
     {
     }

libtransmission/port-forwarding.cc

@@ -24,7 +24,7 @@ using namespace std::literals;
 
 struct tr_shared
 {
-    tr_shared(tr_session& session_in)
+    explicit tr_shared(tr_session& session_in)
         : session{ session_in }
     {
     }

libtransmission/rpc-server.cc

@@ -109,7 +109,7 @@ static bool constexpr tr_rpc_address_is_valid(tr_rpc_address const& a)
 ****
 ***/
 
-static char const* get_current_session_id(tr_rpc_server* server)
+static char const* get_current_session_id(tr_rpc_server const* server)
 {
     return server->session->session_id.c_str();
 }

libtransmission/session-id.h

@@ -17,7 +17,7 @@ class tr_session_id
 public:
     using current_time_func_t = time_t (*)();
 
-    tr_session_id(current_time_func_t get_current_time)
+    explicit tr_session_id(current_time_func_t get_current_time)
         : get_current_time_{ get_current_time }
     {
     }
@@ -46,7 +46,7 @@ private:
     static auto constexpr SessionIdSize = size_t{ 48 };
     static auto constexpr SessionIdDurationSec = time_t{ 60 * 60 }; /* expire in an hour */
 
-    using session_id_t = std::array<char, SessionIdSize + 1>; // +1 for '\0';
+    using session_id_t = std::array<char, SessionIdSize + 1>; // add one for '\0'
     static session_id_t make_session_id();
 
     current_time_func_t const get_current_time_;

libtransmission/session.h

@@ -86,7 +86,7 @@ struct tr_bindinfo
 struct tr_turtle_info
 {
     /* TR_UP and TR_DOWN speed limits */
-    unsigned int speedLimit_Bps[2] = {};
+    std::array<unsigned int, 2> speedLimit_Bps = {};
 
     /* is turtle mode on right now? */
     bool isEnabled = false;

libtransmission/timer-ev.cc

@@ -19,7 +19,7 @@ namespace libtransmission
 class EvTimer final : public Timer
 {
 public:
-    EvTimer(struct event_base* base)
+    explicit EvTimer(struct event_base* base)
         : base_{ base }
     {
         setRepeating(is_repeating_);
@@ -106,7 +106,7 @@ private:
         static_cast<EvTimer*>(vself)->handleTimer();
     }
 
-    void handleTimer()
+    void handleTimer() const
     {
         TR_ASSERT(callback_);
         callback_();

libtransmission/utils.cc

@@ -103,7 +103,7 @@ bool tr_loadFile(std::string_view filename, std::vector<char>& contents, tr_erro
     /* try to stat the file */
     tr_error* my_error = nullptr;
     auto const info = tr_sys_path_get_info(szfilename, 0, &my_error);
-    if (!info)
+    if (my_error != nullptr)
     {
         tr_logAddError(fmt::format(
             _("Couldn't read '{path}': {error} ({error_code})"),
@@ -114,7 +114,7 @@ bool tr_loadFile(std::string_view filename, std::vector<char>& contents, tr_erro
         return false;
     }
 
-    if (!info->isFile())
+    if (!info || !info->isFile())
     {
         tr_logAddError(fmt::format(_("Couldn't read '{path}': Not a regular file"), fmt::arg("path", filename)));
         tr_error_set(error, TR_ERROR_EISDIR, "Not a regular file"sv);

tests/libtransmission/announce-list-test.cc

@@ -385,7 +385,7 @@ TEST_F(AnnounceListTest, save)
         std::end(modified_tm.announceList())));
 
     // cleanup
-    std::remove(test_file.c_str());
+    (void)std::remove(test_file.c_str());
 }
 
 TEST_F(AnnounceListTest, SingleAnnounce)

tests/libtransmission/test-fixtures.h

@@ -127,15 +127,13 @@ protected:
         }
 
         tr_error* error = nullptr;
-
-        if (auto path = tr_sys_dir_get_current(&error); !std::empty(path))
+        auto path = tr_sys_dir_get_current(&error);
+        if (error != nullptr)
         {
-            return path;
+            std::cerr << "tr_sys_dir_get_current error: '" << error->message << "'" << std::endl;
+            tr_error_free(error);
         }
-
-        std::cerr << "tr_sys_dir_get_current error: '" << error->message << "'" << std::endl;
-        tr_error_free(error);
-        return {};
+        return path;
     }
 
     static std::string create_sandbox(std::string const& parent_dir, std::string const& tmpl)
@@ -205,7 +203,7 @@ protected:
         {
             uint64_t n = {};
             tr_error* local_error = nullptr;
-            if (!tr_sys_file_write(fd, left, n_left, &n, error))
+            if (!tr_sys_file_write(fd, left, n_left, &n, &local_error))
             {
                 fprintf(stderr, "Error writing file: '%s'\n", local_error->message);
                 tr_error_propagate(error, &local_error);

tests/libtransmission/watchdir-test.cc

@@ -78,7 +78,12 @@ protected:
         auto watchdir = force_generic ?
             Watchdir::createGeneric(path, std::move(callback), *timer_maker_, GenericRescanInterval) :
             Watchdir::create(path, std::move(callback), *timer_maker_, ev_base_.get());
-        dynamic_cast<impl::BaseWatchdir*>(watchdir.get())->setRetryDuration(RetryDuration);
+
+        if (auto* const base_watchdir = dynamic_cast<impl::BaseWatchdir*>(watchdir.get()); base_watchdir != nullptr)
+        {
+            base_watchdir->setRetryDuration(RetryDuration);
+        }
+
         return watchdir;
     }
 
@@ -230,7 +235,9 @@ TEST_P(WatchDirTest, retry)
     auto watchdir = createWatchDir(path, callback);
     auto constexpr FastRetryWaitTime = 20ms;
     auto constexpr ThreeRetries = FastRetryWaitTime * 4;
-    dynamic_cast<impl::BaseWatchdir*>(watchdir.get())->setRetryDuration(FastRetryWaitTime);
+    auto* const base_watchdir = dynamic_cast<impl::BaseWatchdir*>(watchdir.get());
+    ASSERT_TRUE(base_watchdir != nullptr);
+    base_watchdir->setRetryDuration(FastRetryWaitTime);
 
     processEvents(ThreeRetries);
     EXPECT_EQ(0U, std::size(names));

utils/create.cc

@@ -129,14 +129,12 @@ int parseCommandLine(app_options& options, int argc, char const* const* argv)
 std::string tr_getcwd()
 {
     tr_error* error = nullptr;
-    if (auto cur = tr_sys_dir_get_current(&error); !std::empty(cur))
+    auto cur = tr_sys_dir_get_current(&error);
     {
-        return cur;
+        fprintf(stderr, "getcwd error: \"%s\"", error->message);
+        tr_error_free(error);
     }
-
-    fprintf(stderr, "getcwd error: \"%s\"", error->message);
-    tr_error_free(error);
-    return "";
+    return cur;
 }
 
 } // namespace