commit db024adc90069ce9961f3993aba1b7372f09d77a Author: Nick Mathewson nickm@torproject.org Date: Mon Dec 4 15:09:18 2017 -0500

Switch to a safer FREE_AND_NULL implementation

This one only evaluates the input once, so it cannot mess up even if there are side effects. --- src/common/address.h | 2 +- src/common/aes.h | 3 ++- src/common/compat_libevent.h | 3 ++- src/common/compress.h | 3 ++- src/common/compress_lzma.h | 4 +++- src/common/compress_zlib.h | 4 +++- src/common/compress_zstd.h | 4 +++- src/common/log.c | 2 +- src/common/timers.h | 2 +- src/common/util.h | 21 ++++++++++++++++----- src/or/hs_service.c | 2 +- src/or/hs_service.h | 9 ++++++--- src/or/networkstatus.h | 3 ++- src/or/policies.h | 3 ++- src/or/rendcache.c | 6 +++--- src/or/rendcache.h | 10 ++++++---- src/or/rendservice.c | 6 +++--- src/or/router.h | 3 ++- src/or/routerlist.c | 2 +- src/or/shared_random_state.c | 2 +- src/test/test.c | 2 +- src/test/test_dir.c | 2 +- 22 files changed, 63 insertions(+), 35 deletions(-)

diff --git a/src/common/address.h b/src/common/address.h index 84d136d67..6f59e1c96 100644 --- a/src/common/address.h +++ b/src/common/address.h @@ -208,7 +208,7 @@ MOCK_DECL(int,get_interface_address6,(int severity, sa_family_t family, tor_addr_t *addr)); void interface_address6_list_free_(smartlist_t * addrs);// XXXX #define interface_address6_list_free(addrs) \ - FREE_AND_NULL(interface_address6_list, (addrs)) + FREE_AND_NULL_UNMATCHED(smartlist_t, interface_address6_list_free_, (addrs)) MOCK_DECL(smartlist_t *,get_interface_address6_list,(int severity, sa_family_t family, int include_internal)); diff --git a/src/common/aes.h b/src/common/aes.h index 4874f728d..c2720d29b 100644 --- a/src/common/aes.h +++ b/src/common/aes.h @@ -18,7 +18,8 @@ typedef struct aes_cnt_cipher aes_cnt_cipher_t; aes_cnt_cipher_t* aes_new_cipher(const uint8_t *key, const uint8_t *iv, int key_bits); void aes_cipher_free_(aes_cnt_cipher_t *cipher); -#define aes_cipher_free(cipher) FREE_AND_NULL(aes_cipher, (cipher)) +#define aes_cipher_free(cipher) \ + FREE_AND_NULL_UNMATCHED(aes_cnt_cipher_t, aes_cipher_free_, (cipher)) void aes_crypt_inplace(aes_cnt_cipher_t *cipher, char *data, size_t len);

int evaluate_evp_for_aes(int force_value); diff --git a/src/common/compat_libevent.h b/src/common/compat_libevent.h index 65807315c..2c86194d0 100644 --- a/src/common/compat_libevent.h +++ b/src/common/compat_libevent.h @@ -20,7 +20,8 @@ void suppress_libevent_log_msg(const char *msg); (sock),(tcp),(cb),(data));

void tor_event_free_(struct event *ev); -#define tor_event_free(ev) FREE_AND_NULL(tor_event, (ev)) +#define tor_event_free(ev) \ + FREE_AND_NULL_UNMATCHED(struct event, tor_event_free_, (ev))

typedef struct periodic_timer_t periodic_timer_t;

diff --git a/src/common/compress.h b/src/common/compress.h index eee4a315d..7420f169f 100644 --- a/src/common/compress.h +++ b/src/common/compress.h @@ -81,7 +81,8 @@ tor_compress_output_t tor_compress_process(tor_compress_state_t *state, const char **in, size_t *in_len, int finish); void tor_compress_free_(tor_compress_state_t *state); -#define tor_compress_free(st) FREE_AND_NULL(tor_compress, (st)) +#define tor_compress_free(st) \ + FREE_AND_NULL_UNMATCHED(tor_compress_state_t, tor_compress_free_, (st))

size_t tor_compress_state_size(const tor_compress_state_t *state);

diff --git a/src/common/compress_lzma.h b/src/common/compress_lzma.h index 1e92fd660..986bfe9a6 100644 --- a/src/common/compress_lzma.h +++ b/src/common/compress_lzma.h @@ -32,7 +32,9 @@ tor_lzma_compress_process(tor_lzma_compress_state_t *state, int finish);

void tor_lzma_compress_free_(tor_lzma_compress_state_t *state); -#define tor_lzma_compress_free(st) FREE_AND_NULL(tor_lzma_compress, (st)) +#define tor_lzma_compress_free(st) \ + FREE_AND_NULL_UNMATCHED(tor_lzma_compress_state_t, \ + tor_lzma_compress_free_, (st))

size_t tor_lzma_compress_state_size(const tor_lzma_compress_state_t *state);

diff --git a/src/common/compress_zlib.h b/src/common/compress_zlib.h index 3377e0e8d..701e2f9e8 100644 --- a/src/common/compress_zlib.h +++ b/src/common/compress_zlib.h @@ -32,7 +32,9 @@ tor_zlib_compress_process(tor_zlib_compress_state_t *state, int finish);

void tor_zlib_compress_free_(tor_zlib_compress_state_t *state); -#define tor_zlib_compress_free(st) FREE_AND_NULL(tor_zlib_compress, (st)) +#define tor_zlib_compress_free(st) \ + FREE_AND_NULL_UNMATCHED(tor_zlib_compress_state_t, \ + tor_zlib_compress_free_, (st))

size_t tor_zlib_compress_state_size(const tor_zlib_compress_state_t *state);

diff --git a/src/common/compress_zstd.h b/src/common/compress_zstd.h index 9f67a9c58..3e18febb1 100644 --- a/src/common/compress_zstd.h +++ b/src/common/compress_zstd.h @@ -32,7 +32,9 @@ tor_zstd_compress_process(tor_zstd_compress_state_t *state, int finish);

void tor_zstd_compress_free_(tor_zstd_compress_state_t *state); -#define tor_zstd_compress_free(st) FREE_AND_NULL(tor_zstd_compress, (st)) +#define tor_zstd_compress_free(st) \ + FREE_AND_NULL_UNMATCHED(tor_zstd_compress_state_t, \ + tor_zstd_compress_free_, (st))

size_t tor_zstd_compress_state_size(const tor_zstd_compress_state_t *state);

diff --git a/src/common/log.c b/src/common/log.c index 83098b101..d305c9ae5 100644 --- a/src/common/log.c +++ b/src/common/log.c @@ -65,7 +65,7 @@ typedef struct logfile_t {

static void log_free_(logfile_t *victim); #define log_free(lg) \ - FREE_AND_NULL(log, (lg)) + FREE_AND_NULL_UNMATCHED(logfile_t, log_free_, (lg))

/** Helper: map a log severity to descriptive string. */ static inline const char * diff --git a/src/common/timers.h b/src/common/timers.h index 6c9594d31..86e892016 100644 --- a/src/common/timers.h +++ b/src/common/timers.h @@ -18,7 +18,7 @@ void timer_get_cb(const tor_timer_t *t, void timer_schedule(tor_timer_t *t, const struct timeval *delay); void timer_disable(tor_timer_t *t); void timer_free_(tor_timer_t *t); -#define timer_free(t) FREE_AND_NULL(timer, (t)) +#define timer_free(t) FREE_AND_NULL_UNMATCHED(tor_timer_t, timer_free_, (t))

void timers_initialize(void); void timers_shutdown(void); diff --git a/src/common/util.h b/src/common/util.h index c5bd3f0bd..9ed11260d 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -109,13 +109,24 @@ extern int dmalloc_free(const char *file, const int line, void *pnt,

void tor_log_mallinfo(int severity);

+/* Helper macro: free a variable of type 'typename' using freefn, and + * set the variable to NULL. + * + * We use this for legacy cases when freefn and typename don't line up + * perfectly. + */ +#define FREE_AND_NULL_UNMATCHED(typename, freefn, var) \ + do { \ + /* only evaluate (var) once. */ \ + typename **tmp__free__ptr ## freefn = &(var); \ + freefn(*tmp__free__ptr ## freefn); \ + (*tmp__free__ptr ## freefn) = NULL; \ + } while (0) + /* Helper macro: free a variable of type 'type' using type_free_, and * set the variable to NULL. */ -#define FREE_AND_NULL(type, var) \ - do { \ - type ## _free_(var); \ - (var) = NULL; \ - } while (0) +#define FREE_AND_NULL(type, var) \ + FREE_AND_NULL_UNMATCHED(type ## _t, type ## _free_, (var))

/** Macro: yield a pointer to the field at position <b>off</b> within the * structure <b>st</b>. Example: diff --git a/src/or/hs_service.c b/src/or/hs_service.c index e88fb2389..1f93c2d52 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -370,7 +370,7 @@ service_intro_point_free_(hs_service_intro_point_t *ip) static void service_intro_point_free_void(void *obj) { - service_intro_point_free(obj); + service_intro_point_free_(obj); }

/* Return a newly allocated service intro point and fully initialized from the diff --git a/src/or/hs_service.h b/src/or/hs_service.h index b75903016..f933ba6ab 100644 --- a/src/or/hs_service.h +++ b/src/or/hs_service.h @@ -295,8 +295,9 @@ STATIC hs_service_intro_point_t *service_intro_point_new( const extend_info_t *ei, unsigned int is_legacy); STATIC void service_intro_point_free_(hs_service_intro_point_t *ip); -#define service_intro_point_free(ip) \ - FREE_AND_NULL(service_intro_point, (ip)) +#define service_intro_point_free(ip) \ + FREE_AND_NULL_UNMATCHED(hs_service_intro_point_t, \ + service_intro_point_free_, (ip)) STATIC void service_intro_point_add(digest256map_t *map, hs_service_intro_point_t *ip); STATIC void service_intro_point_remove(const hs_service_t *service, @@ -330,7 +331,9 @@ STATIC char * encode_desc_rev_counter_for_state(const hs_service_descriptor_t *desc);

STATIC void service_descriptor_free_(hs_service_descriptor_t *desc); -#define service_descriptor_free(d) FREE_AND_NULL(service_descriptor, (d)) +#define service_descriptor_free(d) \ + FREE_AND_NULL_UNMATCHED(hs_service_descriptor_t, \ + service_descriptor_free_, (d))

STATIC uint64_t check_state_line_for_service_rev_counter(const char *state_line, diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h index f62b5d240..37350bb1b 100644 --- a/src/or/networkstatus.h +++ b/src/or/networkstatus.h @@ -21,7 +21,8 @@ int router_reload_consensus_networkstatus(void); void routerstatus_free_(routerstatus_t *rs); #define routerstatus_free(rs) FREE_AND_NULL(routerstatus, (rs)) void networkstatus_vote_free_(networkstatus_t *ns); -#define networkstatus_vote_free(ns) FREE_AND_NULL(networkstatus_vote, (ns)) +#define networkstatus_vote_free(ns) \ + FREE_AND_NULL_UNMATCHED(networkstatus_t, networkstatus_vote_free_, (ns)) networkstatus_voter_info_t *networkstatus_get_voter_by_id( networkstatus_t *vote, const char *identity); diff --git a/src/or/policies.h b/src/or/policies.h index 062f33018..451a7a7f3 100644 --- a/src/or/policies.h +++ b/src/or/policies.h @@ -116,7 +116,8 @@ int policy_write_item(char *buf, size_t buflen, const addr_policy_t *item, int format_for_desc);

void addr_policy_list_free_(smartlist_t *p); -#define addr_policy_list_free(lst) FREE_AND_NULL(addr_policy_list, (lst)) +#define addr_policy_list_free(lst) \ + FREE_AND_NULL_UNMATCHED(smartlist_t, addr_policy_list_free_, (lst)) void addr_policy_free_(addr_policy_t *p); #define addr_policy_free(p) FREE_AND_NULL(addr_policy, (p)) void policies_free_all(void); diff --git a/src/or/rendcache.c b/src/or/rendcache.c index fa454321d..6f56df671 100644 --- a/src/or/rendcache.c +++ b/src/or/rendcache.c @@ -131,7 +131,7 @@ rend_cache_failure_intro_entry_free_(rend_cache_failure_intro_t *entry) static void rend_cache_failure_intro_entry_free_void(void *entry) { - rend_cache_failure_intro_entry_free(entry); + rend_cache_failure_intro_entry_free_(entry); }

/** Allocate a rend cache failure intro object and return it. <b>failure</b> @@ -165,7 +165,7 @@ rend_cache_failure_entry_free_(rend_cache_failure_t *entry) STATIC void rend_cache_failure_entry_free_void(void *entry) { - rend_cache_failure_entry_free(entry); + rend_cache_failure_entry_free_(entry); }

/** Allocate a rend cache failure object and return it. This function can @@ -219,7 +219,7 @@ rend_cache_entry_free_(rend_cache_entry_t *e) static void rend_cache_entry_free_void(void *p) { - rend_cache_entry_free(p); + rend_cache_entry_free_(p); }

/** Free all storage held by the service descriptor cache. */ diff --git a/src/or/rendcache.h b/src/or/rendcache.h index 66b48e032..c1e9207a9 100644 --- a/src/or/rendcache.h +++ b/src/or/rendcache.h @@ -94,11 +94,13 @@ STATIC void rend_cache_entry_free_(rend_cache_entry_t *e); #define rend_cache_entry_free(e) FREE_AND_NULL(rend_cache_entry, (e)) STATIC void rend_cache_failure_intro_entry_free_(rend_cache_failure_intro_t *entry); -#define rend_cache_failure_intro_entry_free(e) \ - FREE_AND_NULL(rend_cache_failure_intro_entry, (e)) +#define rend_cache_failure_intro_entry_free(e) \ + FREE_AND_NULL_UNMATCHED(rend_cache_failure_intro_t, \ + rend_cache_failure_intro_entry_free_, (e)) STATIC void rend_cache_failure_entry_free_(rend_cache_failure_t *entry); -#define rend_cache_failure_entry_free(e) \ - FREE_AND_NULL(rend_cache_failure_entry, (e)) +#define rend_cache_failure_entry_free(e) \ + FREE_AND_NULL_UNMATCHED(rend_cache_failure_t, \ + rend_cache_failure_entry_free_, (e)) STATIC int cache_failure_intro_lookup(const uint8_t *identity, const char *service_id, rend_cache_failure_intro_t diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 568afe79b..3e318f73f 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -174,7 +174,7 @@ rend_authorized_client_free_(rend_authorized_client_t *client) static void rend_authorized_client_strmap_item_free(void *authorized_client) { - rend_authorized_client_free(authorized_client); + rend_authorized_client_free_(authorized_client); }

/** Release the storage held by <b>service</b>. @@ -3726,7 +3726,7 @@ upload_service_descriptor(rend_service_t *service) } /* Free memory for descriptors. */ for (i = 0; i < smartlist_len(descs); i++) - rend_encoded_v2_service_descriptor_free(smartlist_get(descs, i)); + rend_encoded_v2_service_descriptor_free_(smartlist_get(descs, i)); smartlist_clear(descs); /* Update next upload time. */ if (seconds_valid - REND_TIME_PERIOD_OVERLAPPING_V2_DESCS @@ -3757,7 +3757,7 @@ upload_service_descriptor(rend_service_t *service) } /* Free memory for descriptors. */ for (i = 0; i < smartlist_len(descs); i++) - rend_encoded_v2_service_descriptor_free(smartlist_get(descs, i)); + rend_encoded_v2_service_descriptor_free_(smartlist_get(descs, i)); smartlist_clear(descs); } } diff --git a/src/or/router.h b/src/or/router.h index cd953da7a..e282b9975 100644 --- a/src/or/router.h +++ b/src/or/router.h @@ -37,7 +37,8 @@ int get_onion_key_grace_period(void);

di_digest256_map_t *construct_ntor_key_map(void); void ntor_key_map_free_(di_digest256_map_t *map); -#define ntor_key_map_free(map) FREE_AND_NULL(ntor_key_map, (map)) +#define ntor_key_map_free(map) \ + FREE_AND_NULL_UNMATCHED(di_digest256_map_t, ntor_key_map_free_, (map))

int router_initialize_tls_context(void); int init_keys(void); diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 877479969..0236761eb 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -3268,7 +3268,7 @@ signed_descriptor_from_routerinfo(routerinfo_t *ri) static void extrainfo_free_void(void *e) { - extrainfo_free(e); + extrainfo_free_(e); }

/** Free all storage held by a routerlist <b>rl</b>. */ diff --git a/src/or/shared_random_state.c b/src/or/shared_random_state.c index ae904cfda..8c5a497bb 100644 --- a/src/or/shared_random_state.c +++ b/src/or/shared_random_state.c @@ -271,7 +271,7 @@ commit_add_to_state(sr_commit_t *commit, sr_state_t *state) static void commit_free_(void *p) { - sr_commit_free(p); + sr_commit_free_(p); }

/* Free a state that was allocated with state_new(). */ diff --git a/src/test/test.c b/src/test/test.c index 1c5b654df..c08967c02 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -616,7 +616,7 @@ test_rend_fns(void *arg) done: if (descs) { for (i = 0; i < smartlist_len(descs); i++) - rend_encoded_v2_service_descriptor_free(smartlist_get(descs, i)); + rend_encoded_v2_service_descriptor_free_(smartlist_get(descs, i)); smartlist_free(descs); } if (parsed) diff --git a/src/test/test_dir.c b/src/test/test_dir.c index 4cab307ce..35e4fb44c 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -567,7 +567,7 @@ test_dir_routerinfo_parsing(void *arg) static void routerinfo_free_wrapper_(void *arg) { - routerinfo_free(arg); + routerinfo_free_(arg); }

static void