[tor-commits] [tor/master] router: refactor router_build_fresh_descriptor() static function interfaces
asn at torproject.org
asn at torproject.org
Tue Mar 26 13:37:54 UTC 2019
commit f19b64dce90c082b0e19f059b94c2d42b015a956
Author: teor <teor at torproject.org>
Date: Thu Jan 10 19:47:24 2019 +1000
router: refactor router_build_fresh_descriptor() static function interfaces
Tidy the arguments and return values of these functions, and clean up their
memory management.
Preparation for testing 29017 and 20918.
---
src/feature/relay/router.c | 169 +++++++++++++++++++++++++++++----------------
src/feature/relay/router.h | 1 +
2 files changed, 112 insertions(+), 58 deletions(-)
diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c
index e57e9fb8d..d9242448c 100644
--- a/src/feature/relay/router.c
+++ b/src/feature/relay/router.c
@@ -152,6 +152,8 @@ routerinfo_err_to_string(int err)
return "Cannot generate descriptor";
case TOR_ROUTERINFO_ERROR_DESC_REBUILDING:
return "Descriptor still rebuilding - not ready yet";
+ case TOR_ROUTERINFO_ERROR_INTERNAL_BUG:
+ return "Internal bug, see logs for details";
}
log_warn(LD_BUG, "unknown routerinfo error %d - shouldn't happen", err);
@@ -1941,23 +1943,33 @@ get_my_declared_family(const or_options_t *options)
return result;
}
-/** Build a fresh routerinfo for this OR, without any of the fields that depend
- * on the corresponding extrainfo. Set r to the generated routerinfo.
- * Return 0 on success, -1 on temporary error. Caller is responsible for
- * freeing the generated routerinfo if 0 is returned.
+/** Allocate a fresh, unsigned routerinfo for this OR, without any of the
+ * fields that depend on the corresponding extrainfo.
+ *
+ * On success, set ri_out to the new routerinfo, and return 0.
+ * Caller is responsible for freeing the generated routerinfo.
+ *
+ * Returns a negative value and sets ri_out to NULL on temporary error.
*/
static int
-router_build_fresh_routerinfo(routerinfo_t **r)
+router_build_fresh_routerinfo(routerinfo_t **ri_out)
{
- routerinfo_t *ri;
+ routerinfo_t *ri = NULL;
uint32_t addr;
char platform[256];
int hibernating = we_are_hibernating();
const or_options_t *options = get_options();
+ int result = TOR_ROUTERINFO_ERROR_INTERNAL_BUG;
+
+ if (BUG(!ri_out)) {
+ result = TOR_ROUTERINFO_ERROR_INTERNAL_BUG;
+ goto err;
+ }
if (router_pick_published_address(options, &addr, 0) < 0) {
log_warn(LD_CONFIG, "Don't know my address while generating descriptor");
- return TOR_ROUTERINFO_ERROR_NO_EXT_ADDR;
+ result = TOR_ROUTERINFO_ERROR_NO_EXT_ADDR;
+ goto err;
}
/* Log a message if the address in the descriptor doesn't match the ORPort
@@ -2014,8 +2026,8 @@ router_build_fresh_routerinfo(routerinfo_t **r)
ri->identity_pkey = crypto_pk_dup_key(get_server_identity_key());
if (BUG(crypto_pk_get_digest(ri->identity_pkey,
ri->cache_info.identity_digest) < 0)) {
- routerinfo_free(ri);
- return TOR_ROUTERINFO_ERROR_DIGEST_FAILED;
+ result = TOR_ROUTERINFO_ERROR_DIGEST_FAILED;
+ goto err;
}
ri->cache_info.signing_key_cert =
tor_cert_dup(get_master_signing_key_cert());
@@ -2054,20 +2066,26 @@ router_build_fresh_routerinfo(routerinfo_t **r)
ri->declared_family = get_my_declared_family(options);
- *r = ri;
+ goto done;
+
+ err:
+ routerinfo_free(ri);
+ *ri_out = NULL;
+ return result;
+
+ done:
+ *ri_out = ri;
return 0;
}
-/** Build an extrainfo for this OR, based on the routerinfo ri. Set e to the
- * generated extrainfo. Return 0 on success, -1 on temporary error. Failure to
- * generate an extrainfo is not an error and is indicated by setting e to
- * NULL. Caller is responsible for freeing the generated extrainfo if 0 is
- * returned.
+/** Allocate and return an extrainfo for this OR, based on the routerinfo ri.
+ *
+ * Caller is responsible for freeing the generated extrainfo.
*/
-static int
-router_build_fresh_extrainfo(const routerinfo_t *ri, extrainfo_t **e)
+static extrainfo_t *
+router_build_fresh_extrainfo(const routerinfo_t *ri)
{
- extrainfo_t *ei;
+ extrainfo_t *ei = NULL;
/* Now generate the extrainfo. */
ei = tor_malloc_zero(sizeof(extrainfo_t));
@@ -2079,24 +2097,23 @@ router_build_fresh_extrainfo(const routerinfo_t *ri, extrainfo_t **e)
memcpy(ei->cache_info.identity_digest, ri->cache_info.identity_digest,
DIGEST_LEN);
- *e = ei;
- return 0;
+
+ return ei;
}
/** Create a signed descriptor for ei, and add it to ei->cache_info.
- * Return ei on success, free ei and return NULL on temporary error.
- * Caller is responsible for freeing the returned extrainfo
- * (if it is not NULL), including any extra fields set in ei->cache_info.
+ *
+ * Return 0 on success, -1 on temporary error.
+ * On error, ei->cache_info is not modified.
*/
-static extrainfo_t *
+static int
router_update_extrainfo_descriptor_body(extrainfo_t *ei)
{
if (extrainfo_dump_to_string(&ei->cache_info.signed_descriptor_body,
ei, get_server_identity_key(),
get_master_signing_keypair()) < 0) {
log_warn(LD_BUG, "Couldn't generate extra-info descriptor.");
- extrainfo_free(ei);
- ei = NULL;
+ return -1;
} else {
ei->cache_info.signed_descriptor_len =
strlen(ei->cache_info.signed_descriptor_body);
@@ -2107,12 +2124,11 @@ router_update_extrainfo_descriptor_body(extrainfo_t *ei)
ei->cache_info.signed_descriptor_body,
ei->cache_info.signed_descriptor_len,
DIGEST_SHA256);
+ return 0;
}
-
- return ei;
}
-/** If ei is not NULL, set the fields in ri that depend on ei.
+/** Set the fields in ri that depend on ei.
*/
static void
router_update_routerinfo_from_extrainfo(routerinfo_t *ri,
@@ -2133,24 +2149,26 @@ router_update_routerinfo_from_extrainfo(routerinfo_t *ri,
}
/** Create a signed descriptor for ri, and add it to ri->cache_info.
- * Return 0 on success, free ri and ei and return -1 on temporary error.
- * TODO: freeing ri and ei, but leaving dangling pointers, is a bad interface.
- * Caller is responsible for freeing the generated ri and ei if 0 is returned,
- * including any extra fields set in ri->cache_info.
+ *
+ * Return 0 on success, and a negative value on temporary error.
+ * If ri is NULL, logs a BUG() warning and returns a negative value.
+ * On error, ri->cache_info is not modified.
*/
static int
-router_update_routerinfo_descriptor_body(routerinfo_t *ri, extrainfo_t *ei)
+router_update_routerinfo_descriptor_body(routerinfo_t *ri)
{
+ if (BUG(!ri))
+ return TOR_ROUTERINFO_ERROR_INTERNAL_BUG;
+
if (! (ri->cache_info.signed_descriptor_body =
router_dump_router_to_string(ri, get_server_identity_key(),
get_onion_key(),
get_current_curve25519_keypair(),
get_master_signing_keypair())) ) {
log_warn(LD_BUG, "Couldn't generate router descriptor.");
- routerinfo_free(ri);
- extrainfo_free(ei);
return TOR_ROUTERINFO_ERROR_CANNOT_GENERATE;
}
+
ri->cache_info.signed_descriptor_len =
strlen(ri->cache_info.signed_descriptor_body);
@@ -2200,40 +2218,63 @@ router_update_routerinfo_digest(routerinfo_t *ri)
ri->cache_info.signed_descriptor_digest);
}
-/** Build a fresh routerinfo, signed server descriptor, and extra-info document
- * for this OR. Set r to the generated routerinfo, e to the generated
- * extra-info document. Return 0 on success, -1 on temporary error. Failure to
- * generate an extra-info document is not an error and is indicated by setting
- * e to NULL. Caller is responsible for freeing generated documents if 0 is
- * returned.
+/** Build a fresh routerinfo, signed server descriptor, and signed extra-info
+ * document for this OR.
+ *
+ * Set r to the generated routerinfo, e to the generated extra-info document.
+ * Failure to generate an extra-info document is not an error and is indicated
+ * by setting e to NULL.
+ * Return 0 on success, and a negative value on temporary error.
+ * Caller is responsible for freeing generated documents on success.
*/
int
router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e)
{
- int result = -1;
+ int result = TOR_ROUTERINFO_ERROR_INTERNAL_BUG;
routerinfo_t *ri = NULL;
extrainfo_t *ei = NULL;
- /* TODO: return ri */
+ if (BUG(!r))
+ goto err;
+
+ if (BUG(!e))
+ goto err;
+
result = router_build_fresh_routerinfo(&ri);
- if (result < 0)
- return result;
+ if (result < 0) {
+ goto err;
+ }
+ /* If ri is NULL, then result should be negative. So this check should be
+ * unreachable. */
+ if (BUG(!ri)) {
+ result = TOR_ROUTERINFO_ERROR_INTERNAL_BUG;
+ goto err;
+ }
- /* TODO: return ei */
- result = router_build_fresh_extrainfo(ri, &ei);
- if (result < 0)
- return result;
+ ei = router_build_fresh_extrainfo(ri);
+ /* Failing to create an ei is not an error, but at this stage,
+ * router_build_fresh_extrainfo() should not fail. */
+ if (BUG(!ei))
+ goto skip_ei;
- /* TODO: this function frees ei on failure, instead, goto err */
- ei = router_update_extrainfo_descriptor_body(ei);
+ result = router_update_extrainfo_descriptor_body(ei);
+ if (result < 0)
+ goto skip_ei;
/* TODO: don't rely on tor_malloc_zero */
router_update_routerinfo_from_extrainfo(ri, ei);
- /* TODO: this function frees ri and ei on failure, instead, goto err */
- result = router_update_routerinfo_descriptor_body(ri, ei);
+ /* TODO: disentangle these GOTOs, or split into another function. */
+ goto ei_ok;
+
+ skip_ei:
+ extrainfo_free(ei);
+ ei = NULL;
+
+ ei_ok:
+ result = router_update_routerinfo_descriptor_body(ri);
if (result < 0)
- return result;
+ goto err;
/* TODO: fold into router_build_fresh_routerinfo() */
router_update_routerinfo_purpose(ri);
@@ -2246,11 +2287,23 @@ router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e)
router_update_routerinfo_digest(ri);
if (ei) {
- tor_assert(!
- routerinfo_incompatible_with_extrainfo(ri->identity_pkey, ei,
- &ri->cache_info, NULL));
+ if (BUG(routerinfo_incompatible_with_extrainfo(ri->identity_pkey, ei,
+ &ri->cache_info, NULL))) {
+ result = TOR_ROUTERINFO_ERROR_INTERNAL_BUG;
+ goto err;
+ }
}
+ goto done;
+
+ err:
+ routerinfo_free(ri);
+ extrainfo_free(ei);
+ *r = NULL;
+ *e = NULL;
+ return result;
+
+ done:
*r = ri;
*e = ei;
return 0;
diff --git a/src/feature/relay/router.h b/src/feature/relay/router.h
index 60bc857ce..46364206e 100644
--- a/src/feature/relay/router.h
+++ b/src/feature/relay/router.h
@@ -23,6 +23,7 @@ struct ed25519_keypair_t;
#define TOR_ROUTERINFO_ERROR_DIGEST_FAILED (-4)
#define TOR_ROUTERINFO_ERROR_CANNOT_GENERATE (-5)
#define TOR_ROUTERINFO_ERROR_DESC_REBUILDING (-6)
+#define TOR_ROUTERINFO_ERROR_INTERNAL_BUG (-7)
crypto_pk_t *get_onion_key(void);
time_t get_onion_key_set_at(void);
More information about the tor-commits
mailing list