fix: replace String::from_utf16_lossy with String::from_utf16 (#568)

Author: TheBestTvarynka

crates/winscard/src/lib.rs

@@ -184,6 +184,13 @@ impl From<core::str::Utf8Error> for Error {
     }
 }
 
+#[cfg(feature = "std")]
+impl From<std::string::FromUtf16Error> for Error {
+    fn from(value: std::string::FromUtf16Error) -> Self {
+        Error::new(ErrorKind::InvalidParameter, value.to_string())
+    }
+}
+
 #[cfg(feature = "std")]
 impl From<std::ffi::NulError> for Error {
     fn from(value: std::ffi::NulError) -> Self {

ffi/src/sspi/credentials_attributes.rs

@@ -108,9 +108,10 @@ pub unsafe fn extract_kdc_proxy_settings(p_buffer: NonNull<c_void>) -> Result<Kd
     // SAFETY:
     // - `proxy_server_ptr` is guaranteed to be non-null due to the prior check.
     // - `proxy_server_length` is valid.
-    let proxy_server = String::from_utf16_lossy(unsafe {
+    let proxy_server = String::from_utf16(unsafe {
         from_raw_parts(proxy_server_ptr, *proxy_server_length as usize / size_of::<SecWChar>())
-    });
+    })
+    .map_err(Error::from)?;
 
     let client_tls_cred = if *client_tls_cred_offset != 0 && *client_tls_cred_length != 0 {
         // SAFETY:
@@ -129,7 +130,10 @@ pub unsafe fn extract_kdc_proxy_settings(p_buffer: NonNull<c_void>) -> Result<Kd
         // - `client_tls_cred_ptr` is guaranteed to be non-null due to the prior check.
         // - `client_tls_cred_length` is valid.
         let client_tls_cred_data = unsafe { from_raw_parts(client_tls_cred_ptr, *client_tls_cred_length as usize) };
-        Some(String::from_utf16_lossy(client_tls_cred_data))
+
+        let client_tls_cred = String::from_utf16(client_tls_cred_data).map_err(Error::from)?;
+
+        Some(client_tls_cred)
     } else {
         None
     };

ffi/src/sspi/scard_cert.rs

@@ -53,7 +53,7 @@ pub fn smart_card_info(username: &[u8], pkcs11_module: &Path) -> Result<SystemSm
     let pkcs11 = Pkcs11::new(pkcs11_module)?;
     pkcs11.initialize(CInitializeArgs::OsThreads)?;
 
-    let username = utf16_bytes_to_utf8_string(username);
+    let username = utf16_bytes_to_utf8_string(username)?;
 
     for slot in pkcs11.get_slots_with_token()? {
         let session = pkcs11.open_ro_session(slot)?;

ffi/src/sspi/sec_handle.rs

@@ -463,11 +463,13 @@ pub unsafe extern "system" fn AcquireCredentialsHandleW(
         check_null!(p_auth_data);
         check_null!(ph_credential);
 
-        // SAFETY:
-        // - `psz_package` is guaranteed to be non-null due to the prior check.
-        // - The memory region `psz_package` contains a valid null-terminator at the end of string.
-        // - The memory region `psz_package` points to is valid for reads of bytes up to and including null-terminator.
-        let security_package_name = unsafe { c_w_str_to_string(psz_package) };
+        let security_package_name = try_execute!(
+            // SAFETY:
+            // - `psz_package` is guaranteed to be non-null due to the prior check.
+            // - The memory region `psz_package` contains a valid null-terminator at the end of string.
+            // - The memory region `psz_package` points to is valid for reads of bytes up to and including null-terminator.
+            unsafe { c_w_str_to_string(psz_package) }.map_err(Error::from)
+        );
         try_execute!(verify_security_package(&security_package_name));
 
         debug!(?security_package_name);
@@ -737,11 +739,13 @@ pub unsafe extern "system" fn InitializeSecurityContextW(
         let service_principal = if p_target_name.is_null() {
             String::new()
         } else {
-            // SAFETY:
-            // - `p_target_name` is guaranteed to be non-null due to the prior check.
-            // - The memory region `p_target_name` contains a valid null-terminator at the end of string.
-            // - The memory region `p_target_name` points to is valid for reads of bytes up to and including null-terminator.
-            unsafe { c_w_str_to_string(p_target_name) }
+            try_execute!(
+                // SAFETY:
+                // - `p_target_name` is guaranteed to be non-null due to the prior check.
+                // - The memory region `p_target_name` contains a valid null-terminator at the end of string.
+                // - The memory region `p_target_name` points to is valid for reads of bytes up to and including null-terminator.
+                unsafe { c_w_str_to_string(p_target_name) }.map_err(Error::from)
+            )
         };
         debug!(?service_principal, "Target name (SPN)");
 
@@ -1334,11 +1338,13 @@ pub unsafe extern "system" fn SetCredentialsAttributesW(
         };
 
         if ul_attribute == SECPKG_CRED_ATTR_NAMES {
-            // SAFETY:
-            // - `p_buffer` is guaranteed to be non-null due to the prior check.
-            // - The memory region `p_buffer` contains a valid null-terminator at the end of string.
-            // - The memory region `p_buffer` points to is valid for reads of bytes up to and including null-terminator.
-            let workstation = unsafe { c_w_str_to_string(p_buffer as *const _) };
+            let workstation = try_execute!(
+                // SAFETY:
+                // - `p_buffer` is guaranteed to be non-null due to the prior check.
+                // - The memory region `p_buffer` contains a valid null-terminator at the end of string.
+                // - The memory region `p_buffer` points to is valid for reads of bytes up to and including null-terminator.
+                unsafe { c_w_str_to_string(p_buffer as *const _) }.map_err(Error::from)
+            );
 
             credentials_handle.attributes.workstation = Some(workstation);
 
@@ -1355,11 +1361,13 @@ pub unsafe extern "system" fn SetCredentialsAttributesW(
             0
         } else if ul_attribute == SECPKG_CRED_ATTR_KDC_URL {
             let cred_attr = p_buffer.cast::<SecPkgCredentialsKdcUrlW>();
-            // SAFETY:
-            // - `p_buffer` is guaranteed to be non-null due to the prior check.
-            // - The memory region `p_buffer` contains a valid null-terminator at the end of string.
-            // - The memory region `p_buffer` points to is valid for reads of bytes up to and including null-terminator.
-            let kdc_url = unsafe { c_w_str_to_string((*cred_attr).kdc_url.cast_const()) };
+            let kdc_url = try_execute!(
+                // SAFETY:
+                // - `p_buffer` is guaranteed to be non-null due to the prior check.
+                // - The memory region `p_buffer` contains a valid null-terminator at the end of string.
+                // - The memory region `p_buffer` points to is valid for reads of bytes up to and including null-terminator.
+                unsafe { c_w_str_to_string((*cred_attr).kdc_url.cast_const()) }.map_err(Error::from)
+            );
             credentials_handle.attributes.kdc_url = Some(kdc_url);
 
             0
@@ -1545,32 +1553,42 @@ pub unsafe extern "system" fn ChangeAccountPasswordW(
         check_null!(psz_new_password);
         check_null!(p_output);
 
-        // SAFETY:
-        // - `psz_package_name` is guaranteed to be non-null due to the prior check.
-        // - The memory region `psz_package_name` contains a valid null-terminator at the end of string.
-        // - The memory region `psz_package_name` points to is valid for reads of bytes up to and including null-terminator.
-        let mut security_package_name = unsafe { c_w_str_to_string(psz_package_name) };
+        let mut security_package_name = try_execute!(
+            // SAFETY:
+            // - `psz_package_name` is guaranteed to be non-null due to the prior check.
+            // - The memory region `psz_package_name` contains a valid null-terminator at the end of string.
+            // - The memory region `psz_package_name` points to is valid for reads of bytes up to and including null-terminator.
+            unsafe { c_w_str_to_string(psz_package_name) }.map_err(Error::from)
+        );
 
-        // SAFETY:
-        // - `psz_domain_name` is guaranteed to be non-null due to the prior check.
-        // - The memory region `psz_domain_name` contains a valid null-terminator at the end of string.
-        // - The memory region `psz_domain_name` points to is valid for reads of bytes up to and including null-terminator.
-        let mut domain = unsafe { c_w_str_to_string(psz_domain_name) };
-        // SAFETY:
-        // - `psz_account_name` is guaranteed to be non-null due to the prior check.
-        // - The memory region `psz_account_name` contains a valid null-terminator at the end of string.
-        // - The memory region `psz_account_name` points to is valid for reads of bytes up to and including null-terminator.
-        let mut username = unsafe { c_w_str_to_string(psz_account_name) };
-        // SAFETY:
-        // - `psz_old_password` is guaranteed to be non-null due to the prior check.
-        // - The memory region `psz_old_password` contains a valid null-terminator at the end of string.
-        // - The memory region `psz_old_password` points to is valid for reads of bytes up to and including null-terminator.
-        let mut password = Secret::new(unsafe { c_w_str_to_string(psz_old_password) });
-        // SAFETY:
-        // - `psz_new_password` is guaranteed to be non-null due to the prior check.
-        // - The memory region `psz_new_password` contains a valid null-terminator at the end of string.
-        // - The memory region `psz_new_password` points to is valid for reads of bytes up to and including null-terminator.
-        let mut new_password = Secret::new(unsafe { c_w_str_to_string(psz_new_password) });
+        let mut domain = try_execute!(
+            // SAFETY:
+            // - `psz_domain_name` is guaranteed to be non-null due to the prior check.
+            // - The memory region `psz_domain_name` contains a valid null-terminator at the end of string.
+            // - The memory region `psz_domain_name` points to is valid for reads of bytes up to and including null-terminator.
+            unsafe { c_w_str_to_string(psz_domain_name) }.map_err(Error::from)
+        );
+        let mut username = try_execute!(
+            // SAFETY:
+            // - `psz_account_name` is guaranteed to be non-null due to the prior check.
+            // - The memory region `psz_account_name` contains a valid null-terminator at the end of string.
+            // - The memory region `psz_account_name` points to is valid for reads of bytes up to and including null-terminator.
+            unsafe { c_w_str_to_string(psz_account_name) }.map_err(Error::from)
+        );
+        let mut password = Secret::new(try_execute!(
+            // SAFETY:
+            // - `psz_old_password` is guaranteed to be non-null due to the prior check.
+            // - The memory region `psz_old_password` contains a valid null-terminator at the end of string.
+            // - The memory region `psz_old_password` points to is valid for reads of bytes up to and including null-terminator.
+            unsafe { c_w_str_to_string(psz_old_password) }.map_err(Error::from)
+        ));
+        let mut new_password = Secret::new(try_execute!(
+            // SAFETY:
+            // - `psz_new_password` is guaranteed to be non-null due to the prior check.
+            // - The memory region `psz_new_password` contains a valid null-terminator at the end of string.
+            // - The memory region `psz_new_password` points to is valid for reads of bytes up to and including null-terminator.
+            unsafe { c_w_str_to_string(psz_new_password) }.map_err(Error::from)
+        ));
 
         // SAFETY:
         // * `security_package_name' is a `String`.

ffi/src/sspi/sec_pkg_info.rs

@@ -2,7 +2,7 @@ use std::ffi::CStr;
 use std::mem::size_of;
 use std::ptr::copy_nonoverlapping;
 
-use sspi::{enumerate_security_packages, str_to_w_buff, PackageInfo, KERBEROS_VERSION};
+use sspi::{enumerate_security_packages, str_to_w_buff, Error, PackageInfo, KERBEROS_VERSION};
 #[cfg(windows)]
 use symbol_rename_macro::rename_symbol;
 
@@ -448,11 +448,13 @@ pub unsafe extern "system" fn QuerySecurityPackageInfoW(
         check_null!(p_package_name);
         check_null!(pp_package_info);
 
-        // SAFETY:
-        // - `p_package_name` is guaranteed to be non-null due to the prior check.
-        // - The memory region `p_package_name` contains a valid null-terminator at the end of string.
-        // - The memory region `p_package_name` points to is valid for reads of bytes up to and including null-terminator.
-        let pkg_name = unsafe { c_w_str_to_string(p_package_name) };
+        let pkg_name = try_execute!(
+            // SAFETY:
+            // - `p_package_name` is guaranteed to be non-null due to the prior check.
+            // - The memory region `p_package_name` contains a valid null-terminator at the end of string.
+            // - The memory region `p_package_name` points to is valid for reads of bytes up to and including null-terminator.
+            unsafe { c_w_str_to_string(p_package_name) }.map_err(Error::from)
+        );
 
         let pkg_info: RawSecPkgInfoW = try_execute!(enumerate_security_packages())
             .into_iter()

ffi/src/sspi/sec_winnt_auth_identity.rs

@@ -478,13 +478,18 @@ pub unsafe fn auth_data_to_identity_buffers_w(
             let auth_data = unsafe { auth_data.as_ref() }.expect("auth_data pointer should not be null");
 
             if !auth_data.package_list.is_null() && auth_data.package_list_length > 0 {
-                // SAFETY: `package_list` is not null due to a prior check.
-                *package_list = Some(String::from_utf16_lossy(unsafe {
-                    from_raw_parts(
-                        auth_data.package_list,
-                        usize::try_from(auth_data.package_list_length).unwrap(),
+                *package_list = Some(
+                    String::from_utf16(
+                        // SAFETY: `package_list` is not null due to a prior check.
+                        unsafe {
+                            from_raw_parts(
+                                auth_data.package_list,
+                                usize::try_from(auth_data.package_list_length).unwrap(),
+                            )
+                        },
                     )
-                }));
+                    .map_err(Error::from)?,
+                );
             }
 
             (
@@ -1263,18 +1268,21 @@ mod tests {
 
             assert_eq!(
                 "user",
-                String::from_utf16_lossy(from_raw_parts((*identity).user, (*identity).user_length as usize))
+                String::from_utf16(from_raw_parts((*identity).user, (*identity).user_length as usize))
+                    .expect("user is a correct utf-16 string")
             );
             assert_eq!(
                 "pass",
-                String::from_utf16_lossy(from_raw_parts(
+                String::from_utf16(from_raw_parts(
                     (*identity).password,
                     (*identity).password_length as usize
                 ))
+                .expect("password is a correct utf-16 string")
             );
             assert_eq!(
                 "domain",
-                String::from_utf16_lossy(from_raw_parts((*identity).domain, (*identity).domain_length as usize))
+                String::from_utf16(from_raw_parts((*identity).domain, (*identity).domain_length as usize))
+                    .expect("domain is a correct utf-16 string")
             );
 
             let status = SspiFreeAuthIdentity(identity as *mut _);

ffi/src/utils.rs

@@ -1,4 +1,5 @@
 use std::slice::from_raw_parts;
+use std::string::FromUtf16Error;
 
 use libc::c_char;
 
@@ -13,7 +14,7 @@ pub(crate) fn into_raw_ptr<T>(value: T) -> *mut T {
 /// Behavior is undefined is any of the following conditions are violated:
 ///
 /// * `s` must be a [valid], null-terminated C string.
-pub(crate) unsafe fn c_w_str_to_string(s: *const u16) -> String {
+pub(crate) unsafe fn c_w_str_to_string(s: *const u16) -> Result<String, FromUtf16Error> {
     let mut len = 0;
 
     // SAFETY: `s` is a valid, null-terminated C string.
@@ -22,7 +23,7 @@ pub(crate) unsafe fn c_w_str_to_string(s: *const u16) -> String {
     }
 
     // SAFETY: `s` is a valid, null-terminated C string.
-    String::from_utf16_lossy(unsafe { from_raw_parts(s, len) })
+    String::from_utf16(unsafe { from_raw_parts(s, len) })
 }
 
 /// The returned length includes the null terminator char.

ffi/src/winscard/scard.rs

@@ -155,11 +155,13 @@ pub unsafe extern "system" fn SCardConnectW(
     check_null!(ph_card);
     check_null!(pdw_active_protocol);
 
-    // SAFETY:
-    // - `sz_reader` is guaranteed to be non-null due to the prior check.
-    // - The memory region `sz_reader` contains a valid null-terminator at the end of string.
-    // - The memory region `sz_reader` points to is valid for reads of bytes up to and including null-terminator.
-    let reader_name = unsafe { c_w_str_to_string(sz_reader) };
+    let reader_name = try_execute!(
+        // SAFETY:
+        // - `sz_reader` is guaranteed to be non-null due to the prior check.
+        // - The memory region `sz_reader` contains a valid null-terminator at the end of string.
+        // - The memory region `sz_reader` points to is valid for reads of bytes up to and including null-terminator.
+        unsafe { c_w_str_to_string(sz_reader) }.map_err(Error::from)
+    );
 
     try_execute!(
         // SAFETY: