fix: replace String::from_utf16_lossy with String::from_utf16
#568
Conversation
| }) | ||
| .map_err(Error::from)?; |
There was a problem hiding this comment.
nitpick: ? should already insert the glue code for the From conversion.
| }) | |
| .map_err(Error::from)?; | |
| })?; |
note: I highly recommend we don’t use some::Result or use some::Error because the naming is confusing at usage sites. I.e.: for clarity, I think it’s better to use explicit sspi::Result and sspi::Error so we have minimal context when reading the code. (This is the suggested style in IronRDP.)
There was a problem hiding this comment.
🤔
Yes, you are right. I do not know why I added it. There may have been a type inference issue at some point, and I forgot to clean up the code afterward.
| *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(), | ||
| ) | ||
| }, | ||
| ) | ||
| })); |
There was a problem hiding this comment.
nitpick: Thats’ a lot of nested items. We could consider breaking down things a little bit.
| use uuid::Uuid; | ||
| use winscard::winscard::{CurrentState, ReaderState, WinScardContext}; | ||
| use winscard::{ErrorKind, ScardContext as PivCardContext, SmartCardInfo, WinScardResult}; | ||
| use winscard::{Error, ErrorKind, ScardContext as PivCardContext, SmartCardInfo, WinScardResult}; |
There was a problem hiding this comment.
nitpick: My preferred style would be to not import Error nor ErrorKind, and avoid as renamings because it’s then not obvious what we are talking about locally.
From
String::from_utf16_lossydocumentation:String::from_utf16_lossychanges the credentials when the input data is not a UTF-8 valid buffer. We should not do this because it could result in invalid credentials being sent to the target server.It's either to use
StringorOsString, but notString::from_utf16_lossy.