Implement SSL_clear with ssl_new and ssl_free.

State on s3 gets freed in both ssl3_clear and ssl3_free. Considate to just
ssl3_free. This replaces the (SSL,ssl,ssl3)_clear calls in (SSL,ssl,ssl3)_new
with the state that was initialized. This results in a little code duplication
between SSL_new and SSL_clear because state is on the wrong object. I've just
left TODOs for now; some of it will need disentangling.

We're far from it, but going forward, separate state between s and s->s3 as:

- s contains configuration state, DTLS or TLS. It is initialized from SSL_CTX,
  configurable directly afterwards, and preserved across SSL_clear calls.
  (Including when it's implicitly set as part of a handshake callback.)

- Connection state hangs off s->s3 (TLS) and s->d1 (DTLS). It is reset across
  SSL_clear. This should happen naturally out of a ssl_free/ssl_new pair.

The goal is to avoid needing separate initialize and reset code for anything;
the point any particular state is reset is the point its owning context is
destroyed and recreated.

Change-Id: I5d779010778109f8c339c07433a0777feaf94d1f
Reviewed-on: https://boringssl-review.googlesource.com/2822
Reviewed-by: Adam Langley <agl@google.com>

ssl/d1_lib.c

@@ -162,7 +162,13 @@ int dtls1_new(SSL *s) {
   }
 
   s->d1 = d1;
-  s->method->ssl_clear(s);
+
+  /* Set the version to the highest version for DTLS. This controls the initial
+   * state of |s->enc_method| and what the API reports as the version prior to
+   * negotiation.
+   *
+   * TODO(davidben): This is fragile and confusing. */
+  s->version = DTLS1_2_VERSION;
   return 1;
 }
 
@@ -214,6 +220,10 @@ static void dtls1_clear_queues(SSL *s) {
 void dtls1_free(SSL *s) {
   ssl3_free(s);
 
+  if (s == NULL || s->d1 == NULL) {
+    return;
+  }
+
   dtls1_clear_queues(s);
 
   pqueue_free(s->d1->unprocessed_rcds.q);
@@ -226,41 +236,6 @@ void dtls1_free(SSL *s) {
   s->d1 = NULL;
 }
 
-void dtls1_clear(SSL *s) {
-  pqueue unprocessed_rcds;
-  pqueue processed_rcds;
-  pqueue buffered_messages;
-  pqueue sent_messages;
-  pqueue buffered_app_data;
-  unsigned int mtu;
-
-  if (s->d1) {
-    unprocessed_rcds = s->d1->unprocessed_rcds.q;
-    processed_rcds = s->d1->processed_rcds.q;
-    buffered_messages = s->d1->buffered_messages;
-    sent_messages = s->d1->sent_messages;
-    buffered_app_data = s->d1->buffered_app_data.q;
-    mtu = s->d1->mtu;
-
-    dtls1_clear_queues(s);
-
-    memset(s->d1, 0, sizeof(*(s->d1)));
-
-    if (SSL_get_options(s) & SSL_OP_NO_QUERY_MTU) {
-      s->d1->mtu = mtu;
-    }
-
-    s->d1->unprocessed_rcds.q = unprocessed_rcds;
-    s->d1->processed_rcds.q = processed_rcds;
-    s->d1->buffered_messages = buffered_messages;
-    s->d1->sent_messages = sent_messages;
-    s->d1->buffered_app_data.q = buffered_app_data;
-  }
-
-  ssl3_clear(s);
-  s->version = DTLS1_2_VERSION;
-}
-
 long dtls1_ctrl(SSL *s, int cmd, long larg, void *parg) {
   int ret = 0;
 

ssl/d1_meth.c

@@ -60,7 +60,6 @@
 
 static const SSL_PROTOCOL_METHOD DTLS_protocol_method = {
   dtls1_new,
-  dtls1_clear,
   dtls1_free,
   dtls1_accept,
   dtls1_connect,

ssl/s3_lib.c

@@ -640,15 +640,20 @@ int ssl3_new(SSL *s) {
   memset(s3->wrec.seq_num, 0, sizeof(s3->wrec.seq_num));
 
   s->s3 = s3;
-  s->method->ssl_clear(s);
 
+  /* Set the version to the highest supported version for TLS. This controls the
+   * initial state of |s->enc_method| and what the API reports as the version
+   * prior to negotiation.
+   *
+   * TODO(davidben): This is fragile and confusing. */
+  s->version = TLS1_2_VERSION;
   return 1;
 err:
   return 0;
 }
 
 void ssl3_free(SSL *s) {
-  if (s == NULL) {
+  if (s == NULL || s->s3 == NULL) {
     return;
   }
 
@@ -699,92 +704,6 @@ void ssl3_free(SSL *s) {
   s->s3 = NULL;
 }
 
-void ssl3_clear(SSL *s) {
-  uint8_t *rp, *wp;
-  size_t rlen, wlen;
-  int init_extra;
-
-  /* TODO(davidben): Can this just call ssl3_free + ssl3_new. rbuf, wbuf, and
-   * init_extra are preserved, but this may not serve anything more than saving
-   * a malloc. */
-
-  if (s->s3->sniff_buffer != NULL) {
-    BUF_MEM_free(s->s3->sniff_buffer);
-  }
-  s->s3->sniff_buffer = NULL;
-
-  ssl3_cleanup_key_block(s);
-  if (s->s3->tmp.ca_names != NULL) {
-    sk_X509_NAME_pop_free(s->s3->tmp.ca_names, X509_NAME_free);
-  }
-  s->s3->tmp.ca_names = NULL;
-  if (s->s3->tmp.certificate_types != NULL) {
-    OPENSSL_free(s->s3->tmp.certificate_types);
-  }
-  s->s3->tmp.certificate_types = NULL;
-  if (s->s3->tmp.peer_ecpointformatlist) {
-    OPENSSL_free(s->s3->tmp.peer_ecpointformatlist);
-  }
-  s->s3->tmp.peer_ecpointformatlist = NULL;
-  if (s->s3->tmp.peer_ellipticcurvelist) {
-    OPENSSL_free(s->s3->tmp.peer_ellipticcurvelist);
-  }
-  s->s3->tmp.peer_ellipticcurvelist = NULL;
-  if (s->s3->tmp.peer_psk_identity_hint) {
-    OPENSSL_free(s->s3->tmp.peer_psk_identity_hint);
-  }
-  s->s3->tmp.peer_psk_identity_hint = NULL;
-
-  if (s->s3->tmp.dh != NULL) {
-    DH_free(s->s3->tmp.dh);
-    s->s3->tmp.dh = NULL;
-  }
-  if (s->s3->tmp.ecdh != NULL) {
-    EC_KEY_free(s->s3->tmp.ecdh);
-    s->s3->tmp.ecdh = NULL;
-  }
-  rp = s->s3->rbuf.buf;
-  wp = s->s3->wbuf.buf;
-  rlen = s->s3->rbuf.len;
-  wlen = s->s3->wbuf.len;
-  init_extra = s->s3->init_extra;
-  if (s->s3->handshake_buffer) {
-    BIO_free(s->s3->handshake_buffer);
-    s->s3->handshake_buffer = NULL;
-  }
-  if (s->s3->handshake_dgst) {
-    ssl3_free_digest_list(s);
-  }
-
-  if (s->s3->alpn_selected) {
-    OPENSSL_free(s->s3->alpn_selected);
-    s->s3->alpn_selected = NULL;
-  }
-  memset(s->s3, 0, sizeof *s->s3);
-  s->s3->rbuf.buf = rp;
-  s->s3->wbuf.buf = wp;
-  s->s3->rbuf.len = rlen;
-  s->s3->wbuf.len = wlen;
-  s->s3->init_extra = init_extra;
-
-  ssl_free_wbio_buffer(s);
-
-  s->packet_length = 0;
-  s->s3->renegotiate = 0;
-  s->s3->total_renegotiations = 0;
-  s->s3->num_renegotiations = 0;
-  s->s3->in_read_app_data = 0;
-  s->version = TLS1_2_VERSION;
-
-  if (s->next_proto_negotiated) {
-    OPENSSL_free(s->next_proto_negotiated);
-    s->next_proto_negotiated = NULL;
-    s->next_proto_negotiated_len = 0;
-  }
-
-  s->s3->tlsext_channel_id_valid = 0;
-}
-
 static int ssl3_set_req_cert_type(CERT *c, const uint8_t *p, size_t len);
 
 long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg) {

ssl/s3_meth.c

@@ -59,7 +59,6 @@
 
 static const SSL_PROTOCOL_METHOD TLS_protocol_method = {
     ssl3_new,
-    ssl3_clear,
     ssl3_free,
     ssl3_accept,
     ssl3_connect,

ssl/ssl_lib.c

@@ -190,6 +190,12 @@ int SSL_clear(SSL *s) {
     assert(s->state == 0);
   }
 
+  /* TODO(davidben): Some state on |s| is reset both in |SSL_new| and
+   * |SSL_clear| because it is per-connection state rather than configuration
+   * state. Per-connection state should be on |s->s3| and |s->d1| so it is
+   * naturally reset at the right points between |SSL_new|, |SSL_clear|, and
+   * |ssl3_new|. */
+
   s->rwstate = SSL_NOTHING;
   s->rstate = SSL_ST_READ_HEADER;
 
@@ -198,11 +204,39 @@ int SSL_clear(SSL *s) {
     s->init_buf = NULL;
   }
 
+  s->packet = NULL;
+  s->packet_length = 0;
+
   ssl_clear_cipher_ctx(s);
   ssl_clear_hash_ctx(&s->read_hash);
   ssl_clear_hash_ctx(&s->write_hash);
 
-  s->method->ssl_clear(s);
+  if (s->next_proto_negotiated) {
+    OPENSSL_free(s->next_proto_negotiated);
+    s->next_proto_negotiated = NULL;
+    s->next_proto_negotiated_len = 0;
+  }
+
+  /* The s->d1->mtu is simultaneously configuration (preserved across
+   * clear) and connection-specific state (gets reset).
+   *
+   * TODO(davidben): Avoid this. */
+  unsigned mtu = 0;
+  if (s->d1 != NULL) {
+    mtu = s->d1->mtu;
+  }
+
+  s->method->ssl_free(s);
+  if (!s->method->ssl_new(s)) {
+    return 0;
+  }
+  s->enc_method = ssl3_get_enc_method(s->version);
+  assert(s->enc_method != NULL);
+
+  if (SSL_IS_DTLS(s) && (SSL_get_options(s) & SSL_OP_NO_QUERY_MTU)) {
+    s->d1->mtu = mtu;
+  }
+
   s->client_version = s->version;
 
   return 1;
@@ -315,7 +349,8 @@ SSL *SSL_new(SSL_CTX *ctx) {
 
   s->references = 1;
 
-  SSL_clear(s);
+  s->rwstate = SSL_NOTHING;
+  s->rstate = SSL_ST_READ_HEADER;
 
   CRYPTO_new_ex_data(CRYPTO_EX_INDEX_SSL, s, &s->ex_data);
 

ssl/ssl_locl.h

@@ -559,7 +559,6 @@ struct ssl_method_st {
 /* Used to hold functions for SSLv2 or SSLv3/TLSv1 functions */
 struct ssl_protocol_method_st {
   int (*ssl_new)(SSL *s);
-  void (*ssl_clear)(SSL *s);
   void (*ssl_free)(SSL *s);
   int (*ssl_accept)(SSL *s);
   int (*ssl_connect)(SSL *s);
@@ -792,7 +791,6 @@ int ssl3_read(SSL *s, void *buf, int len);
 int ssl3_peek(SSL *s, void *buf, int len);
 int ssl3_write(SSL *s, const void *buf, int len);
 int ssl3_shutdown(SSL *s);
-void ssl3_clear(SSL *s);
 long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg);
 long ssl3_ctx_ctrl(SSL_CTX *s, int cmd, long larg, void *parg);
 long ssl3_callback_ctrl(SSL *s, int cmd, void (*fp)(void));
@@ -877,7 +875,6 @@ int dtls1_new(SSL *s);
 int dtls1_accept(SSL *s);
 int dtls1_connect(SSL *s);
 void dtls1_free(SSL *s);
-void dtls1_clear(SSL *s);
 long dtls1_ctrl(SSL *s, int cmd, long larg, void *parg);
 int dtls1_shutdown(SSL *s);