free() is specified to be NULL-safe, and I don't know of any implementations that violate this. Tor's *_free() functions conform, although relaycache_free() prints a warning (which I remove in the below diff).

I checked every *_free() function for NULL-safety before removing conditions for it. This includes an OpenSSL function or two.

This is only a first pass. I got bored, and I wanted to make sure that there's interest in committing this. :)

diff --git a/src/common/backtrace.c b/src/common/backtrace.c index a2d5378..0aa8a17 100644 --- a/src/common/backtrace.c +++ b/src/common/backtrace.c @@ -176,8 +176,7 @@ install_bt_handler(void) char **symbols; int depth = backtrace(cb_buf, MAX_DEPTH); symbols = backtrace_symbols(cb_buf, depth); - if (symbols) - free(symbols); + free(symbols); }

return rv; diff --git a/src/common/torgzip.c b/src/common/torgzip.c index 4f23407..3158b96 100644 --- a/src/common/torgzip.c +++ b/src/common/torgzip.c @@ -403,9 +403,7 @@ tor_gzip_uncompress(char **out, size_t *out_len, inflateEnd(stream); tor_free(stream); } - if (*out) { - tor_free(*out); - } + tor_free(*out); return -1; }

diff --git a/src/common/tortls.c b/src/common/tortls.c index 7447822..23b1dcf 100644 --- a/src/common/tortls.c +++ b/src/common/tortls.c @@ -625,10 +625,8 @@ tor_tls_create_certificate(crypto_pk_t *rsa,

goto done; error: - if (x509) { - X509_free(x509); - x509 = NULL; - } + X509_free(x509); + x509 = NULL; done: tls_log_errors(NULL, LOG_WARN, LD_NET, "generating certificate"); if (sign_pkey) @@ -722,8 +720,7 @@ tor_x509_cert_free(tor_x509_cert_t *cert) { if (! cert) return; - if (cert->cert) - X509_free(cert->cert); + X509_free(cert->cert); tor_free(cert->encoded); memwipe(cert, 0x03, sizeof(*cert)); tor_free(cert); @@ -1328,12 +1325,9 @@ tor_tls_context_new(crypto_pk_t *identity, unsigned int key_lifetime, crypto_pk_free(rsa_auth); if (result) tor_tls_context_decref(result); - if (cert) - X509_free(cert); - if (idcert) - X509_free(idcert); - if (authcert) - X509_free(authcert); + X509_free(cert); + X509_free(idcert); + X509_free(authcert); return NULL; }

@@ -2035,8 +2029,7 @@ tor_tls_finish_handshake(tor_tls_t *tls) "a v2 handshake on %p.", tls); tls->wasV2Handshake = 1; } - if (cert) - X509_free(cert); + X509_free(cert); #endif if (SSL_set_cipher_list(tls->ssl, SERVER_CIPHER_LIST) == 0) { tls_log_errors(NULL, LOG_WARN, LD_HANDSHAKE, "re-setting ciphers"); @@ -2319,8 +2312,7 @@ tor_tls_verify(int severity, tor_tls_t *tls, crypto_pk_t **identity_key) r = 0;

done: - if (cert) - X509_free(cert); + X509_free(cert); if (id_pkey) EVP_PKEY_free(id_pkey);

@@ -2353,8 +2345,7 @@ tor_tls_check_lifetime(int severity, tor_tls_t *tls,

r = 0; done: - if (cert) - X509_free(cert); + X509_free(cert); /* Not expected to get invoked */ tls_log_errors(tls, LOG_WARN, LD_NET, "checking certificate lifetime");

diff --git a/src/common/util.h b/src/common/util.h index 8bb4505..9ba9d9a 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -107,8 +107,8 @@ extern int dmalloc_free(const char *file, const int line, void *pnt, STMT_END #else /** Release memory allocated by tor_malloc, tor_realloc, tor_strdup, etc. - * Unlike the free() function, tor_free() will still work on NULL pointers, - * and it sets the pointer value to NULL after freeing it. + * Unlike the free() function, tor_free() sets the pointer value to NULL + * after freeing it. * * This is a macro. If you need a function pointer to release memory from * tor_malloc(), use tor_free_(). diff --git a/src/ext/eventdns.c b/src/ext/eventdns.c index a0c7ff2..cfc3344 100644 --- a/src/ext/eventdns.c +++ b/src/ext/eventdns.c @@ -1923,9 +1923,7 @@ server_request_free(struct server_request *req) rc = --req->port->refcnt; }

- if (req->response) { - mm_free(req->response); - } + mm_free(req->response);

server_request_free_answers(req);

@@ -3216,8 +3214,7 @@ load_nameservers_with_getnetworkparams(void) }

done: - if (buf) - mm_free(buf); + mm_free(buf); if (handle) FreeLibrary(handle); return status; diff --git a/src/ext/tinytest_demo.c b/src/ext/tinytest_demo.c index c07f099..7279216 100644 --- a/src/ext/tinytest_demo.c +++ b/src/ext/tinytest_demo.c @@ -169,8 +169,7 @@ test_memcpy(void *ptr)

end: /* This time our end block has something to do. */ - if (mem) - free(mem); + free(mem); }

void diff --git a/src/or/buffers.c b/src/or/buffers.c index 85fcbc6..e5d2894 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -1014,7 +1014,7 @@ fetch_var_cell_from_evbuffer(struct evbuffer *buf, var_cell_t **out, result = 1;

done: - if (free_hdr && hdr) + if (free_hdr) tor_free(hdr); return result; } @@ -2410,8 +2410,7 @@ generic_buffer_set_to_copy(generic_buffer_t **output, } } #else - if (*output) - buf_free(*output); + buf_free(*output); *output = buf_copy(input); #endif return 0; diff --git a/src/or/channel.c b/src/or/channel.c index af09502..b1dc63e 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -917,10 +917,8 @@ channel_force_free(channel_t *chan) channel_clear_remote_end(chan);

/* Get rid of cmux */ - if (chan->cmux) { - circuitmux_free(chan->cmux); - chan->cmux = NULL; - } + circuitmux_free(chan->cmux); + chan->cmux = NULL;

/* We might still have a cell queue; kill it */ TOR_SIMPLEQ_FOREACH_SAFE(cell, &chan->incoming_queue, next, cell_tmp) { diff --git a/src/or/config.c b/src/or/config.c index 6e782de..e3d7721 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -7263,11 +7263,9 @@ init_cookie_authentication(const char *fname, const char *header, goto done; }

- /* If we've already set the cookie, free it before re-setting - it. This can happen if we previously generated a cookie, but - couldn't write it to a disk. */ - if (*cookie_out) - tor_free(*cookie_out); + /* Free this cookie before re-setting it. This can happen if we + * previously generated a cookie, but couldn't write it to a disk. */ + tor_free(*cookie_out);

/* Generate the cookie */ *cookie_out = tor_malloc(cookie_len); diff --git a/src/or/ext_orport.c b/src/or/ext_orport.c index e8c8aa6..b4f4cab 100644 --- a/src/or/ext_orport.c +++ b/src/or/ext_orport.c @@ -476,9 +476,7 @@ connection_ext_or_handle_cmd_useraddr(connection_t *conn, /* record the address */ tor_addr_copy(&conn->addr, &addr); conn->port = port; - if (conn->address) { - tor_free(conn->address); - } + tor_free(conn->address); conn->address = tor_dup_addr(&addr);

return 0; @@ -509,11 +507,8 @@ connection_ext_or_handle_cmd_transport(or_connection_t *conn, return -1; }

- /* If ext_or_transport is already occupied (because the PT sent two - * TRANSPORT commands), deallocate the old name and keep the new - * one */ - if (conn->ext_or_transport) - tor_free(conn->ext_or_transport); + /* deallocate the ext_or_transport's old name and keep the new one */ + tor_free(conn->ext_or_transport);

conn->ext_or_transport = transport_str; return 0; @@ -643,7 +638,6 @@ connection_ext_or_start_auth(or_connection_t *or_conn) void ext_orport_free_all(void) { - if (ext_or_auth_cookie) /* Free the auth cookie */ - tor_free(ext_or_auth_cookie); + tor_free(ext_or_auth_cookie); }

diff --git a/src/or/onion_tap.c b/src/or/onion_tap.c index 487cbee..4645ca4 100644 --- a/src/or/onion_tap.c +++ b/src/or/onion_tap.c @@ -75,7 +75,7 @@ onion_skin_TAP_create(crypto_pk_t *dest_router_key, return 0; err: memwipe(challenge, 0, sizeof(challenge)); - if (dh) crypto_dh_free(dh); + crypto_dh_free(dh); return -1; }

diff --git a/src/or/rendservice.c b/src/or/rendservice.c index db6bc4b..1fb5245 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -1746,7 +1746,7 @@ rend_service_receive_introduction(origin_circuit_t *circuit, (unsigned)circuit->base_.n_circ_id); err: status = -1; - if (dh) crypto_dh_free(dh); + crypto_dh_free(dh); if (launched) { circuit_mark_for_close(TO_CIRCUIT(launched), reason); } diff --git a/src/or/replaycache.c b/src/or/replaycache.c index 569e073..f327fa7 100644 --- a/src/or/replaycache.c +++ b/src/or/replaycache.c @@ -18,10 +18,8 @@ void replaycache_free(replaycache_t *r) { - if (!r) { - log_info(LD_BUG, "replaycache_free() called on NULL"); + if (!r) return; - }

if (r->digests_seen) digestmap_free(r->digests_seen, tor_free_);

diff --git a/src/or/routerparse.c b/src/or/routerparse.c index c2206f1..71bcb7e 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -3840,8 +3840,7 @@ assert_addr_policy_ok(smartlist_t *lst) static void token_clear(directory_token_t *tok) { - if (tok->key) - crypto_pk_free(tok->key); + crypto_pk_free(tok->key); }

#define ALLOC_ZERO(sz) memarea_alloc_zero(area,sz) diff --git a/src/test/test.c b/src/test/test.c index e10e260..d978f78 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -572,14 +572,10 @@ test_rend_fns(void *arg) rend_encoded_v2_service_descriptor_free(smartlist_get(descs, i)); smartlist_free(descs); } - if (parsed) - rend_service_descriptor_free(parsed); - if (generated) - rend_service_descriptor_free(generated); - if (pk1) - crypto_pk_free(pk1); - if (pk2) - crypto_pk_free(pk2); + rend_service_descriptor_free(parsed); + rend_service_descriptor_free(generated); + crypto_pk_free(pk1); + crypto_pk_free(pk2); tor_free(intro_points_encrypted); }

diff --git a/src/test/test_buffers.c b/src/test/test_buffers.c index e8fce12..8cc1b61 100644 --- a/src/test/test_buffers.c +++ b/src/test/test_buffers.c @@ -189,10 +189,8 @@ test_buffers_basic(void *arg) }

done: - if (buf) - buf_free(buf); - if (buf2) - buf_free(buf2); + buf_free(buf); + buf_free(buf2); }

static void @@ -362,10 +360,8 @@ test_buffer_copy(void *arg) }

done: - if (buf) - generic_buffer_free(buf); - if (buf2) - generic_buffer_free(buf2); + generic_buffer_free(buf); + generic_buffer_free(buf2); }

static void diff --git a/src/test/test_crypto.c b/src/test/test_crypto.c index dbaec61..71a94f8 100644 --- a/src/test/test_crypto.c +++ b/src/test/test_crypto.c @@ -269,10 +269,8 @@ test_crypto_aes(void *arg)

done: tor_free(mem_op_hex_tmp); - if (env1) - crypto_cipher_free(env1); - if (env2) - crypto_cipher_free(env2); + crypto_cipher_free(env1); + crypto_cipher_free(env2); tor_free(data1); tor_free(data2); tor_free(data3); @@ -412,10 +410,8 @@ test_crypto_sha(void *arg) tt_mem_op(d_out1,OP_EQ, d_out2, DIGEST_LEN);

done: - if (d1) - crypto_digest_free(d1); - if (d2) - crypto_digest_free(d2); + crypto_digest_free(d1); + crypto_digest_free(d2); tor_free(mem_op_hex_tmp); }

diff --git a/src/test/test_dir.c b/src/test/test_dir.c index 855746e..bf7ce59 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -346,20 +346,17 @@ test_dir_formats(void *arg) dirserv_free_fingerprint_list();

done: - if (r1) - routerinfo_free(r1); - if (r2) - routerinfo_free(r2); - if (rp2) - routerinfo_free(rp2); + routerinfo_free(r1); + routerinfo_free(r2); + routerinfo_free(rp2);

tor_free(rsa_cc); tor_free(buf); tor_free(pk1_str); tor_free(pk2_str); - if (pk1) crypto_pk_free(pk1); - if (pk2) crypto_pk_free(pk2); - if (rp1) routerinfo_free(rp1); + crypto_pk_free(pk1); + crypto_pk_free(pk2); + routerinfo_free(rp1); tor_free(dir1); /* XXXX And more !*/ tor_free(dir2); /* And more !*/ } @@ -2315,32 +2312,19 @@ test_a_networkstatus( tor_free(consensus_text); tor_free(consensus_text_md);

- if (vote) - networkstatus_vote_free(vote); - if (v1) - networkstatus_vote_free(v1); - if (v2) - networkstatus_vote_free(v2); - if (v3) - networkstatus_vote_free(v3); - if (con) - networkstatus_vote_free(con); - if (con_md) - networkstatus_vote_free(con_md); - if (sign_skey_1) - crypto_pk_free(sign_skey_1); - if (sign_skey_2) - crypto_pk_free(sign_skey_2); - if (sign_skey_3) - crypto_pk_free(sign_skey_3); - if (sign_skey_leg1) - crypto_pk_free(sign_skey_leg1); - if (cert1) - authority_cert_free(cert1); - if (cert2) - authority_cert_free(cert2); - if (cert3) - authority_cert_free(cert3); + networkstatus_vote_free(vote); + networkstatus_vote_free(v1); + networkstatus_vote_free(v2); + networkstatus_vote_free(v3); + networkstatus_vote_free(con); + networkstatus_vote_free(con_md); + crypto_pk_free(sign_skey_1); + crypto_pk_free(sign_skey_2); + crypto_pk_free(sign_skey_3); + crypto_pk_free(sign_skey_leg1); + authority_cert_free(cert1); + authority_cert_free(cert2); + authority_cert_free(cert3);

tor_free(consensus_text2); tor_free(consensus_text3); @@ -2348,18 +2332,12 @@ test_a_networkstatus( tor_free(consensus_text_md3); tor_free(detached_text1); tor_free(detached_text2); - if (con2) - networkstatus_vote_free(con2); - if (con3) - networkstatus_vote_free(con3); - if (con_md2) - networkstatus_vote_free(con_md2); - if (con_md3) - networkstatus_vote_free(con_md3); - if (dsig1) - ns_detached_signatures_free(dsig1); - if (dsig2) - ns_detached_signatures_free(dsig2); + networkstatus_vote_free(con2); + networkstatus_vote_free(con3); + networkstatus_vote_free(con_md2); + networkstatus_vote_free(con_md3); + ns_detached_signatures_free(dsig1); + ns_detached_signatures_free(dsig2); }

/** Run unit tests for generating and parsing V3 consensus networkstatus diff --git a/src/test/test_replay.c b/src/test/test_replay.c index a02c160..538b2df 100644 --- a/src/test/test_replay.c +++ b/src/test/test_replay.c @@ -27,7 +27,7 @@ test_replaycache_alloc(void *arg) tt_assert(r != NULL);

done: - if (r) replaycache_free(r); + replaycache_free(r);

return; } @@ -51,7 +51,7 @@ test_replaycache_badalloc(void *arg) tt_assert(r == NULL);

done: - if (r) replaycache_free(r); + replaycache_free(r);

return; } @@ -90,7 +90,7 @@ test_replaycache_miss(void *arg) tt_int_op(result,OP_EQ, 0);

done: - if (r) replaycache_free(r); + replaycache_free(r);

return; } @@ -116,7 +116,7 @@ test_replaycache_hit(void *arg) tt_int_op(result,OP_EQ, 1);

done: - if (r) replaycache_free(r); + replaycache_free(r);

return; } @@ -147,7 +147,7 @@ test_replaycache_age(void *arg) tt_int_op(result,OP_EQ, 0);

done: - if (r) replaycache_free(r); + replaycache_free(r);

return; } @@ -175,7 +175,7 @@ test_replaycache_elapsed(void *arg) tt_int_op(elapsed,OP_EQ, 100);

done: - if (r) replaycache_free(r); + replaycache_free(r);

return; } @@ -206,7 +206,7 @@ test_replaycache_noexpire(void *arg) tt_int_op(result,OP_EQ, 1);

done: - if (r) replaycache_free(r); + replaycache_free(r);

return; } @@ -248,7 +248,7 @@ test_replaycache_scrub(void *arg) tt_int_op(digestmap_size(r->digests_seen),OP_EQ, 0);

done: - if (r) replaycache_free(r); + replaycache_free(r);

return; } @@ -292,7 +292,7 @@ test_replaycache_future(void *arg) tt_int_op(elapsed,OP_EQ, 0);

done: - if (r) replaycache_free(r); + replaycache_free(r);

return; } @@ -335,7 +335,7 @@ test_replaycache_realtime(void *arg) replaycache_scrub_if_needed(r);

done: - if (r) replaycache_free(r); + replaycache_free(r); return; }

diff --git a/src/test/test_routerset.c b/src/test/test_routerset.c index 9bd0c12..b13ec40 100644 --- a/src/test/test_routerset.c +++ b/src/test/test_routerset.c @@ -1144,8 +1144,7 @@ NS(test_main)(void *arg) tt_int_op(r, OP_EQ, 0);

done: - if (set != NULL) - routerset_free(set); + routerset_free(set); }

country_t @@ -1186,8 +1185,7 @@ NS(test_main)(void *arg) tt_int_op(smartlist_contains_string(set->list, "{??}"), OP_EQ, 1);

done: - if (set != NULL) - routerset_free(set); + routerset_free(set); }

country_t @@ -1249,8 +1247,7 @@ NS(test_main)(void *arg) tt_int_op(smartlist_contains_string(set->list, "{a1}"), OP_EQ, 1);

done: - if (set != NULL) - routerset_free(set); + routerset_free(set); }

country_t diff --git a/src/test/test_status.c b/src/test/test_status.c index cbc8af1..7e18b5d 100644 --- a/src/test/test_status.c +++ b/src/test/test_status.c @@ -145,8 +145,7 @@ NS(test_main)(void *arg) tor_free(actual);

done: - if (actual != NULL) - tor_free(actual); + tor_free(actual); }

#undef NS_SUBMODULE @@ -226,8 +225,7 @@ NS(test_main)(void *arg) tor_free(actual);

done: - if (actual != NULL) - tor_free(actual); + tor_free(actual); }

#undef NS_SUBMODULE diff --git a/src/test/test_util.c b/src/test/test_util.c index 0a5783e..a4341d9 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1707,8 +1707,7 @@ test_util_gzip(void *arg) tt_int_op(21,OP_EQ, len2);

done: - if (state) - tor_zlib_free(state); + tor_zlib_free(state); tor_free(buf2); tor_free(buf3); tor_free(buf1);