From e85b49d3755375b3e535cbd4b07d4fbf953948cb Mon Sep 17 00:00:00 2001
From: Steven Fackler <sfackler@gmail.com>
Date: Fri, 18 Dec 2015 21:19:16 -0800
Subject: [PATCH] Work around the worst of clone bogusness

SslStream::{clone,try_clone} are inherently broken since the Ssl object
shared by both streams is only going to be talking to one stream. Stuff
like hyper depends on try_clone, so we'll leave it here for now but
minimize the brokenness to "no worse than what it used to be like".
They'll be removed in 0.8.

cc #325
---
 openssl/src/ssl/bio.rs       | 38 +++++++++---------------------------
 openssl/src/ssl/mod.rs       | 34 +++++++++++++++++++-------------
 openssl/src/ssl/tests/mod.rs |  8 ++++++++
 3 files changed, 37 insertions(+), 43 deletions(-)

diff --git a/openssl/src/ssl/bio.rs b/openssl/src/ssl/bio.rs
index ef63d146..99d9af0a 100644
--- a/openssl/src/ssl/bio.rs
+++ b/openssl/src/ssl/bio.rs
@@ -6,35 +6,20 @@ use std::io::prelude::*;
 use std::mem;
 use std::slice;
 use std::ptr;
+use std::sync::Arc;
 
 use ssl::error::SslError;
 
 // "rust"
 const NAME: [c_char; 5] = [114, 117, 115, 116, 0];
 
-// we use this after removing the stream from the BIO so that we don't have to
-// worry about freeing the heap allocated BIO_METHOD after freeing the BIO.
-static DESTROY_METHOD: BIO_METHOD = BIO_METHOD {
-    type_: BIO_TYPE_NONE,
-    name: &NAME[0],
-    bwrite: None,
-    bread: None,
-    bputs: None,
-    bgets: None,
-    ctrl: None,
-    create: None,
-    destroy: Some(destroy),
-    callback_ctrl: None,
-};
-
 pub struct StreamState<S> {
     pub stream: S,
     pub error: Option<io::Error>,
 }
 
-pub fn new<S: Read + Write>(stream: S) -> Result<(*mut BIO, Box<BIO_METHOD>), SslError> {
-
-    let method = Box::new(BIO_METHOD {
+pub fn new<S: Read + Write>(stream: S) -> Result<(*mut BIO, Arc<BIO_METHOD>), SslError> {
+    let method = Arc::new(BIO_METHOD {
         type_: BIO_TYPE_NONE,
         name: &NAME[0],
         bwrite: Some(bwrite::<S>),
@@ -43,7 +28,7 @@ pub fn new<S: Read + Write>(stream: S) -> Result<(*mut BIO, Box<BIO_METHOD>), Ss
         bgets: None,
         ctrl: Some(ctrl::<S>),
         create: Some(create),
-        destroy: None, // covered in the replacement BIO_METHOD
+        destroy: Some(destroy::<S>),
         callback_ctrl: None,
     });
 
@@ -66,14 +51,6 @@ pub unsafe fn take_error<S>(bio: *mut BIO) -> Option<io::Error> {
     state.error.take()
 }
 
-pub unsafe fn take_stream<S>(bio: *mut BIO) -> S {
-    let state: Box<StreamState<S>> = Box::from_raw((*bio).ptr as *mut _);
-    (*bio).ptr = ptr::null_mut();
-    (*bio).method = &DESTROY_METHOD as *const _ as *mut _;
-    (*bio).init = 0;
-    state.stream
-}
-
 pub unsafe fn get_ref<'a, S: 'a>(bio: *mut BIO) -> &'a S {
     let state: &'a StreamState<S> = mem::transmute((*bio).ptr);
     &state.stream
@@ -159,11 +136,14 @@ unsafe extern "C" fn create(bio: *mut BIO) -> c_int {
     1
 }
 
-unsafe extern "C" fn destroy(bio: *mut BIO) -> c_int {
+unsafe extern "C" fn destroy<S>(bio: *mut BIO) -> c_int {
     if bio.is_null() {
         return 0;
     }
 
-    assert!((*bio).ptr.is_null());
+    assert!(!(*bio).ptr.is_null());
+    Box::<StreamState<S>>::from_raw((*bio).ptr as *mut _);
+    (*bio).ptr = ptr::null_mut();
+    (*bio).init = 0;
     1
 }
diff --git a/openssl/src/ssl/mod.rs b/openssl/src/ssl/mod.rs
index d529347f..b4385290 100644
--- a/openssl/src/ssl/mod.rs
+++ b/openssl/src/ssl/mod.rs
@@ -10,7 +10,7 @@ use std::str;
 use std::net;
 use std::path::Path;
 use std::ptr;
-use std::sync::{Once, ONCE_INIT, Mutex};
+use std::sync::{Once, ONCE_INIT, Mutex, Arc};
 use std::cmp;
 use std::any::Any;
 #[cfg(any(feature = "npn", feature = "alpn"))]
@@ -778,6 +778,7 @@ impl Drop for Ssl {
 }
 
 impl Clone for Ssl {
+    /// # Deprecated
     fn clone(&self) -> Ssl {
         unsafe { rust_SSL_clone(self.ssl) };
         Ssl { ssl: self.ssl }
@@ -1003,23 +1004,22 @@ make_LibSslError! {
 /// A stream wrapper which handles SSL encryption for an underlying stream.
 pub struct SslStream<S> {
     ssl: Ssl,
-    _method: Box<ffi::BIO_METHOD>, // :(
+    _method: Arc<ffi::BIO_METHOD>, // NOTE: this *must* be after the Ssl field so things drop right
     _p: PhantomData<S>,
 }
 
 unsafe impl<S: Send> Send for SslStream<S> {}
 
 impl<S: Clone + Read + Write> Clone for SslStream<S> {
+    /// # Deprecated
+    ///
+    /// This method does not behave as expected and will be removed in a future
+    /// release.
     fn clone(&self) -> SslStream<S> {
-        let stream = self.get_ref().clone();
-        Self::new_base(self.ssl.clone(), stream)
-    }
-}
-
-impl<S> Drop for SslStream<S> {
-    fn drop(&mut self) {
-        unsafe {
-            let _ = bio::take_stream::<S>(self.ssl.get_raw_rbio());
+        SslStream {
+            ssl: self.ssl.clone(),
+            _method: self._method.clone(),
+            _p: PhantomData,
         }
     }
 }
@@ -1232,10 +1232,16 @@ impl<S> SslStream<S> {
 }
 
 impl SslStream<::std::net::TcpStream> {
-    /// Like `TcpStream::try_clone`.
+    /// # Deprecated
+    ///
+    /// This method does not behave as expected and will be removed in a future
+    /// release.
     pub fn try_clone(&self) -> io::Result<SslStream<::std::net::TcpStream>> {
-        let stream = try!(self.get_ref().try_clone());
-        Ok(Self::new_base(self.ssl.clone(), stream))
+        Ok(SslStream {
+            ssl: self.ssl.clone(),
+            _method: self._method.clone(),
+            _p: PhantomData
+        })
     }
 }
 
diff --git a/openssl/src/ssl/tests/mod.rs b/openssl/src/ssl/tests/mod.rs
index 7ed8fc3b..af3c005e 100644
--- a/openssl/src/ssl/tests/mod.rs
+++ b/openssl/src/ssl/tests/mod.rs
@@ -949,3 +949,11 @@ fn test_read_nonblocking() {
     assert!(bytes_read >= 5);
     assert_eq!(&input_buffer[..5], b"HTTP/");
 }
+
+#[test]
+fn broken_try_clone_doesnt_crash() {
+    let context = SslContext::new(SslMethod::Sslv23).unwrap();
+    let inner = TcpStream::connect("example.com:443").unwrap();
+    let stream1 = SslStream::connect(&context, inner).unwrap();
+    let _stream2 = stream1.try_clone().unwrap();
+}