Merge pull request #797 from sfackler/fixmes

Fix a bunch of FIXMEs

5 files changed, 39 insertions, 36 deletions

diff --git a/openssl/src/ssl/callbacks.rs b/openssl/src/ssl/callbacks.rs
index d7c48050..78602a54 100644
--- a/openssl/src/ssl/callbacks.rs
+++ b/openssl/src/ssl/callbacks.rs
@@ -1,5 +1,5 @@
 use ffi;
-use libc::{c_int, c_uint, c_char, c_uchar, c_void};
+use libc::{c_char, c_int, c_uchar, c_uint, c_void};
 use std::any::Any;
 use std::ffi::CStr;
 use std::ptr;
@@ -11,14 +11,14 @@ use error::ErrorStack;
 use dh::Dh;
 #[cfg(any(all(feature = "v101", ossl101), all(feature = "v102", ossl102)))]
 use ec_key::EcKey;
-use ssl::{get_callback_idx, get_ssl_callback_idx, SslRef, SniError, NPN_PROTOS_IDX};
+use ssl::{get_callback_idx, get_ssl_callback_idx, SniError, SslRef, NPN_PROTOS_IDX};
 #[cfg(any(all(feature = "v102", ossl102), all(feature = "v110", ossl110)))]
 use ssl::ALPN_PROTOS_IDX;
 use x509::X509StoreContextRef;
 
 pub extern "C" fn raw_verify<F>(preverify_ok: c_int, x509_ctx: *mut ffi::X509_STORE_CTX) -> c_int
 where
-    F: Fn(bool, &X509StoreContextRef) -> bool + Any + 'static + Sync + Send,
+    F: Fn(bool, &mut X509StoreContextRef) -> bool + Any + 'static + Sync + Send,
 {
     unsafe {
         let idx = ffi::SSL_get_ex_data_X509_STORE_CTX_idx();
@@ -27,7 +27,7 @@ where
         let verify = ffi::SSL_CTX_get_ex_data(ssl_ctx, get_callback_idx::<F>());
         let verify: &F = &*(verify as *mut F);
 
-        let ctx = X509StoreContextRef::from_ptr(x509_ctx);
+        let ctx = X509StoreContextRef::from_ptr_mut(x509_ctx);
 
         verify(preverify_ok != 0, ctx) as c_int
     }
@@ -74,7 +74,7 @@ pub extern "C" fn ssl_raw_verify<F>(
     x509_ctx: *mut ffi::X509_STORE_CTX,
 ) -> c_int
 where
-    F: Fn(bool, &X509StoreContextRef) -> bool + Any + 'static + Sync + Send,
+    F: Fn(bool, &mut X509StoreContextRef) -> bool + Any + 'static + Sync + Send,
 {
     unsafe {
         let idx = ffi::SSL_get_ex_data_X509_STORE_CTX_idx();
@@ -82,7 +82,7 @@ where
         let verify = ffi::SSL_get_ex_data(ssl as *const _, get_ssl_callback_idx::<F>());
         let verify: &F = &*(verify as *mut F);
 
-        let ctx = X509StoreContextRef::from_ptr(x509_ctx);
+        let ctx = X509StoreContextRef::from_ptr_mut(x509_ctx);
 
         verify(preverify_ok != 0, ctx) as c_int
     }
@@ -121,7 +121,6 @@ pub unsafe fn select_proto_using(
     inlen: c_uint,
     ex_data: c_int,
 ) -> c_int {
-
     // First, get the list of protocols (that the client should support) saved in the context
     // extra data.
     let ssl_ctx = ffi::SSL_get_SSL_CTX(ssl);
@@ -132,8 +131,8 @@ pub unsafe fn select_proto_using(
     let client_len = protocols.len() as c_uint;
     // Finally, let OpenSSL find a protocol to be used, by matching the given server and
     // client lists.
-    if ffi::SSL_select_next_proto(out, outlen, inbuf, inlen, client, client_len) !=
-        ffi::OPENSSL_NPN_NEGOTIATED
+    if ffi::SSL_select_next_proto(out, outlen, inbuf, inlen, client, client_len)
+        != ffi::OPENSSL_NPN_NEGOTIATED
     {
         ffi::SSL_TLSEXT_ERR_NOACK
     } else {
diff --git a/openssl/src/ssl/connector.rs b/openssl/src/ssl/connector.rs
index dc65ad5e..dc13ea97 100644
--- a/openssl/src/ssl/connector.rs
+++ b/openssl/src/ssl/connector.rs
@@ -63,11 +63,9 @@ impl SslConnectorBuilder {
         ctx.set_default_verify_paths()?;
         // From https://github.com/python/cpython/blob/a170fa162dc03f0a014373349e548954fff2e567/Lib/ssl.py#L193
         ctx.set_cipher_list(
-            "TLS13-AES-256-GCM-SHA384:TLS13-CHACHA20-POLY1305-SHA256:\
-             TLS13-AES-128-GCM-SHA256:\
-             ECDH+AESGCM:ECDH+CHACHA20:DH+AESGCM:DH+CHACHA20:ECDH+AES256:DH+AES256:\
-             ECDH+AES128:DH+AES:ECDH+HIGH:DH+HIGH:RSA+AESGCM:RSA+AES:RSA+HIGH:\
-             !aNULL:!eNULL:!MD5:!3DES",
+            "TLS13-AES-256-GCM-SHA384:TLS13-CHACHA20-POLY1305-SHA256:TLS13-AES-128-GCM-SHA256:\
+             ECDH+AESGCM:ECDH+CHACHA20:DH+AESGCM:DH+CHACHA20:ECDH+AES256:DH+AES256:ECDH+AES128:\
+             DH+AES:ECDH+HIGH:DH+HIGH:RSA+AESGCM:RSA+AES:RSA+HIGH:!aNULL:!eNULL:!MD5:!3DES",
         )?;
         setup_verify(&mut ctx);
 
diff --git a/openssl/src/ssl/error.rs b/openssl/src/ssl/error.rs
index 48f49979..358a88b0 100644
--- a/openssl/src/ssl/error.rs
+++ b/openssl/src/ssl/error.rs
@@ -101,11 +101,10 @@ pub enum HandshakeError<S> {
     SetupFailure(ErrorStack),
     /// The handshake failed.
     Failure(MidHandshakeSslStream<S>),
-    /// The handshake was interrupted midway through.
+    /// The handshake encountered a `WouldBlock` error midway through.
     ///
     /// This error will never be returned for blocking streams.
-    // FIXME change to WouldBlock
-    Interrupted(MidHandshakeSslStream<S>),
+    WouldBlock(MidHandshakeSslStream<S>),
 }
 
 impl<S: Any + fmt::Debug> StdError for HandshakeError<S> {
@@ -113,15 +112,14 @@ impl<S: Any + fmt::Debug> StdError for HandshakeError<S> {
         match *self {
             HandshakeError::SetupFailure(_) => "stream setup failed",
             HandshakeError::Failure(_) => "the handshake failed",
-            HandshakeError::Interrupted(_) => "the handshake was interrupted",
+            HandshakeError::WouldBlock(_) => "the handshake was interrupted",
         }
     }
 
     fn cause(&self) -> Option<&StdError> {
         match *self {
             HandshakeError::SetupFailure(ref e) => Some(e),
-            HandshakeError::Failure(ref s) |
-            HandshakeError::Interrupted(ref s) => Some(s.error()),
+            HandshakeError::Failure(ref s) | HandshakeError::WouldBlock(ref s) => Some(s.error()),
         }
     }
 }
@@ -131,8 +129,7 @@ impl<S: Any + fmt::Debug> fmt::Display for HandshakeError<S> {
         f.write_str(StdError::description(self))?;
         match *self {
             HandshakeError::SetupFailure(ref e) => write!(f, ": {}", e)?,
-            HandshakeError::Failure(ref s) |
-            HandshakeError::Interrupted(ref s) => {
+            HandshakeError::Failure(ref s) | HandshakeError::WouldBlock(ref s) => {
                 write!(f, ": {}", s.error())?;
                 if let Some(err) = s.ssl().verify_result() {
                     write!(f, ": {}", err)?;
diff --git a/openssl/src/ssl/mod.rs b/openssl/src/ssl/mod.rs
index 371b66c0..c59ad8d8 100644
--- a/openssl/src/ssl/mod.rs
+++ b/openssl/src/ssl/mod.rs
@@ -41,12 +41,12 @@
 //! let mut pkcs12 = vec![];
 //! file.read_to_end(&mut pkcs12).unwrap();
 //! let pkcs12 = Pkcs12::from_der(&pkcs12).unwrap();
-//! let identity = pkcs12.parse("password123").unwrap();
+//! let identity = pkcs12.parse(b"password123").unwrap();
 //!
 //! let acceptor = SslAcceptorBuilder::mozilla_intermediate(SslMethod::tls(),
 //!                                                         &identity.pkey,
 //!                                                         &identity.cert,
-//!                                                         &identity.chain)
+//!                                                         &identity.chain.unwrap())
 //!     .unwrap()
 //!     .build();
 //! let acceptor = Arc::new(acceptor);
@@ -83,7 +83,7 @@ use std::fmt;
 use std::io;
 use std::io::prelude::*;
 use std::marker::PhantomData;
-use std::mem;
+use std::mem::{self, ManuallyDrop};
 use std::ops::{Deref, DerefMut};
 use std::panic::resume_unwind;
 use std::path::Path;
@@ -490,7 +490,7 @@ impl SslContextBuilder {
     pub fn set_verify_callback<F>(&mut self, mode: SslVerifyMode, verify: F)
     where
         // FIXME should take a mutable reference to the store
-        F: Fn(bool, &X509StoreContextRef) -> bool + Any + 'static + Sync + Send,
+        F: Fn(bool, &mut X509StoreContextRef) -> bool + Any + 'static + Sync + Send,
     {
         unsafe {
             let verify = Box::new(verify);
@@ -1500,7 +1500,7 @@ impl SslRef {
     pub fn set_verify_callback<F>(&mut self, mode: SslVerifyMode, verify: F)
     where
         // FIXME should take a mutable reference to the x509 store
-        F: Fn(bool, &X509StoreContextRef) -> bool + Any + 'static + Sync + Send,
+        F: Fn(bool, &mut X509StoreContextRef) -> bool + Any + 'static + Sync + Send,
     {
         unsafe {
             let verify = Box::new(verify);
@@ -2069,7 +2069,7 @@ impl Ssl {
         } else {
             match stream.make_error(ret) {
                 e @ Error::WantWrite(_) | e @ Error::WantRead(_) => {
-                    Err(HandshakeError::Interrupted(MidHandshakeSslStream {
+                    Err(HandshakeError::WouldBlock(MidHandshakeSslStream {
                         stream: stream,
                         error: e,
                     }))
@@ -2103,7 +2103,7 @@ impl Ssl {
         } else {
             match stream.make_error(ret) {
                 e @ Error::WantWrite(_) | e @ Error::WantRead(_) => {
-                    Err(HandshakeError::Interrupted(MidHandshakeSslStream {
+                    Err(HandshakeError::WouldBlock(MidHandshakeSslStream {
                         stream: stream,
                         error: e,
                     }))
@@ -2163,7 +2163,7 @@ impl<S> MidHandshakeSslStream<S> {
             match self.stream.make_error(ret) {
                 e @ Error::WantWrite(_) | e @ Error::WantRead(_) => {
                     self.error = e;
-                    Err(HandshakeError::Interrupted(self))
+                    Err(HandshakeError::WouldBlock(self))
                 }
                 err => {
                     self.error = err;
@@ -2176,12 +2176,21 @@ impl<S> MidHandshakeSslStream<S> {
 
 /// A TLS session over a stream.
 pub struct SslStream<S> {
-    // FIXME use ManuallyDrop
-    ssl: Ssl,
-    _method: BioMethod, // NOTE: this *must* be after the Ssl field so things drop right
+    ssl: ManuallyDrop<Ssl>,
+    method: ManuallyDrop<BioMethod>,
     _p: PhantomData<S>,
 }
 
+impl<S> Drop for SslStream<S> {
+    fn drop(&mut self) {
+        // ssl holds a reference to method internally so it has to drop first
+        unsafe {
+            ManuallyDrop::drop(&mut self.ssl);
+            ManuallyDrop::drop(&mut self.method);
+        }
+    }
+}
+
 impl<S> fmt::Debug for SslStream<S>
 where
     S: fmt::Debug,
@@ -2201,8 +2210,8 @@ impl<S: Read + Write> SslStream<S> {
             ffi::SSL_set_bio(ssl.as_ptr(), bio, bio);
 
             SslStream {
-                ssl: ssl,
-                _method: method,
+                ssl: ManuallyDrop::new(ssl),
+                method: ManuallyDrop::new(method),
                 _p: PhantomData,
             }
         }
diff --git a/openssl/src/ssl/tests/mod.rs b/openssl/src/ssl/tests/mod.rs
index 365f0168..69369ed4 100644
--- a/openssl/src/ssl/tests/mod.rs
+++ b/openssl/src/ssl/tests/mod.rs
@@ -753,7 +753,7 @@ fn wait_io(stream: &TcpStream, read: bool, timeout_ms: u32) -> bool {
 fn handshake(res: Result<SslStream<TcpStream>, HandshakeError<TcpStream>>) -> SslStream<TcpStream> {
     match res {
         Ok(s) => s,
-        Err(HandshakeError::Interrupted(s)) => {
+        Err(HandshakeError::WouldBlock(s)) => {
             wait_io(s.get_ref(), true, 1_000);
             handshake(s.handshake())
         }