Merge pull request #5558 from coolreader18/openssl-probe-no-env

Use non-env-var methods from openssl_probe

Cargo.toml

@@ -192,6 +192,7 @@ unsafe_code = "allow"
 unsafe_op_in_unsafe_fn = "deny"
 missing_unsafe_on_extern = "deny"
 unsafe_attr_outside_unsafe = "deny"
+deprecated_safe_2024 = "deny"
 
 [workspace.lints.clippy]
 perf = "warn"

stdlib/Cargo.toml

@@ -17,8 +17,8 @@ threading = ["rustpython-common/threading", "rustpython-vm/threading"]
 zlib = ["libz-sys", "flate2/zlib"]
 bz2 = ["bzip2"]
 sqlite = ["dep:libsqlite3-sys"]
-ssl = ["openssl", "openssl-sys", "foreign-types-shared"]
-ssl-vendor = ["ssl", "openssl/vendored", "openssl-probe"]
+ssl = ["openssl", "openssl-sys", "foreign-types-shared", "openssl-probe"]
+ssl-vendor = ["ssl", "openssl/vendored"]
 
 [dependencies]
 # rustpython crates

stdlib/build.rs

@@ -1,5 +1,6 @@
 fn main() {
     println!(r#"cargo::rustc-check-cfg=cfg(osslconf, values("OPENSSL_NO_COMP"))"#);
+    println!(r#"cargo::rustc-check-cfg=cfg(openssl_vendored)"#);
 
     #[allow(clippy::unusual_byte_groupings)]
     let ossl_vers = [
@@ -36,4 +37,9 @@ fn main() {
             println!("cargo:rustc-cfg=osslconf=\"{conf}\"");
         }
     }
+    // it's possible for openssl-sys to link against the system openssl under certain conditions,
+    // so let the ssl module know to only perform a probe if we're actually vendored
+    if std::env::var("DEP_OPENSSL_VENDORED").is_ok_and(|s| s == "1") {
+        println!("cargo::rustc-cfg=openssl_vendored")
+    }
 }

stdlib/src/ssl.rs

@@ -1,20 +1,32 @@
 use crate::vm::{builtins::PyModule, PyRef, VirtualMachine};
+use openssl_probe::ProbeResult;
 
 pub(crate) fn make_module(vm: &VirtualMachine) -> PyRef<PyModule> {
-    // if openssl is vendored, it doesn't know the locations of system certificates
-    #[cfg(feature = "ssl-vendor")]
-    if let None | Some("0") = option_env!("OPENSSL_NO_VENDOR") {
-        // TODO: use openssl_probe::probe() instead
-        unsafe { openssl_probe::init_openssl_env_vars() };
-    }
-    openssl::init();
+    // if openssl is vendored, it doesn't know the locations
+    // of system certificates - cache the probe result now.
+    #[cfg(openssl_vendored)]
+    LazyLock::force(&PROBE);
     _ssl::make_module(vm)
 }
 
+// define our own copy of ProbeResult so we can handle the vendor case
+// easily, without having to have a bunch of cfgs
+cfg_if::cfg_if! {
+    if #[cfg(openssl_vendored)] {
+        use std::sync::LazyLock;
+        static PROBE: LazyLock<ProbeResult> = LazyLock::new(openssl_probe::probe);
+        fn probe() -> &'static ProbeResult { &PROBE }
+    } else {
+        fn probe() -> &'static ProbeResult {
+            &ProbeResult { cert_file: None, cert_dir: None }
+        }
+    }
+}
+
 #[allow(non_upper_case_globals)]
 #[pymodule(with(ossl101, windows))]
 mod _ssl {
-    use super::bio;
+    use super::{bio, probe};
     use crate::{
         common::{
             ascii,
@@ -46,10 +58,12 @@ mod _ssl {
         x509::{self, X509Ref, X509},
     };
     use openssl_sys as sys;
+    use rustpython_vm::ospath::OsPath;
     use std::{
         ffi::CStr,
         fmt,
         io::{Read, Write},
+        path::Path,
         time::Instant,
     };
 
@@ -355,21 +369,43 @@ mod _ssl {
             .ok_or_else(|| vm.new_value_error(format!("unknown NID {nid}")))
     }
 
+    fn get_cert_file_dir() -> (&'static Path, &'static Path) {
+        let probe = probe();
+        // on windows, these should be utf8 strings
+        fn path_from_bytes(c: &CStr) -> &Path {
+            #[cfg(unix)]
+            {
+                use std::os::unix::ffi::OsStrExt;
+                std::ffi::OsStr::from_bytes(c.to_bytes()).as_ref()
+            }
+            #[cfg(windows)]
+            {
+                c.to_str().unwrap().as_ref()
+            }
+        }
+        let cert_file = probe.cert_file.as_deref().unwrap_or_else(|| {
+            path_from_bytes(unsafe { CStr::from_ptr(sys::X509_get_default_cert_file()) })
+        });
+        let cert_dir = probe.cert_dir.as_deref().unwrap_or_else(|| {
+            path_from_bytes(unsafe { CStr::from_ptr(sys::X509_get_default_cert_dir()) })
+        });
+        (cert_file, cert_dir)
+    }
+
     #[pyfunction]
-    fn get_default_verify_paths() -> (String, String, String, String) {
-        macro_rules! convert {
-            ($f:ident) => {
-                CStr::from_ptr(sys::$f()).to_string_lossy().into_owned()
-            };
-        }
-        unsafe {
-            (
-                convert!(X509_get_default_cert_file_env),
-                convert!(X509_get_default_cert_file),
-                convert!(X509_get_default_cert_dir_env),
-                convert!(X509_get_default_cert_dir),
-            )
-        }
+    fn get_default_verify_paths(
+        vm: &VirtualMachine,
+    ) -> PyResult<(&'static str, PyObjectRef, &'static str, PyObjectRef)> {
+        let cert_file_env = unsafe { CStr::from_ptr(sys::X509_get_default_cert_file_env()) }
+            .to_str()
+            .unwrap();
+        let cert_dir_env = unsafe { CStr::from_ptr(sys::X509_get_default_cert_dir_env()) }
+            .to_str()
+            .unwrap();
+        let (cert_file, cert_dir) = get_cert_file_dir();
+        let cert_file = OsPath::new_str(cert_file).filename(vm)?;
+        let cert_dir = OsPath::new_str(cert_dir).filename(vm)?;
+        Ok((cert_file_env, cert_file, cert_dir_env, cert_dir))
     }
 
     #[pyfunction(name = "RAND_status")]
@@ -591,9 +627,18 @@ mod _ssl {
 
         #[pymethod]
         fn set_default_verify_paths(&self, vm: &VirtualMachine) -> PyResult<()> {
-            self.builder()
-                .set_default_verify_paths()
-                .map_err(|e| convert_openssl_error(vm, e))
+            cfg_if::cfg_if! {
+                if #[cfg(openssl_vendored)] {
+                    let (cert_file, cert_dir) = get_cert_file_dir();
+                    self.builder()
+                        .load_verify_locations(Some(cert_file), Some(cert_dir))
+                        .map_err(|e| convert_openssl_error(vm, e))
+                } else {
+                    self.builder()
+                        .set_default_verify_paths()
+                        .map_err(|e| convert_openssl_error(vm, e))
+                }
+            }
         }
 
         #[pymethod]
@@ -640,6 +685,12 @@ mod _ssl {
                     vm.new_type_error("cafile, capath and cadata cannot be all omitted".to_owned())
                 );
             }
+            if let Some(cafile) = &args.cafile {
+                cafile.ensure_no_nul(vm)?
+            }
+            if let Some(capath) = &args.capath {
+                capath.ensure_no_nul(vm)?
+            }
 
             #[cold]
             fn invalid_cadata(vm: &VirtualMachine) -> PyBaseExceptionRef {
@@ -648,6 +699,8 @@ mod _ssl {
                 )
             }
 
+            let mut ctx = self.builder();
+
             // validate cadata type and load cadata
             if let Some(cadata) = args.cadata {
                 let certs = match cadata {
@@ -660,7 +713,6 @@ mod _ssl {
                     Either::B(b) => b.with_ref(x509_stack_from_der),
                 };
                 let certs = certs.map_err(|e| convert_openssl_error(vm, e))?;
-                let mut ctx = self.builder();
                 let store = ctx.cert_store_mut();
                 for cert in certs {
                     store
@@ -670,29 +722,11 @@ mod _ssl {
             }
 
             if args.cafile.is_some() || args.capath.is_some() {
-                let cafile = args.cafile.map(|s| s.to_cstring(vm)).transpose()?;
-                let capath = args.capath.map(|s| s.to_cstring(vm)).transpose()?;
-                let ret = unsafe {
-                    let ctx = self.ctx.write();
-                    sys::SSL_CTX_load_verify_locations(
-                        ctx.as_ptr(),
-                        cafile
-                            .as_ref()
-                            .map_or_else(std::ptr::null, |cs| cs.as_ptr()),
-                        capath
-                            .as_ref()
-                            .map_or_else(std::ptr::null, |cs| cs.as_ptr()),
-                    )
-                };
-                if ret != 1 {
-                    let errno = crate::common::os::last_posix_errno();
-                    let err = if errno != 0 {
-                        crate::vm::stdlib::os::errno_err(vm)
-                    } else {
-                        convert_openssl_error(vm, ErrorStack::get())
-                    };
-                    return Err(err);
-                }
+                ctx.load_verify_locations(
+                    args.cafile.as_ref().map(|s| s.as_str().as_ref()),
+                    args.capath.as_ref().map(|s| s.as_str().as_ref()),
+                )
+                .map_err(|e| convert_openssl_error(vm, e))?;
             }
 
             Ok(())

vm/src/stdlib/os.rs

@@ -401,7 +401,8 @@ pub(super) mod _os {
         }
         let key = super::bytes_as_osstr(key, vm)?;
         let value = super::bytes_as_osstr(value, vm)?;
-        env::set_var(key, value);
+        // SAFETY: requirements forwarded from the caller
+        unsafe { env::set_var(key, value) };
         Ok(())
     }
 
@@ -421,7 +422,8 @@ pub(super) mod _os {
             ));
         }
         let key = super::bytes_as_osstr(key, vm)?;
-        env::remove_var(key);
+        // SAFETY: requirements forwarded from the caller
+        unsafe { env::remove_var(key) };
         Ok(())
     }
 

vm/src/utils.rs

@@ -1,6 +1,7 @@
 use crate::{
     builtins::PyStr,
     convert::{ToPyException, ToPyObject},
+    exceptions::cstring_error,
     PyObjectRef, PyResult, VirtualMachine,
 };
 
@@ -17,22 +18,22 @@ impl ToPyObject for std::convert::Infallible {
     }
 }
 
-pub trait ToCString {
-    fn to_cstring(&self, vm: &VirtualMachine) -> PyResult<std::ffi::CString>;
-}
-
-impl ToCString for &str {
-    fn to_cstring(&self, vm: &VirtualMachine) -> PyResult<std::ffi::CString> {
-        std::ffi::CString::new(*self).map_err(|err| err.to_pyexception(vm))
-    }
-}
-
-impl ToCString for PyStr {
+pub trait ToCString: AsRef<str> {
     fn to_cstring(&self, vm: &VirtualMachine) -> PyResult<std::ffi::CString> {
         std::ffi::CString::new(self.as_ref()).map_err(|err| err.to_pyexception(vm))
     }
+    fn ensure_no_nul(&self, vm: &VirtualMachine) -> PyResult<()> {
+        if self.as_ref().as_bytes().contains(&b'\0') {
+            Err(cstring_error(vm))
+        } else {
+            Ok(())
+        }
+    }
 }
 
+impl ToCString for &str {}
+impl ToCString for PyStr {}
+
 pub(crate) fn collection_repr<'a, I>(
     class_name: Option<&str>,
     prefix: &str,