From 0e321c2d216bfc6e3994eae6c798eb7e1ec60b82 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Sat, 25 Dec 2021 11:22:12 -0600 Subject: [PATCH] fix: clang warnings (#2339) * fix: comparison-is-always-false warning * fix: C/C++ mixing (memset() to clear a C++ object) --- libtransmission/crypto.cc | 25 +++++++++++-------------- libtransmission/crypto.h | 23 ++++++++++------------- libtransmission/interned-string.h | 12 +++++++++++- libtransmission/peer-io.cc | 8 +++----- libtransmission/peer-io.h | 15 +++++++++++---- tests/libtransmission/crypto-test.cc | 16 ++++------------ 6 files changed, 50 insertions(+), 49 deletions(-) diff --git a/libtransmission/crypto.cc b/libtransmission/crypto.cc index 37fb1482d..61d50a752 100644 --- a/libtransmission/crypto.cc +++ b/libtransmission/crypto.cc @@ -52,24 +52,21 @@ static void ensureKeyExists(tr_crypto* crypto) } } -void tr_cryptoConstruct(tr_crypto* crypto, tr_sha1_digest_t const* torrent_hash, bool isIncoming) +tr_crypto::tr_crypto(tr_sha1_digest_t const* torrent_hash_in, bool is_incoming_in) + : is_incoming{ is_incoming_in } { - memset(crypto, 0, sizeof(tr_crypto)); - - crypto->isIncoming = isIncoming; - - if (torrent_hash != nullptr) + if (torrent_hash_in != nullptr) { - tr_cryptoSetTorrentHash(crypto, *torrent_hash); + this->torrent_hash = *torrent_hash_in; } } -void tr_cryptoDestruct(tr_crypto* crypto) +tr_crypto::~tr_crypto() { - tr_dh_secret_free(crypto->mySecret); - tr_dh_free(crypto->dh); - tr_free(crypto->enc_key); - tr_free(crypto->dec_key); + tr_dh_secret_free(this->mySecret); + tr_dh_free(this->dh); + tr_free(this->enc_key); + tr_free(this->dec_key); } /** @@ -128,7 +125,7 @@ static void crypt_rc4(struct arc4_context* key, size_t buf_len, void const* buf_ void tr_cryptoDecryptInit(tr_crypto* crypto) { - init_rc4(crypto, &crypto->dec_key, crypto->isIncoming ? "keyA" : "keyB"); // lgtm[cpp/weak-cryptographic-algorithm] + init_rc4(crypto, &crypto->dec_key, crypto->is_incoming ? "keyA" : "keyB"); // lgtm[cpp/weak-cryptographic-algorithm] } void tr_cryptoDecrypt(tr_crypto* crypto, size_t buf_len, void const* buf_in, void* buf_out) @@ -138,7 +135,7 @@ void tr_cryptoDecrypt(tr_crypto* crypto, size_t buf_len, void const* buf_in, voi void tr_cryptoEncryptInit(tr_crypto* crypto) { - init_rc4(crypto, &crypto->enc_key, crypto->isIncoming ? "keyB" : "keyA"); // lgtm[cpp/weak-cryptographic-algorithm] + init_rc4(crypto, &crypto->enc_key, crypto->is_incoming ? "keyB" : "keyA"); // lgtm[cpp/weak-cryptographic-algorithm] } void tr_cryptoEncrypt(tr_crypto* crypto, size_t buf_len, void const* buf_in, void* buf_out) diff --git a/libtransmission/crypto.h b/libtransmission/crypto.h index afb1c7f7a..d29dafae1 100644 --- a/libtransmission/crypto.h +++ b/libtransmission/crypto.h @@ -35,21 +35,18 @@ enum /** @brief Holds state information for encrypted peer communications */ struct tr_crypto { - std::optional torrent_hash; - struct arc4_context* dec_key; - struct arc4_context* enc_key; - tr_dh_ctx_t dh; - uint8_t myPublicKey[KEY_LEN]; - tr_dh_secret_t mySecret; - bool isIncoming; + tr_crypto(tr_sha1_digest_t const* torrent_hash = nullptr, bool is_incoming = true); + ~tr_crypto(); + + std::optional torrent_hash = {}; + struct arc4_context* dec_key = nullptr; + struct arc4_context* enc_key = nullptr; + tr_dh_ctx_t dh = {}; + uint8_t myPublicKey[KEY_LEN] = {}; + tr_dh_secret_t mySecret = {}; + bool is_incoming = false; }; -/** @brief construct a new tr_crypto object */ -void tr_cryptoConstruct(tr_crypto* crypto, tr_sha1_digest_t const* torrent_hash, bool isIncoming); - -/** @brief destruct an existing tr_crypto object */ -void tr_cryptoDestruct(tr_crypto* crypto); - void tr_cryptoSetTorrentHash(tr_crypto* crypto, tr_sha1_digest_t const& torrent_hash); std::optional tr_cryptoGetTorrentHash(tr_crypto const* crypto); diff --git a/libtransmission/interned-string.h b/libtransmission/interned-string.h index 7c5405e3a..ecd59e66d 100644 --- a/libtransmission/interned-string.h +++ b/libtransmission/interned-string.h @@ -98,7 +98,17 @@ public: [[nodiscard]] auto compare(tr_interned_string const& that) const // <=> { - return this->quark() - that.quark(); + if (this->quark() < that.quark()) + { + return -1; + } + + if (this->quark() > that.quark()) + { + return 1; + } + + return 0; } [[nodiscard]] bool operator<(tr_interned_string const& that) const diff --git a/libtransmission/peer-io.cc b/libtransmission/peer-io.cc index 95d70944f..128d5ce02 100644 --- a/libtransmission/peer-io.cc +++ b/libtransmission/peer-io.cc @@ -610,7 +610,7 @@ static tr_peerIo* tr_peerIoNew( tr_address const* addr, tr_port port, tr_sha1_digest_t const* torrent_hash, - bool isIncoming, + bool is_incoming, bool isSeed, struct tr_peer_socket const socket) { @@ -630,8 +630,7 @@ static tr_peerIo* tr_peerIoNew( maybeSetCongestionAlgorithm(socket.handle.tcp, session->peerCongestionAlgorithm()); } - auto* io = new tr_peerIo{ session, *addr, port, isSeed }; - tr_cryptoConstruct(&io->crypto, torrent_hash, isIncoming); + auto* io = new tr_peerIo{ session, torrent_hash, is_incoming, *addr, port, isSeed }; io->socket = socket; io->bandwidth = new Bandwidth(parent); io->bandwidth->setPeer(io); @@ -653,7 +652,7 @@ static tr_peerIo* tr_peerIoNew( dbgmsg(io, "%s", "calling UTP_SetCallbacks &utp_function_table"); UTP_SetCallbacks(socket.handle.utp, &utp_function_table, io); - if (!isIncoming) + if (!is_incoming) { dbgmsg(io, "%s", "calling UTP_Connect"); UTP_Connect(socket.handle.utp); @@ -875,7 +874,6 @@ static void io_dtor(void* vio) event_disable(io, EV_READ | EV_WRITE); delete io->bandwidth; io_close_socket(io); - tr_cryptoDestruct(&io->crypto); while (io->outbuf_datatypes != nullptr) { diff --git a/libtransmission/peer-io.h b/libtransmission/peer-io.h index 6355d5c81..72ff7f1d5 100644 --- a/libtransmission/peer-io.h +++ b/libtransmission/peer-io.h @@ -67,8 +67,15 @@ auto inline constexpr PEER_IO_MAGIC_NUMBER = 206745; class tr_peerIo { public: - tr_peerIo(tr_session* session_in, tr_address const& addr_in, tr_port port_in, bool is_seed_in) - : addr{ addr_in } + tr_peerIo( + tr_session* session_in, + tr_sha1_digest_t const* torrent_hash, + bool is_incoming, + tr_address const& addr_in, + tr_port port_in, + bool is_seed_in) + : crypto{ torrent_hash, is_incoming } + , addr{ addr_in } , session{ session_in } , inbuf{ evbuffer_new() } , outbuf{ evbuffer_new() } @@ -83,7 +90,7 @@ public: evbuffer_free(inbuf); } - tr_crypto crypto = {}; + tr_crypto crypto; tr_address const addr; @@ -230,7 +237,7 @@ int tr_peerIoReconnect(tr_peerIo* io); constexpr bool tr_peerIoIsIncoming(tr_peerIo const* io) { - return io->crypto.isIncoming; + return io->crypto.is_incoming; } // TODO: remove this func; let caller get the current time instead diff --git a/tests/libtransmission/crypto-test.cc b/tests/libtransmission/crypto-test.cc index 3fbcace2f..7fea393d3 100644 --- a/tests/libtransmission/crypto-test.cc +++ b/tests/libtransmission/crypto-test.cc @@ -40,27 +40,22 @@ TEST(Crypto, torrentHash) { auto a = tr_crypto{}; - tr_cryptoConstruct(&a, nullptr, true); EXPECT_FALSE(tr_cryptoGetTorrentHash(&a)); tr_cryptoSetTorrentHash(&a, SomeHash); EXPECT_TRUE(tr_cryptoGetTorrentHash(&a)); EXPECT_EQ(SomeHash, *tr_cryptoGetTorrentHash(&a)); - tr_cryptoDestruct(&a); - tr_cryptoConstruct(&a, &SomeHash, false); + a = tr_crypto{ &SomeHash, false }; EXPECT_TRUE(tr_cryptoGetTorrentHash(&a)); EXPECT_EQ(SomeHash, *tr_cryptoGetTorrentHash(&a)); - - tr_cryptoDestruct(&a); } TEST(Crypto, encryptDecrypt) { - auto a = tr_crypto{}; - tr_cryptoConstruct(&a, &SomeHash, false); - auto b = tr_crypto_{}; - tr_cryptoConstruct_(&b, &SomeHash, true); + auto a = tr_crypto{ &SomeHash, false }; + auto b = tr_crypto_{ &SomeHash, true }; + auto public_key_length = int{}; EXPECT_TRUE(tr_cryptoComputeSecret(&a, tr_cryptoGetMyPublicKey_(&b, &public_key_length))); EXPECT_TRUE(tr_cryptoComputeSecret_(&b, tr_cryptoGetMyPublicKey(&a, &public_key_length))); @@ -84,9 +79,6 @@ TEST(Crypto, encryptDecrypt) tr_cryptoDecryptInit(&a); tr_cryptoDecrypt(&a, input2.size(), encrypted2.data(), decrypted2.data()); EXPECT_EQ(input2, std::string(decrypted2.data(), input2.size())); - - tr_cryptoDestruct_(&b); - tr_cryptoDestruct(&a); } TEST(Crypto, sha1)