From 2f10cb1730f646a6fc3c18d89eaeab1ed530d7dd Mon Sep 17 00:00:00 2001 From: iphydf Date: Mon, 7 Mar 2022 12:18:32 +0000 Subject: [PATCH] cleanup: Count re-adding an existing bootstrap node as success. It does nothing in the onion, but we shouldn't be reporting failure for it. Also added a bit more information to the `tox_bootstrap` logging. --- auto_tests/save_load_test.c | 2 +- toxcore/onion_client.c | 14 +++++-- toxcore/onion_client.h | 12 +++++- toxcore/tox.c | 80 +++++++++++++++++++------------------ toxcore/tox_test.cc | 16 ++++++++ 5 files changed, 80 insertions(+), 44 deletions(-) diff --git a/auto_tests/save_load_test.c b/auto_tests/save_load_test.c index 001b34e174..12fd36c5ef 100644 --- a/auto_tests/save_load_test.c +++ b/auto_tests/save_load_test.c @@ -27,7 +27,7 @@ #ifdef TCP_RELAY_PORT #undef TCP_RELAY_PORT #endif -#define TCP_RELAY_PORT 33430 +#define TCP_RELAY_PORT 33431 static void accept_friend_request(Tox *m, const uint8_t *public_key, const uint8_t *data, size_t length, void *userdata) { diff --git a/toxcore/onion_client.c b/toxcore/onion_client.c index 031aa6ca52..3fb0568228 100644 --- a/toxcore/onion_client.c +++ b/toxcore/onion_client.c @@ -150,8 +150,16 @@ Net_Crypto *onion_get_net_crypto(const Onion_Client *onion_c) /** @brief Add a node to the path_nodes bootstrap array. * - * return false on failure - * return true on success + * If a node with the given public key was already in the bootstrap array, this function has no + * effect and returns successfully. There is currently no way to update the IP/port for a bootstrap + * node, so if it changes, the Onion_Client must be recreated. + * + * @param onion_c The onion client object. + * @param ip_port IP/port for the bootstrap node. + * @param public_key DHT public key for the bootstrap node. + * + * @retval false on failure + * @retval true on success */ bool onion_add_bs_path_node(Onion_Client *onion_c, const IP_Port *ip_port, const uint8_t *public_key) { @@ -161,7 +169,7 @@ bool onion_add_bs_path_node(Onion_Client *onion_c, const IP_Port *ip_port, const for (unsigned int i = 0; i < MAX_PATH_NODES; ++i) { if (pk_equal(public_key, onion_c->path_nodes_bs[i].public_key)) { - return false; + return true; } } diff --git a/toxcore/onion_client.h b/toxcore/onion_client.h index 8c71f68bdf..d541982e2e 100644 --- a/toxcore/onion_client.h +++ b/toxcore/onion_client.h @@ -64,8 +64,16 @@ Net_Crypto *onion_get_net_crypto(const Onion_Client *onion_c); /** @brief Add a node to the path_nodes bootstrap array. * - * return false on failure - * return true on success + * If a node with the given public key was already in the bootstrap array, this function has no + * effect and returns successfully. There is currently no way to update the IP/port for a bootstrap + * node, so if it changes, the Onion_Client must be recreated. + * + * @param onion_c The onion client object. + * @param ip_port IP/port for the bootstrap node. + * @param public_key DHT public key for the bootstrap node. + * + * @retval false on failure + * @retval true on success */ non_null() bool onion_add_bs_path_node(Onion_Client *onion_c, const IP_Port *ip_port, const uint8_t *public_key); diff --git a/toxcore/tox.c b/toxcore/tox.c index 828b93d6e6..f17cc7395d 100644 --- a/toxcore/tox.c +++ b/toxcore/tox.c @@ -764,34 +764,49 @@ void tox_get_savedata(const Tox *tox, uint8_t *savedata) unlock(tox); } -bool tox_bootstrap(Tox *tox, const char *host, uint16_t port, const uint8_t *public_key, Tox_Err_Bootstrap *error) +non_null(5) nullable(1, 2, 4, 6) +static int32_t resolve_bootstrap_node(Tox *tox, const char *host, uint16_t port, const uint8_t *public_key, IP_Port **root, Tox_Err_Bootstrap *error) { assert(tox != nullptr); + assert(root != nullptr); if (host == nullptr || public_key == nullptr) { SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_NULL); - return false; + return -1; } if (port == 0) { SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_PORT); - return false; + return -1; } - IP_Port *root; - - const int32_t count = net_getipport(host, &root, TOX_SOCK_DGRAM); + const int32_t count = net_getipport(host, root, TOX_SOCK_DGRAM); if (count == -1) { LOGGER_DEBUG(tox->m->log, "could not resolve bootstrap node '%s'", host); - net_freeipport(root); + net_freeipport(*root); SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_HOST); + return -1; + } + + assert(*root != nullptr); + return count; +} + +bool tox_bootstrap(Tox *tox, const char *host, uint16_t port, const uint8_t *public_key, Tox_Err_Bootstrap *error) +{ + IP_Port *root; + const int32_t count = resolve_bootstrap_node(tox, host, port, public_key, &root, error); + + if (count == -1) { return false; } lock(tox); assert(count >= 0); - bool success = false; + bool onion_success = false; + // UDP bootstrap is default success if it's disabled (because we don't even try). + bool udp_success = tox->m->options.udp_disabled; for (int32_t i = 0; i < count; ++i) { root[i].port = net_htons(port); @@ -800,13 +815,13 @@ bool tox_bootstrap(Tox *tox, const char *host, uint16_t port, const uint8_t *pub // If UDP is enabled, the caller cares about whether any of the // bootstrap calls below will succeed. In TCP-only mode, adding // onion path nodes successfully is sufficient. - success = success || tox->m->options.udp_disabled; + onion_success = true; } if (!tox->m->options.udp_disabled) { if (dht_bootstrap(tox->m->dht, &root[i], public_key)) { // If any of the bootstrap calls worked, we call it success. - success = true; + udp_success = true; } } } @@ -815,45 +830,34 @@ bool tox_bootstrap(Tox *tox, const char *host, uint16_t port, const uint8_t *pub net_freeipport(root); - if (count > 0 && success) { - SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_OK); - return true; + if (count == 0 || !onion_success || !udp_success) { + LOGGER_DEBUG(tox->m->log, "bootstrap node '%s' resolved to %d IP_Ports%s (onion: %s, UDP: %s)", + host, count, + count > 0 ? ", but failed to bootstrap with any of them" : "", + onion_success ? "success" : "FAILURE", + tox->m->options.udp_disabled ? "disabled" : (udp_success ? "success" : "FAILURE")); + SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_HOST); + return false; } - LOGGER_DEBUG(tox->m->log, "bootstrap node '%s' resolved to %d IP_Ports%s", host, count, - count > 0 ? ", but failed to bootstrap with any of them" : ""); - SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_HOST); - return false; + SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_OK); + return true; } bool tox_add_tcp_relay(Tox *tox, const char *host, uint16_t port, const uint8_t *public_key, Tox_Err_Bootstrap *error) { - assert(tox != nullptr); - - if (host == nullptr || public_key == nullptr) { - SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_NULL); - return false; - } - - if (port == 0) { - SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_PORT); - return false; - } - IP_Port *root; - - const int32_t count = net_getipport(host, &root, TOX_SOCK_STREAM); + const int32_t count = resolve_bootstrap_node(tox, host, port, public_key, &root, error); if (count == -1) { - net_freeipport(root); - SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_HOST); return false; } lock(tox); assert(count >= 0); + LOGGER_DEBUG(tox->m->log, "count: %d", count); for (int32_t i = 0; i < count; ++i) { root[i].port = net_htons(port); @@ -864,13 +868,13 @@ bool tox_add_tcp_relay(Tox *tox, const char *host, uint16_t port, const uint8_t net_freeipport(root); - if (count > 0) { - SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_OK); - return true; + if (count == 0) { + SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_HOST); + return false; } - SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_HOST); - return false; + SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_OK); + return true; } Tox_Connection tox_self_get_connection_status(const Tox *tox) diff --git a/toxcore/tox_test.cc b/toxcore/tox_test.cc index 5e0e9f22cc..407198b723 100644 --- a/toxcore/tox_test.cc +++ b/toxcore/tox_test.cc @@ -17,6 +17,22 @@ static void set_random_name_and_status_message(Tox *tox, uint8_t *name, uint8_t } } +TEST(Tox, BootstrapErrorCodes) +{ + Tox *tox = tox_new(nullptr, nullptr); + ASSERT_NE(tox, nullptr); + + Tox_Err_Bootstrap err; + std::array pk; + tox_bootstrap(tox, "127.0.0.1", 0, pk.data(), &err); + EXPECT_EQ(err, TOX_ERR_BOOTSTRAP_BAD_PORT); + + tox_bootstrap(tox, nullptr, 33445, pk.data(), &err); + EXPECT_EQ(err, TOX_ERR_BOOTSTRAP_NULL); + + tox_kill(tox); +} + TEST(Tox, OneTest) { struct Tox_Options *options = tox_options_new(nullptr);