From 88ae190aca442caff1d2f0ff4078ea3872703a70 Mon Sep 17 00:00:00 2001 From: iphydf Date: Tue, 15 Mar 2022 21:19:43 +0000 Subject: [PATCH] cleanup: Split the huge TCP client packet handler. Also enable cppcheck in both C and C++ mode. --- other/analysis/run-cppcheck | 23 +- .../docker/tox-bootstrapd.sha256 | 2 +- toxcore/DHT.c | 14 +- toxcore/TCP_client.c | 268 ++++++++++-------- toxencryptsave/toxencryptsave.c | 8 +- 5 files changed, 175 insertions(+), 140 deletions(-) diff --git a/other/analysis/run-cppcheck b/other/analysis/run-cppcheck index c71d6e35..b9061094 100755 --- a/other/analysis/run-cppcheck +++ b/other/analysis/run-cppcheck @@ -6,29 +6,34 @@ SKIP_GTEST=1 set -e -CPPCHECK="--enable=all" +CPPCHECK=("--enable=all") CPPCHECK+=("--inconclusive") CPPCHECK+=("--error-exitcode=1") # Used for VLA. CPPCHECK+=("--suppress=allocaCalled") -# We actually write C code. -CPPCHECK+=("--suppress=cstyleCast") -# Used to enable/disable parts using if with constant condition. +# False positives in switch statements. CPPCHECK+=("--suppress=knownConditionTrueFalse") # Cppcheck does not need standard library headers to get proper results. CPPCHECK+=("--suppress=missingIncludeSystem") -# Range-for is fine. -CPPCHECK+=("--suppress=useStlAlgorithm") # TODO(iphydf): Maybe fix? -CPPCHECK+=("--suppress=shadowArgument") -CPPCHECK+=("--suppress=shadowFunction") CPPCHECK+=("--suppress=signConversion") # TODO(iphydf): Fixed in the toxav refactor PR. CPPCHECK+=("--suppress=redundantAssignment") +# We're a library. This only works on whole programs. +CPPCHECK_C=("--suppress=unusedFunction") + +# We actually write C code. +CPPCHECK_CXX=("--suppress=cstyleCast") +# False positive in auto_tests. +CPPCHECK_CXX+=("--suppress=shadowArgument") +CPPCHECK_CXX+=("--suppress=shadowFunction") +# Range-for is fine. +CPPCHECK_CXX+=("--suppress=useStlAlgorithm") run() { echo "Running cppcheck in variant '$*'" - cppcheck "${CPPCHECK[@]}" amalgamation.cc "${CPPFLAGS[@]}" "$@" + cppcheck "${CPPCHECK[@]}" "${CPPCHECK_C[@]}" tox*/*.[ch] tox*/*/*.[ch] "${CPPFLAGS[@]}" "$@" + cppcheck "${CPPCHECK[@]}" "${CPPCHECK_CXX[@]}" amalgamation.cc "${CPPFLAGS[@]}" "$@" } . other/analysis/variants.sh diff --git a/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 b/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 index 913ae88b..dc72e8dd 100644 --- a/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 +++ b/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 @@ -1 +1 @@ -73bc2fb0838ba342f92fa5e2c4a85989c61d04fd87a20db7edc300a9bb42781a /usr/local/bin/tox-bootstrapd +6a7b0559334d281aacd5ebc18ed5523da1ac49d9dd09d81a33166a598d4a97b5 /usr/local/bin/tox-bootstrapd diff --git a/toxcore/DHT.c b/toxcore/DHT.c index 06c1663f..e3f68d24 100644 --- a/toxcore/DHT.c +++ b/toxcore/DHT.c @@ -464,21 +464,21 @@ int pack_ip_port(const Logger *logger, uint8_t *data, uint16_t length, const IP_ } bool is_ipv4; - uint8_t net_family; + uint8_t family; if (net_family_is_ipv4(ip_port->ip.family)) { // TODO(irungentoo): use functions to convert endianness is_ipv4 = true; - net_family = TOX_AF_INET; + family = TOX_AF_INET; } else if (net_family_is_tcp_ipv4(ip_port->ip.family)) { is_ipv4 = true; - net_family = TOX_TCP_INET; + family = TOX_TCP_INET; } else if (net_family_is_ipv6(ip_port->ip.family)) { is_ipv4 = false; - net_family = TOX_AF_INET6; + family = TOX_AF_INET6; } else if (net_family_is_tcp_ipv6(ip_port->ip.family)) { is_ipv4 = false; - net_family = TOX_TCP_INET6; + family = TOX_TCP_INET6; } else { char ip_str[IP_NTOA_LEN]; // TODO(iphydf): Find out why we're trying to pack invalid IPs, stop @@ -494,7 +494,7 @@ int pack_ip_port(const Logger *logger, uint8_t *data, uint16_t length, const IP_ return -1; } - data[0] = net_family; + data[0] = family; memcpy(data + 1, &ip_port->ip.ip.v4, SIZE_IP4); memcpy(data + 1 + SIZE_IP4, &ip_port->port, sizeof(uint16_t)); return size; @@ -505,7 +505,7 @@ int pack_ip_port(const Logger *logger, uint8_t *data, uint16_t length, const IP_ return -1; } - data[0] = net_family; + data[0] = family; memcpy(data + 1, &ip_port->ip.ip.v6, SIZE_IP6); memcpy(data + 1 + SIZE_IP6, &ip_port->port, sizeof(uint16_t)); return size; diff --git a/toxcore/TCP_client.c b/toxcore/TCP_client.c index e243675e..74541256 100644 --- a/toxcore/TCP_client.c +++ b/toxcore/TCP_client.c @@ -614,6 +614,142 @@ TCP_Client_Connection *new_TCP_connection(const Logger *logger, const Mono_Time return temp; } +non_null() +static int handle_TCP_client_routing_response(TCP_Client_Connection *conn, const uint8_t *data, uint16_t length) +{ + if (length != 1 + 1 + CRYPTO_PUBLIC_KEY_SIZE) { + return -1; + } + + if (data[1] < NUM_RESERVED_PORTS) { + return 0; + } + + const uint8_t con_id = data[1] - NUM_RESERVED_PORTS; + + if (conn->connections[con_id].status != 0) { + return 0; + } + + conn->connections[con_id].status = 1; + conn->connections[con_id].number = -1; + memcpy(conn->connections[con_id].public_key, data + 2, CRYPTO_PUBLIC_KEY_SIZE); + + if (conn->response_callback != nullptr) { + conn->response_callback(conn->response_callback_object, con_id, conn->connections[con_id].public_key); + } + + return 0; +} + +non_null() +static int handle_TCP_client_connection_notification(TCP_Client_Connection *conn, const uint8_t *data, uint16_t length) +{ + if (length != 1 + 1) { + return -1; + } + + if (data[1] < NUM_RESERVED_PORTS) { + return -1; + } + + const uint8_t con_id = data[1] - NUM_RESERVED_PORTS; + + if (conn->connections[con_id].status != 1) { + return 0; + } + + conn->connections[con_id].status = 2; + + if (conn->status_callback != nullptr) { + conn->status_callback(conn->status_callback_object, conn->connections[con_id].number, con_id, + conn->connections[con_id].status); + } + + return 0; +} + +non_null() +static int handle_TCP_client_disconnect_notification(TCP_Client_Connection *conn, const uint8_t *data, uint16_t length) +{ + if (length != 1 + 1) { + return -1; + } + + if (data[1] < NUM_RESERVED_PORTS) { + return -1; + } + + const uint8_t con_id = data[1] - NUM_RESERVED_PORTS; + + if (conn->connections[con_id].status == 0) { + return 0; + } + + if (conn->connections[con_id].status != 2) { + return 0; + } + + conn->connections[con_id].status = 1; + + if (conn->status_callback != nullptr) { + conn->status_callback(conn->status_callback_object, conn->connections[con_id].number, con_id, + conn->connections[con_id].status); + } + + return 0; +} + +non_null() +static int handle_TCP_client_ping(const Logger *logger, TCP_Client_Connection *conn, const uint8_t *data, uint16_t length) +{ + if (length != 1 + sizeof(uint64_t)) { + return -1; + } + + uint64_t ping_id; + memcpy(&ping_id, data + 1, sizeof(uint64_t)); + conn->ping_response_id = ping_id; + tcp_send_ping_response(logger, conn); + return 0; +} + +non_null() +static int handle_TCP_client_pong(TCP_Client_Connection *conn, const uint8_t *data, uint16_t length) +{ + if (length != 1 + sizeof(uint64_t)) { + return -1; + } + + uint64_t ping_id; + memcpy(&ping_id, data + 1, sizeof(uint64_t)); + + if (ping_id != 0) { + if (ping_id == conn->ping_id) { + conn->ping_id = 0; + } + + return 0; + } + + return -1; +} + +non_null(1, 2) nullable(4) +static int handle_TCP_client_oob_recv(TCP_Client_Connection *conn, const uint8_t *data, uint16_t length, void *userdata) +{ + if (length <= 1 + CRYPTO_PUBLIC_KEY_SIZE) { + return -1; + } + + if (conn->oob_data_callback != nullptr) { + conn->oob_data_callback(conn->oob_data_callback_object, data + 1, data + 1 + CRYPTO_PUBLIC_KEY_SIZE, + length - (1 + CRYPTO_PUBLIC_KEY_SIZE), userdata); + } + + return 0; +} + /** * @retval 0 on success * @retval -1 on failure @@ -627,129 +763,23 @@ static int handle_TCP_client_packet(const Logger *logger, TCP_Client_Connection } switch (data[0]) { - case TCP_PACKET_ROUTING_RESPONSE: { - if (length != 1 + 1 + CRYPTO_PUBLIC_KEY_SIZE) { - return -1; - } + case TCP_PACKET_ROUTING_RESPONSE: + return handle_TCP_client_routing_response(conn, data, length); - if (data[1] < NUM_RESERVED_PORTS) { - return 0; - } + case TCP_PACKET_CONNECTION_NOTIFICATION: + return handle_TCP_client_connection_notification(conn, data, length); - const uint8_t con_id = data[1] - NUM_RESERVED_PORTS; + case TCP_PACKET_DISCONNECT_NOTIFICATION: + return handle_TCP_client_disconnect_notification(conn, data, length); - if (conn->connections[con_id].status != 0) { - return 0; - } + case TCP_PACKET_PING: + return handle_TCP_client_ping(logger, conn, data, length); - conn->connections[con_id].status = 1; - conn->connections[con_id].number = -1; - memcpy(conn->connections[con_id].public_key, data + 2, CRYPTO_PUBLIC_KEY_SIZE); + case TCP_PACKET_PONG: + return handle_TCP_client_pong(conn, data, length); - if (conn->response_callback != nullptr) { - conn->response_callback(conn->response_callback_object, con_id, conn->connections[con_id].public_key); - } - - return 0; - } - - case TCP_PACKET_CONNECTION_NOTIFICATION: { - if (length != 1 + 1) { - return -1; - } - - if (data[1] < NUM_RESERVED_PORTS) { - return -1; - } - - uint8_t con_id = data[1] - NUM_RESERVED_PORTS; - - if (conn->connections[con_id].status != 1) { - return 0; - } - - conn->connections[con_id].status = 2; - - if (conn->status_callback != nullptr) { - conn->status_callback(conn->status_callback_object, conn->connections[con_id].number, con_id, - conn->connections[con_id].status); - } - - return 0; - } - - case TCP_PACKET_DISCONNECT_NOTIFICATION: { - if (length != 1 + 1) { - return -1; - } - - if (data[1] < NUM_RESERVED_PORTS) { - return -1; - } - - uint8_t con_id = data[1] - NUM_RESERVED_PORTS; - - if (conn->connections[con_id].status == 0) { - return 0; - } - - if (conn->connections[con_id].status != 2) { - return 0; - } - - conn->connections[con_id].status = 1; - - if (conn->status_callback != nullptr) { - conn->status_callback(conn->status_callback_object, conn->connections[con_id].number, con_id, - conn->connections[con_id].status); - } - - return 0; - } - - case TCP_PACKET_PING: { - if (length != 1 + sizeof(uint64_t)) { - return -1; - } - - uint64_t ping_id; - memcpy(&ping_id, data + 1, sizeof(uint64_t)); - conn->ping_response_id = ping_id; - tcp_send_ping_response(logger, conn); - return 0; - } - - case TCP_PACKET_PONG: { - if (length != 1 + sizeof(uint64_t)) { - return -1; - } - - uint64_t ping_id; - memcpy(&ping_id, data + 1, sizeof(uint64_t)); - - if (ping_id != 0) { - if (ping_id == conn->ping_id) { - conn->ping_id = 0; - } - - return 0; - } - - return -1; - } - - case TCP_PACKET_OOB_RECV: { - if (length <= 1 + CRYPTO_PUBLIC_KEY_SIZE) { - return -1; - } - - if (conn->oob_data_callback != nullptr) { - conn->oob_data_callback(conn->oob_data_callback_object, data + 1, data + 1 + CRYPTO_PUBLIC_KEY_SIZE, - length - (1 + CRYPTO_PUBLIC_KEY_SIZE), userdata); - } - - return 0; - } + case TCP_PACKET_OOB_RECV: + return handle_TCP_client_oob_recv(conn, data, length, userdata); case TCP_PACKET_ONION_RESPONSE: { conn->onion_callback(conn->onion_callback_object, data + 1, length - 1, userdata); @@ -761,7 +791,7 @@ static int handle_TCP_client_packet(const Logger *logger, TCP_Client_Connection return -1; } - uint8_t con_id = data[0] - NUM_RESERVED_PORTS; + const uint8_t con_id = data[0] - NUM_RESERVED_PORTS; if (conn->data_callback != nullptr) { conn->data_callback(conn->data_callback_object, conn->connections[con_id].number, con_id, data + 1, length - 1, diff --git a/toxencryptsave/toxencryptsave.c b/toxencryptsave/toxencryptsave.c index aaa49160..baabb14f 100644 --- a/toxencryptsave/toxencryptsave.c +++ b/toxencryptsave/toxencryptsave.c @@ -234,10 +234,10 @@ bool tox_pass_encrypt(const uint8_t *plaintext, size_t plaintext_len, const uint * * returns true on success */ -bool tox_pass_key_decrypt(const Tox_Pass_Key *key, const uint8_t *ciphertext, size_t length, uint8_t *plaintext, - Tox_Err_Decryption *error) +bool tox_pass_key_decrypt(const Tox_Pass_Key *key, const uint8_t *ciphertext, size_t ciphertext_len, + uint8_t *plaintext, Tox_Err_Decryption *error) { - if (length <= TOX_PASS_ENCRYPTION_EXTRA_LENGTH) { + if (ciphertext_len <= TOX_PASS_ENCRYPTION_EXTRA_LENGTH) { SET_ERROR_PARAMETER(error, TOX_ERR_DECRYPTION_INVALID_LENGTH); return false; } @@ -255,7 +255,7 @@ bool tox_pass_key_decrypt(const Tox_Pass_Key *key, const uint8_t *ciphertext, si ciphertext += TOX_ENC_SAVE_MAGIC_LENGTH; ciphertext += crypto_pwhash_scryptsalsa208sha256_SALTBYTES; // salt only affects key derivation - const size_t decrypt_length = length - TOX_PASS_ENCRYPTION_EXTRA_LENGTH; + const size_t decrypt_length = ciphertext_len - TOX_PASS_ENCRYPTION_EXTRA_LENGTH; uint8_t nonce[crypto_box_NONCEBYTES]; memcpy(nonce, ciphertext, crypto_box_NONCEBYTES);