commit 91c63aae8497bc9de6533daae8f927ca09f96fd2 Author: Nick Mathewson <nickm@torproject.org> Date: Wed Jan 24 12:33:13 2018 -0500 In relay_digest_matches(), use stack instead of heap. We'd been using crypto_digest_dup() and crypto_digest_assign() here, but they aren't necessary. Instead we can just use the stack to store the previous state of the SHA_CTX and avoid a malloc/free pair. Closes ticket 24914. --- changes/bug24914 | 3 +++ configure.ac | 4 ++++ src/common/crypto.c | 28 ++++++++++++++++++++++++++++ src/common/crypto.h | 11 +++++++++++ src/or/relay.c | 16 +++++++++------- 5 files changed, 55 insertions(+), 7 deletions(-) diff --git a/changes/bug24914 b/changes/bug24914 new file mode 100644 index 000000000..ea441fd38 --- /dev/null +++ b/changes/bug24914 @@ -0,0 +1,3 @@ + o Minor features (performance): + - Avoid a needless call to malloc() when processing an incoming + relay cell. Closes ticket 24914. diff --git a/configure.ac b/configure.ac index 3cb187b0e..0ba4c4652 100644 --- a/configure.ac +++ b/configure.ac @@ -834,6 +834,10 @@ AC_CHECK_MEMBERS([SSL.state], , , [#include <openssl/ssl.h> ]) +AC_CHECK_SIZEOF(SHA_CTX, , [AC_INCLUDES_DEFAULT() +#include <openssl/sha.h> +]) + dnl Define the set of checks for KIST scheduler support. AC_DEFUN([CHECK_KIST_SUPPORT],[ dnl KIST needs struct tcp_info and for certain members to exist. diff --git a/src/common/crypto.c b/src/common/crypto.c index 107b53ad2..880f92b4d 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -1822,6 +1822,7 @@ crypto_digest_alloc_bytes(digest_algorithm_t alg) /* Gives the length of crypto_digest_t through the end of the field 'd' */ #define END_OF_FIELD(f) (offsetof(crypto_digest_t, f) + \ STRUCT_FIELD_SIZE(crypto_digest_t, f)) + switch (alg) { case DIGEST_SHA1: return END_OF_FIELD(d.sha1); @@ -2007,6 +2008,33 @@ crypto_digest_dup(const crypto_digest_t *digest) return tor_memdup(digest, alloc_bytes); } +/** Temporarily save the state of <b>digest</b> in <b>checkpoint</b>. + * Asserts that <b>digest</b> is a SHA1 digest object. + */ +void +crypto_digest_checkpoint(crypto_digest_checkpoint_t *checkpoint, + const crypto_digest_t *digest) +{ + tor_assert(digest->algorithm == DIGEST_SHA1); + /* The optimizer should turn this into a constant... */ + const size_t bytes = crypto_digest_alloc_bytes(DIGEST_SHA1); + /* ... and remove this assertion entirely. */ + tor_assert(bytes <= sizeof(checkpoint->mem)); + memcpy(checkpoint->mem, digest, bytes); +} + +/** Restore the state of <b>digest</b> from <b>checkpoint</b>. + * Asserts that <b>digest</b> is a SHA1 digest object. Requires that the + * state was previously stored with crypto_digest_checkpoint() */ +void +crypto_digest_restore(crypto_digest_t *digest, + const crypto_digest_checkpoint_t *checkpoint) +{ + tor_assert(digest->algorithm == DIGEST_SHA1); + const size_t bytes = crypto_digest_alloc_bytes(DIGEST_SHA1); + memcpy(digest, checkpoint->mem, bytes); +} + /** Replace the state of the digest object <b>into</b> with the state * of the digest object <b>from</b>. Requires that 'into' and 'from' * have the same digest type. diff --git a/src/common/crypto.h b/src/common/crypto.h index 3caa23773..852ab9ba7 100644 --- a/src/common/crypto.h +++ b/src/common/crypto.h @@ -98,6 +98,13 @@ typedef struct crypto_digest_t crypto_digest_t; typedef struct crypto_xof_t crypto_xof_t; typedef struct crypto_dh_t crypto_dh_t; +#define DIGEST_CHECKPOINT_BYTES (SIZEOF_VOID_P + SIZEOF_SHA_CTX) +/** Structure used to temporarily save the a digest object. Only implemented + * for SHA1 digest for now. */ +typedef struct crypto_digest_checkpoint_t { + uint8_t mem[DIGEST_CHECKPOINT_BYTES]; +} crypto_digest_checkpoint_t; + /* global state */ int crypto_early_init(void) ATTR_WUR; int crypto_global_init(int hardwareAccel, @@ -235,6 +242,10 @@ void crypto_digest_add_bytes(crypto_digest_t *digest, const char *data, void crypto_digest_get_digest(crypto_digest_t *digest, char *out, size_t out_len); crypto_digest_t *crypto_digest_dup(const crypto_digest_t *digest); +void crypto_digest_checkpoint(crypto_digest_checkpoint_t *checkpoint, + const crypto_digest_t *digest); +void crypto_digest_restore(crypto_digest_t *digest, + const crypto_digest_checkpoint_t *checkpoint); void crypto_digest_assign(crypto_digest_t *into, const crypto_digest_t *from); void crypto_hmac_sha256(char *hmac_out, diff --git a/src/or/relay.c b/src/or/relay.c index b1b99526d..aa53cda77 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -151,9 +151,9 @@ relay_digest_matches(crypto_digest_t *digest, cell_t *cell) { uint32_t received_integrity, calculated_integrity; relay_header_t rh; - crypto_digest_t *backup_digest=NULL; + crypto_digest_checkpoint_t backup_digest; - backup_digest = crypto_digest_dup(digest); + crypto_digest_checkpoint(&backup_digest, digest); relay_header_unpack(&rh, cell->payload); memcpy(&received_integrity, rh.integrity, 4); @@ -167,19 +167,21 @@ relay_digest_matches(crypto_digest_t *digest, cell_t *cell) crypto_digest_add_bytes(digest, (char*) cell->payload, CELL_PAYLOAD_SIZE); crypto_digest_get_digest(digest, (char*) &calculated_integrity, 4); + int rv = 1; + if (calculated_integrity != received_integrity) { // log_fn(LOG_INFO,"Recognized=0 but bad digest. Not recognizing."); // (%d vs %d).", received_integrity, calculated_integrity); /* restore digest to its old form */ - crypto_digest_assign(digest, backup_digest); + crypto_digest_restore(digest, &backup_digest); /* restore the relay header */ memcpy(rh.integrity, &received_integrity, 4); relay_header_pack(cell->payload, &rh); - crypto_digest_free(backup_digest); - return 0; + rv = 0; } - crypto_digest_free(backup_digest); - return 1; + + memwipe(&backup_digest, 0, sizeof(backup_digest)); + return rv; } /** Apply <b>cipher</b> to CELL_PAYLOAD_SIZE bytes of <b>in</b>