[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[tor-dev] Remove NULL checks for *_free() calls



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);
_______________________________________________
tor-dev mailing list
tor-dev@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev