From 0d4dec074a931973d3f66bd87c78a93a6a2166e7 Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Tue, 23 Apr 2024 10:42:52 +0100 Subject: [PATCH] Remove locking This was the wrong direction: coming commits want to call callbacks (which themselves can and do re-enter the API) which would lead to deadlock. Let's just accept that the API we're implementing is not thread safe. I also looked at deferring callbacks to outside the lock scope, but that removes the ability for callbacks to have side effects during the handshake. --- rustls-libssl/src/entry.rs | 556 +++++++++------------------ rustls-libssl/src/lib.rs | 32 +- rustls-libssl/src/not_thread_safe.rs | 35 ++ 3 files changed, 226 insertions(+), 397 deletions(-) create mode 100644 rustls-libssl/src/not_thread_safe.rs diff --git a/rustls-libssl/src/entry.rs b/rustls-libssl/src/entry.rs index c833168..183786b 100644 --- a/rustls-libssl/src/entry.rs +++ b/rustls-libssl/src/entry.rs @@ -6,13 +6,9 @@ use core::{mem, ptr}; use std::io::{self, Read}; use std::os::raw::{c_char, c_int, c_long, c_uchar, c_uint, c_void}; -use std::sync::Mutex; use std::{fs, path::PathBuf}; -use openssl_sys::{ - stack_st_X509, OPENSSL_malloc, EVP_PKEY, X509, X509_STORE, X509_STORE_CTX, - X509_V_ERR_UNSPECIFIED, -}; +use openssl_sys::{stack_st_X509, OPENSSL_malloc, EVP_PKEY, X509, X509_STORE, X509_STORE_CTX}; use rustls::pki_types::{CertificateDer, PrivatePkcs8KeyDer}; use crate::bio::{Bio, BIO, BIO_METHOD}; @@ -22,6 +18,7 @@ use crate::ffi::{ free_arc, str_from_cstring, to_arc_mut_ptr, try_clone_arc, try_from, try_mut_slice_int, try_ref_from_ptr, try_slice, try_slice_int, try_str, Castable, OwnershipArc, OwnershipRef, }; +use crate::not_thread_safe::NotThreadSafe; use crate::x509::{load_certs, OwnedX509, OwnedX509Stack}; use crate::{HandshakeState, ShutdownResult}; @@ -110,7 +107,7 @@ type SSL_CTX = crate::SslContext; entry! { pub fn _SSL_CTX_new(meth: *const SSL_METHOD) -> *mut SSL_CTX { let method = try_ref_from_ptr!(meth); - to_arc_mut_ptr(Mutex::new(crate::SslContext::new(method))) + to_arc_mut_ptr(NotThreadSafe::new(crate::SslContext::new(method))) } } @@ -131,30 +128,21 @@ entry! { entry! { pub fn _SSL_CTX_get_options(ctx: *const SSL_CTX) -> u64 { let ctx = try_clone_arc!(ctx); - ctx.lock() - .ok() - .map(|ctx| ctx.get_options()) - .unwrap_or_default() + ctx.get().get_options() } } entry! { pub fn _SSL_CTX_clear_options(ctx: *mut SSL_CTX, op: u64) -> u64 { let ctx = try_clone_arc!(ctx); - ctx.lock() - .ok() - .map(|mut ctx| ctx.clear_options(op)) - .unwrap_or_default() + ctx.get_mut().clear_options(op) } } entry! { pub fn _SSL_CTX_set_options(ctx: *mut SSL_CTX, op: u64) -> u64 { let ctx = try_clone_arc!(ctx); - ctx.lock() - .ok() - .map(|mut ctx| ctx.set_options(op)) - .unwrap_or_default() + ctx.get_mut().set_options(op) } } @@ -162,50 +150,45 @@ entry! { pub fn _SSL_CTX_ctrl(ctx: *mut SSL_CTX, cmd: c_int, larg: c_long, parg: *mut c_void) -> c_long { let ctx = try_clone_arc!(ctx); - let result = if let Ok(mut inner) = ctx.lock() { - match SslCtrl::try_from(cmd) { - Ok(SslCtrl::Mode) => { - log::warn!("unimplemented SSL_CTX_set_mode()"); - 0 - } - Ok(SslCtrl::SetMsgCallbackArg) => { - log::warn!("unimplemented SSL_CTX_set_msg_callback_arg()"); - 0 - } - Ok(SslCtrl::SetMaxProtoVersion) => { - log::warn!("unimplemented SSL_CTX_set_max_proto_version()"); - 1 - } - Ok(SslCtrl::SetTlsExtHostname) => { - // not a defined operation in the OpenSSL API - 0 - } - Ok(SslCtrl::SetChain) => { - let chain = if parg.is_null() { - // this is `SSL_CTX_clear_chain_certs` - vec![] - } else { - match larg { - // this is `SSL_CTX_set1_chain` (incs ref) - 1 => OwnedX509Stack::new_copy(parg as *mut stack_st_X509).to_rustls(), - // this is `SSL_CTX_set0_chain` (retain ref) - _ => OwnedX509Stack::new(parg as *mut stack_st_X509).to_rustls(), - } - }; - - inner.stage_certificate_chain(chain); - C_INT_SUCCESS as i64 - } + match SslCtrl::try_from(cmd) { + Ok(SslCtrl::Mode) => { + log::warn!("unimplemented SSL_CTX_set_mode()"); + 0 + } + Ok(SslCtrl::SetMsgCallbackArg) => { + log::warn!("unimplemented SSL_CTX_set_msg_callback_arg()"); + 0 + } + Ok(SslCtrl::SetMaxProtoVersion) => { + log::warn!("unimplemented SSL_CTX_set_max_proto_version()"); + 1 + } + Ok(SslCtrl::SetTlsExtHostname) => { + // not a defined operation in the OpenSSL API + 0 + } + Ok(SslCtrl::SetChain) => { + let chain = if parg.is_null() { + // this is `SSL_CTX_clear_chain_certs` + vec![] + } else { + match larg { + // this is `SSL_CTX_set1_chain` (incs ref) + 1 => OwnedX509Stack::new_copy(parg as *mut stack_st_X509).to_rustls(), + // this is `SSL_CTX_set0_chain` (retain ref) + _ => OwnedX509Stack::new(parg as *mut stack_st_X509).to_rustls(), + } + }; - Err(()) => { - log::warn!("unimplemented _SSL_CTX_ctrl(..., {cmd}, {larg}, ...)"); - 0 - } + ctx.get_mut().stage_certificate_chain(chain); + C_INT_SUCCESS as i64 } - } else { - 0 - }; - result + + Err(()) => { + log::warn!("unimplemented _SSL_CTX_ctrl(..., {cmd}, {larg}, ...)"); + 0 + } + } } } @@ -220,10 +203,7 @@ entry! { return Error::not_supported("verify callback").raise().into(); } - ctx.lock() - .ok() - .map(|mut ctx| ctx.set_verify(crate::VerifyMode::from(mode))) - .unwrap_or_default(); + ctx.get_mut().set_verify(crate::VerifyMode::from(mode)); } } @@ -233,24 +213,20 @@ pub type SSL_verify_cb = entry! { pub fn _SSL_CTX_get_cert_store(ctx: *const SSL_CTX) -> *mut X509_STORE { let ctx = try_clone_arc!(ctx); - ctx.lock() - .ok() - .map(|ctx| ctx.get_x509_store()) - .unwrap_or(ptr::null_mut()) + ctx.get().get_x509_store() } } -fn load_verify_files(ctx: &Mutex, file_names: impl Iterator) -> c_int { +fn load_verify_files( + ctx: &NotThreadSafe, + file_names: impl Iterator, +) -> c_int { let certs = match load_certs(file_names) { Err(e) => return e.into(), Ok(certs) => certs, }; - match ctx - .lock() - .map_err(|_| Error::cannot_lock()) - .and_then(|mut ctx| ctx.add_trusted_certs(certs)) - { + match ctx.get_mut().add_trusted_certs(certs) { Err(e) => e.raise().into(), Ok(()) => C_INT_SUCCESS, } @@ -259,42 +235,24 @@ fn load_verify_files(ctx: &Mutex, file_names: impl Iterator c_int { let ctx = try_clone_arc!(ctx); - match ctx - .lock() - .map_err(|_| Error::cannot_lock()) - .map(|mut ctx| ctx.set_default_verify_paths()) - { - Err(e) => e.raise().into(), - Ok(()) => C_INT_SUCCESS, - } + ctx.get_mut().set_default_verify_paths(); + C_INT_SUCCESS } } entry! { pub fn _SSL_CTX_set_default_verify_dir(ctx: *mut SSL_CTX) -> c_int { let ctx = try_clone_arc!(ctx); - match ctx - .lock() - .map_err(|_| Error::cannot_lock()) - .map(|mut ctx| ctx.set_default_verify_dir()) - { - Err(e) => e.raise().into(), - Ok(()) => C_INT_SUCCESS, - } + ctx.get_mut().set_default_verify_dir(); + C_INT_SUCCESS } } entry! { pub fn _SSL_CTX_set_default_verify_file(ctx: *mut SSL_CTX) -> c_int { let ctx = try_clone_arc!(ctx); - match ctx - .lock() - .map_err(|_| Error::cannot_lock()) - .map(|mut ctx| ctx.set_default_verify_file()) - { - Err(e) => e.raise().into(), - Ok(()) => C_INT_SUCCESS, - } + ctx.get_mut().set_default_verify_file(); + C_INT_SUCCESS } } @@ -303,7 +261,7 @@ entry! { let ctx = try_clone_arc!(ctx); let ca_file = try_str!(ca_file); let path_buf = PathBuf::from(ca_file); - load_verify_files(ctx.as_ref(), [path_buf].into_iter()) + load_verify_files(&ctx, [path_buf].into_iter()) } } @@ -341,14 +299,8 @@ entry! { } }; - match ctx - .lock() - .map_err(|_| Error::cannot_lock()) - .map(|mut ctx| ctx.set_alpn_offer(alpn)) - { - Err(e) => e.raise().into(), - Ok(()) => MysteriouslyOppositeReturnValue::Success, - } + ctx.get_mut().set_alpn_offer(alpn); + MysteriouslyOppositeReturnValue::Success } } @@ -385,14 +337,8 @@ entry! { } } - match ctx - .lock() - .map_err(|_| Error::cannot_lock()) - .map(|mut ctx| ctx.stage_certificate_chain(chain)) - { - Err(e) => e.raise().into(), - Ok(()) => C_INT_SUCCESS, - } + ctx.get_mut().stage_certificate_chain(chain); + C_INT_SUCCESS } } @@ -407,14 +353,8 @@ entry! { let x509 = OwnedX509::new_incref(x); let ee = CertificateDer::from(x509.der_bytes()); - match ctx - .lock() - .map_err(|_| Error::cannot_lock()) - .map(|mut ctx| ctx.stage_certificate_end_entity(ee)) - { - Err(e) => e.raise().into(), - Ok(()) => C_INT_SUCCESS, - } + ctx.get_mut().stage_certificate_end_entity(ee); + C_INT_SUCCESS } } @@ -468,11 +408,7 @@ entry! { Some(key) => key, }; - match ctx - .lock() - .map_err(|_| Error::cannot_lock()) - .and_then(|mut ctx| ctx.commit_private_key(key)) - { + match ctx.get_mut().commit_private_key(key) { Err(e) => e.raise().into(), Ok(()) => C_INT_SUCCESS, } @@ -489,11 +425,7 @@ entry! { let pkey = EvpPkey::new_incref(pkey); - match ctx - .lock() - .map_err(|_| Error::cannot_lock()) - .and_then(|mut ctx| ctx.commit_private_key(pkey)) - { + match ctx.get_mut().commit_private_key(pkey) { Err(e) => e.raise().into(), Ok(()) => C_INT_SUCCESS, } @@ -503,20 +435,14 @@ entry! { entry! { pub fn _SSL_CTX_get0_certificate(ctx: *const SSL_CTX) -> *mut X509 { let ctx = try_clone_arc!(ctx); - ctx.lock() - .ok() - .map(|ctx| ctx.get_certificate()) - .unwrap_or(ptr::null_mut()) + ctx.get().get_certificate() } } entry! { pub fn _SSL_CTX_get0_privatekey(ctx: *const SSL_CTX) -> *mut EVP_PKEY { let ctx = try_clone_arc!(ctx); - ctx.lock() - .ok() - .map(|ctx| ctx.get_privatekey()) - .unwrap_or(ptr::null_mut()) + ctx.get().get_privatekey() } } @@ -529,7 +455,7 @@ entry! { impl Castable for SSL_CTX { type Ownership = OwnershipArc; - type RustType = Mutex; + type RustType = NotThreadSafe; } type SSL = crate::Ssl; @@ -538,17 +464,12 @@ entry! { pub fn _SSL_new(ctx: *mut SSL_CTX) -> *mut SSL { let ctx = try_clone_arc!(ctx); - let ssl_ctx = match ctx.lock().ok() { - Some(ssl_ctx) => ssl_ctx, - None => return ptr::null_mut(), - }; - - let ssl = match crate::Ssl::new(ctx.clone(), &ssl_ctx).ok() { + let ssl = match crate::Ssl::new(ctx.clone(), ctx.get()).ok() { Some(ssl) => ssl, None => return ptr::null_mut(), }; - to_arc_mut_ptr(Mutex::new(ssl)) + to_arc_mut_ptr(NotThreadSafe::new(ssl)) } } @@ -570,79 +491,65 @@ entry! { pub fn _SSL_ctrl(ssl: *mut SSL, cmd: c_int, larg: c_long, parg: *mut c_void) -> c_long { let ssl = try_clone_arc!(ssl); - let result = if let Ok(mut inner) = ssl.lock() { - match SslCtrl::try_from(cmd) { - Ok(SslCtrl::Mode) => { - log::warn!("unimplemented SSL_set_mode()"); - 0 - } - Ok(SslCtrl::SetMsgCallbackArg) => { - log::warn!("unimplemented SSL_set_msg_callback_arg()"); - 0 - } - Ok(SslCtrl::SetMaxProtoVersion) => { - log::warn!("unimplemented SSL_set_max_proto_version()"); - 1 - } - Ok(SslCtrl::SetTlsExtHostname) => { - let hostname = try_str!(parg as *const c_char); - inner.set_sni_hostname(hostname) as c_long - } - Ok(SslCtrl::SetChain) => { - let chain = if parg.is_null() { - // this is `SSL_clear_chain_certs` - vec![] - } else { - match larg { - // this is `SSL_set1_chain` (incs ref) - 1 => OwnedX509Stack::new_copy(parg as *mut stack_st_X509).to_rustls(), - // this is `SSL_set0_chain` (retain ref) - _ => OwnedX509Stack::new(parg as *mut stack_st_X509).to_rustls(), - } - }; - - inner.stage_certificate_chain(chain); - C_INT_SUCCESS as i64 - } - Err(()) => { - log::warn!("unimplemented _SSL_ctrl(..., {cmd}, {larg}, ...)"); - 0 - } + match SslCtrl::try_from(cmd) { + Ok(SslCtrl::Mode) => { + log::warn!("unimplemented SSL_set_mode()"); + 0 } - } else { - 0 - }; - result + Ok(SslCtrl::SetMsgCallbackArg) => { + log::warn!("unimplemented SSL_set_msg_callback_arg()"); + 0 + } + Ok(SslCtrl::SetMaxProtoVersion) => { + log::warn!("unimplemented SSL_set_max_proto_version()"); + 1 + } + Ok(SslCtrl::SetTlsExtHostname) => { + let hostname = try_str!(parg as *const c_char); + ssl.get_mut().set_sni_hostname(hostname) as c_long + } + Ok(SslCtrl::SetChain) => { + let chain = if parg.is_null() { + // this is `SSL_clear_chain_certs` + vec![] + } else { + match larg { + // this is `SSL_set1_chain` (incs ref) + 1 => OwnedX509Stack::new_copy(parg as *mut stack_st_X509).to_rustls(), + // this is `SSL_set0_chain` (retain ref) + _ => OwnedX509Stack::new(parg as *mut stack_st_X509).to_rustls(), + } + }; + + ssl.get_mut().stage_certificate_chain(chain); + C_INT_SUCCESS as i64 + } + Err(()) => { + log::warn!("unimplemented _SSL_ctrl(..., {cmd}, {larg}, ...)"); + 0 + } + } } } entry! { pub fn _SSL_get_options(ssl: *const SSL) -> u64 { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .map(|ssl| ssl.get_options()) - .unwrap_or_default() + ssl.get().get_options() } } entry! { pub fn _SSL_clear_options(ssl: *mut SSL, op: u64) -> u64 { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .map(|mut ssl| ssl.clear_options(op)) - .unwrap_or_default() + ssl.get_mut().clear_options(op) } } entry! { pub fn _SSL_set_options(ssl: *mut SSL, op: u64) -> u64 { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .map(|mut ssl| ssl.set_options(op)) - .unwrap_or_default() + ssl.get_mut().set_options(op) } } @@ -664,38 +571,29 @@ entry! { } }; - match ssl - .lock() - .map_err(|_| Error::cannot_lock()) - .map(|mut ssl| ssl.set_alpn_offer(alpn)) - { - Err(e) => e.raise().into(), - Ok(()) => MysteriouslyOppositeReturnValue::Success, - } + ssl.get_mut().set_alpn_offer(alpn); + MysteriouslyOppositeReturnValue::Success } } entry! { pub fn _SSL_set_connect_state(ssl: *mut SSL) { let ssl = try_clone_arc!(ssl); - let _ = ssl.lock().ok().map(|mut ssl| ssl.set_client_mode()); + ssl.get_mut().set_client_mode(); } } entry! { pub fn _SSL_set_accept_state(ssl: *mut SSL) { let ssl = try_clone_arc!(ssl); - let _ = ssl.lock().ok().map(|mut ssl| ssl.set_server_mode()); + ssl.get_mut().set_server_mode(); } } entry! { pub fn _SSL_is_server(ssl: *const SSL) -> c_int { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .map(|ssl| ssl.is_server()) - .unwrap_or_default() as c_int + ssl.get().is_server() as c_int } } @@ -703,10 +601,7 @@ entry! { pub fn _SSL_set1_host(ssl: *mut SSL, hostname: *const c_char) -> c_int { let ssl = try_clone_arc!(ssl); let maybe_hostname = str_from_cstring(hostname); - ssl.lock() - .ok() - .map(|mut ssl| ssl.set_verify_hostname(maybe_hostname)) - .unwrap_or_default() as c_int + ssl.get_mut().set_verify_hostname(maybe_hostname) as c_int } } @@ -714,23 +609,15 @@ entry! { pub fn _SSL_set_fd(ssl: *mut SSL, fd: c_int) -> c_int { let ssl = try_clone_arc!(ssl); let bio = Bio::new_fd_no_close(fd); - ssl.lock() - .ok() - .map(|mut ssl| { - ssl.set_bio(bio); - true - }) - .unwrap_or_default() as c_int + ssl.get_mut().set_bio(bio); + C_INT_SUCCESS } } entry! { pub fn _SSL_set_bio(ssl: *mut SSL, rbio: *mut BIO, wbio: *mut BIO) { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .map(|mut ssl| ssl.set_bio_pair(Some(rbio), Some(wbio))) - .unwrap_or_default(); + ssl.get_mut().set_bio_pair(Some(rbio), Some(wbio)); } } @@ -740,10 +627,7 @@ entry! { if rbio.is_null() { return; } - ssl.lock() - .ok() - .map(|mut ssl| ssl.set_bio_pair(Some(rbio), None)) - .unwrap_or_default(); + ssl.get_mut().set_bio_pair(Some(rbio), None); } } @@ -753,30 +637,21 @@ entry! { if wbio.is_null() { return; } - ssl.lock() - .ok() - .map(|mut ssl| ssl.set_bio_pair(None, Some(wbio))) - .unwrap_or_default(); + ssl.get_mut().set_bio_pair(None, Some(wbio)); } } entry! { pub fn _SSL_get_rbio(ssl: *const SSL) -> *mut BIO { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .map(|ssl| ssl.get_rbio()) - .unwrap_or_else(ptr::null_mut) + ssl.get().get_rbio() } } entry! { pub fn _SSL_get_wbio(ssl: *const SSL) -> *mut BIO { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .map(|ssl| ssl.get_wbio()) - .unwrap_or_else(ptr::null_mut) + ssl.get().get_wbio() } } @@ -784,13 +659,8 @@ entry! { pub fn _SSL_connect(ssl: *mut SSL) -> c_int { let ssl = try_clone_arc!(ssl); - match ssl - .lock() - .map_err(|_| Error::cannot_lock()) - .and_then(|mut ssl| ssl.connect()) - .map_err(|err| err.raise()) - { - Err(e) => e.into(), + match ssl.get_mut().connect() { + Err(e) => e.raise().into(), Ok(()) => C_INT_SUCCESS, } } @@ -800,13 +670,8 @@ entry! { pub fn _SSL_accept(ssl: *mut SSL) -> c_int { let ssl = try_clone_arc!(ssl); - match ssl - .lock() - .map_err(|_| Error::cannot_lock()) - .and_then(|mut ssl| ssl.accept()) - .map_err(|err| err.raise()) - { - Err(e) => e.into(), + match ssl.get_mut().accept() { + Err(e) => e.raise().into(), Ok(()) => C_INT_SUCCESS, } } @@ -822,13 +687,11 @@ entry! { return ERROR; } - match ssl - .lock() - .map_err(|_| Error::cannot_lock()) - .and_then(|mut ssl| ssl.write(slice)) - .map_err(|err| err.raise()) - { - Err(_e) => ERROR, + match ssl.get_mut().write(slice) { + Err(e) => { + e.raise(); + ERROR + } Ok(written) => written as c_int, } } @@ -840,13 +703,11 @@ entry! { let ssl = try_clone_arc!(ssl, ERROR); let slice = try_mut_slice_int!(buf as *mut u8, num, ERROR); - match ssl - .lock() - .map_err(|_| Error::cannot_lock()) - .and_then(|mut ssl| ssl.read(slice)) - .map_err(|err| err.raise()) - { - Err(_e) => ERROR, + match ssl.get_mut().read(slice) { + Err(e) => { + e.raise(); + ERROR + } Ok(read) => read as c_int, } } @@ -855,7 +716,7 @@ entry! { entry! { pub fn _SSL_want(ssl: *const SSL) -> c_int { let ssl = try_clone_arc!(ssl); - let want = ssl.lock().ok().map(|ssl| ssl.want()).unwrap_or_default(); + let want = ssl.get().want(); if want.read { SSL_READING @@ -876,13 +737,11 @@ entry! { const ERROR: c_int = -1; let ssl = try_clone_arc!(ssl, ERROR); - match ssl - .lock() - .map_err(|_| Error::cannot_lock()) - .and_then(|mut ssl| ssl.try_shutdown()) - .map_err(|err| err.raise()) - { - Err(_e) => ERROR, + match ssl.get_mut().try_shutdown() { + Err(e) => { + e.raise(); + ERROR + } Ok(result) => match result { ShutdownResult::Sent => 0 as c_int, ShutdownResult::Received => 1 as c_int, @@ -894,32 +753,21 @@ entry! { entry! { pub fn _SSL_get_shutdown(ssl: *const SSL) -> c_int { let ssl = try_clone_arc!(ssl); - - ssl.lock().map(|ssl| ssl.get_shutdown()).unwrap_or_default() + ssl.get().get_shutdown() } } entry! { pub fn _SSL_set_shutdown(ssl: *mut SSL, flags: c_int) { let ssl = try_clone_arc!(ssl); - - ssl.lock() - .map_err(|_| Error::cannot_lock()) - .map(|mut ssl| ssl.set_shutdown(flags)) - .map_err(|err| err.raise()) - .unwrap_or_default() + ssl.get_mut().set_shutdown(flags) } } entry! { pub fn _SSL_pending(ssl: *const SSL) -> c_int { let ssl = try_clone_arc!(ssl); - - ssl.lock() - .map_err(|_| Error::cannot_lock()) - .map(|mut ssl| ssl.get_pending_plaintext() as c_int) - .map_err(|err| err.raise()) - .unwrap_or_default() + ssl.get_mut().get_pending_plaintext() as c_int } } @@ -932,11 +780,7 @@ entry! { entry! { pub fn _SSL_get_error(ssl: *const SSL, _ret_code: c_int) -> c_int { let ssl = try_clone_arc!(ssl); - ssl.lock() - .map_err(|_| Error::cannot_lock()) - .map(|mut ssl| ssl.get_error() as c_int) - .map_err(|err| err.raise()) - .unwrap_or_default() + ssl.get_mut().get_error() as c_int } } @@ -948,16 +792,12 @@ entry! { let ssl = try_clone_arc!(ssl); - match ssl.lock().ok().and_then(|mut ssl| { - ssl.get_agreed_alpn().map(|proto| { - unsafe { - // nb. alpn protocols are limited to 255 octets - ptr::write(len, proto.len() as u32); - ptr::write(data, proto.as_ptr()); - }; - }) - }) { - Some(()) => {} + match ssl.get().get_agreed_alpn() { + Some(slice) => unsafe { + // nb. alpn protocols are limited to 255 octets + ptr::write(len, slice.len() as u32); + ptr::write(data, slice.as_ptr()); + }, None => unsafe { ptr::write(len, 0); ptr::write(data, ptr::null()); @@ -969,9 +809,9 @@ entry! { entry! { pub fn _SSL_get_peer_cert_chain(ssl: *const SSL) -> *mut stack_st_X509 { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .and_then(|mut ssl| ssl.get_peer_cert_chain().map(|x509| x509.pointer())) + ssl.get_mut() + .get_peer_cert_chain() + .map(|x509| x509.pointer()) .unwrap_or_else(ptr::null_mut) } } @@ -985,9 +825,9 @@ entry! { entry! { pub fn _SSL_get0_peer_certificate(ssl: *const SSL) -> *mut X509 { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .and_then(|mut ssl| ssl.get_peer_cert().map(|x509| x509.borrow_ref())) + ssl.get_mut() + .get_peer_cert() + .map(|x509| x509.borrow_ref()) .unwrap_or_else(ptr::null_mut) } } @@ -995,9 +835,9 @@ entry! { entry! { pub fn _SSL_get1_peer_certificate(ssl: *const SSL) -> *mut X509 { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .and_then(|mut ssl| ssl.get_peer_cert().map(|x509| x509.up_ref())) + ssl.get_mut() + .get_peer_cert() + .map(|x509| x509.up_ref()) .unwrap_or_else(ptr::null_mut) } } @@ -1005,9 +845,8 @@ entry! { entry! { pub fn _SSL_get_current_cipher(ssl: *const SSL) -> *const SSL_CIPHER { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .and_then(|ssl| ssl.get_negotiated_cipher_suite_id()) + ssl.get() + .get_negotiated_cipher_suite_id() .and_then(crate::SslCipher::find_by_id) .map(|cipher| cipher as *const SSL_CIPHER) .unwrap_or_else(ptr::null) @@ -1017,9 +856,8 @@ entry! { entry! { pub fn _SSL_get_version(ssl: *const SSL) -> *const c_char { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .and_then(|ssl| ssl.get_negotiated_cipher_suite_id()) + ssl.get() + .get_negotiated_cipher_suite_id() .and_then(crate::SslCipher::find_by_id) .map(|cipher| cipher.version.as_ptr()) .unwrap_or_else(ptr::null) @@ -1029,30 +867,21 @@ entry! { entry! { pub fn _SSL_get_verify_result(ssl: *const SSL) -> c_long { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .map(|ssl| ssl.get_last_verification_result()) - .unwrap_or(X509_V_ERR_UNSPECIFIED as i64) + ssl.get().get_last_verification_result() } } entry! { pub fn _SSL_get_certificate(ssl: *const SSL) -> *mut X509 { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .map(|ssl| ssl.get_certificate()) - .unwrap_or(ptr::null_mut()) + ssl.get().get_certificate() } } entry! { pub fn _SSL_get_privatekey(ssl: *const SSL) -> *mut EVP_PKEY { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .map(|ssl| ssl.get_privatekey()) - .unwrap_or(ptr::null_mut()) + ssl.get().get_privatekey() } } @@ -1060,40 +889,28 @@ entry! { // nb. 0 is a reasonable OSSL_HANDSHAKE_STATE, it is OSSL_HANDSHAKE_STATE_TLS_ST_BEFORE pub fn _SSL_get_state(ssl: *const SSL) -> c_uint { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .map(|mut ssl| ssl.handshake_state().into()) - .unwrap_or_default() + ssl.get_mut().handshake_state().into() } } entry! { pub fn _SSL_in_init(ssl: *const SSL) -> c_int { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .map(|mut ssl| ssl.handshake_state().in_init()) - .unwrap_or_default() as c_int + ssl.get_mut().handshake_state().in_init() as c_int } } entry! { pub fn _SSL_in_before(ssl: *const SSL) -> c_int { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .map(|mut ssl| ssl.handshake_state() == HandshakeState::Before) - .unwrap_or_default() as c_int + (ssl.get_mut().handshake_state() == HandshakeState::Before) as c_int } } entry! { pub fn _SSL_is_init_finished(ssl: *const SSL) -> c_int { let ssl = try_clone_arc!(ssl); - ssl.lock() - .ok() - .map(|mut ssl| ssl.handshake_state() == HandshakeState::Finished) - .unwrap_or_default() as c_int + (ssl.get_mut().handshake_state() == HandshakeState::Finished) as c_int } } @@ -1101,13 +918,8 @@ entry! { pub fn _SSL_set_SSL_CTX(ssl: *mut SSL, ctx_ptr: *mut SSL_CTX) -> *mut SSL_CTX { let ssl = try_clone_arc!(ssl); let ctx = try_clone_arc!(ctx_ptr); - ssl.lock() - .ok() - .map(|mut ssl| { - ssl.set_ctx(ctx); - ctx_ptr - }) - .unwrap_or_else(ptr::null_mut) + ssl.get_mut().set_ctx(ctx); + ctx_ptr } } @@ -1122,14 +934,8 @@ entry! { let x509 = OwnedX509::new_incref(x); let ee = CertificateDer::from(x509.der_bytes()); - match ssl - .lock() - .map_err(|_| Error::cannot_lock()) - .map(|mut ssl| ssl.stage_certificate_end_entity(ee)) - { - Err(e) => e.raise().into(), - Ok(()) => C_INT_SUCCESS, - } + ssl.get_mut().stage_certificate_end_entity(ee); + C_INT_SUCCESS } } @@ -1143,11 +949,7 @@ entry! { let pkey = EvpPkey::new_incref(pkey); - match ssl - .lock() - .map_err(|_| Error::cannot_lock()) - .and_then(|mut ssl| ssl.commit_private_key(pkey)) - { + match ssl.get_mut().commit_private_key(pkey) { Err(e) => e.raise().into(), Ok(()) => C_INT_SUCCESS, } @@ -1156,7 +958,7 @@ entry! { impl Castable for SSL { type Ownership = OwnershipArc; - type RustType = Mutex; + type RustType = NotThreadSafe; } type SSL_CIPHER = crate::SslCipher; diff --git a/rustls-libssl/src/lib.rs b/rustls-libssl/src/lib.rs index 50f96aa..18e6f1e 100644 --- a/rustls-libssl/src/lib.rs +++ b/rustls-libssl/src/lib.rs @@ -3,7 +3,7 @@ use core::{mem, ptr}; use std::fs; use std::io::{ErrorKind, Read, Write}; use std::path::PathBuf; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use openssl_probe::ProbeResult; use openssl_sys::{ @@ -37,6 +37,7 @@ mod ffi; #[cfg(miri)] #[allow(non_camel_case_types, dead_code)] mod miri; +mod not_thread_safe; mod sign; mod verifier; mod x509; @@ -334,7 +335,7 @@ pub fn parse_alpn(mut slice: &[u8]) -> Option>> { } struct Ssl { - ctx: Arc>, + ctx: Arc>, raw_options: u64, mode: ConnMode, verify_mode: VerifyMode, @@ -360,7 +361,10 @@ enum ConnState { } impl Ssl { - fn new(ctx: Arc>, inner: &SslContext) -> Result { + fn new( + ctx: Arc>, + inner: &SslContext, + ) -> Result { Ok(Self { ctx, raw_options: inner.raw_options, @@ -379,16 +383,12 @@ impl Ssl { }) } - fn set_ctx(&mut self, ctx: Arc>) { + fn set_ctx(&mut self, ctx: Arc>) { // there are no docs for `SSL_set_SSL_CTX`. it seems the only // meaningful reason to use this is key/certificate switching // (eg, based on SNI). So only bother updating `auth_keys` self.ctx = ctx.clone(); - self.auth_keys = ctx - .lock() - .ok() - .map(|ctx| ctx.auth_keys.clone()) - .unwrap_or_default(); + self.auth_keys = ctx.get().auth_keys.clone(); } fn get_options(&self) -> u64 { @@ -509,11 +509,7 @@ impl Ssl { None => ServerName::try_from("0.0.0.0").unwrap(), }; - let method = self - .ctx - .lock() - .map(|ctx| ctx.method) - .map_err(|_| error::Error::cannot_lock())?; + let method = self.ctx.get().method; let provider = Arc::new(provider::default_provider()); let verifier = Arc::new(verifier::ServerVerifier::new( @@ -563,11 +559,7 @@ impl Ssl { } fn init_server_conn(&mut self) -> Result<(), error::Error> { - let method = self - .ctx - .lock() - .map(|ctx| ctx.method) - .map_err(|_| error::Error::cannot_lock())?; + let method = self.ctx.get().method; let provider = Arc::new(provider::default_provider()); let verifier = Arc::new( @@ -753,7 +745,7 @@ impl Ssl { .unwrap_or_default() } - fn get_agreed_alpn(&mut self) -> Option<&[u8]> { + fn get_agreed_alpn(&self) -> Option<&[u8]> { self.conn().and_then(|conn| conn.alpn_protocol()) } diff --git a/rustls-libssl/src/not_thread_safe.rs b/rustls-libssl/src/not_thread_safe.rs new file mode 100644 index 0000000..1da63e0 --- /dev/null +++ b/rustls-libssl/src/not_thread_safe.rs @@ -0,0 +1,35 @@ +use core::cell::UnsafeCell; + +/// An extremely bad and unsafe laundering of pointer-to-references. +/// +/// OpenSSL's API is specifically not thread-safe. `SSL_CTX` and `SSL` +/// instances must not be shared between threads. See +/// +/// +/// Because the API includes callbacks (that must be called at +/// specific times, and may have side effects) and those callbacks can +/// re-enter the API, just having a `Mutex` here is not workable: +/// `Mutex` is not recursive, and cannot be without being a font of +/// multiple mutable references onto one object. +pub struct NotThreadSafe { + cell: UnsafeCell, +} + +impl NotThreadSafe { + pub fn new(value: T) -> Self { + Self { + cell: UnsafeCell::new(value), + } + } + + pub fn get(&self) -> &T { + // safety: extremely not + unsafe { &*(self.cell.get() as *const T) } + } + + #[allow(clippy::mut_from_ref)] + pub fn get_mut(&self) -> &mut T { + // safety: extremely not + unsafe { &mut *self.cell.get() } + } +}