From d4663961c41a0fb6f81f4a54aefd0fedce49d350 Mon Sep 17 00:00:00 2001
From: Robin Krahl <robin.krahl@ireas.org>
Date: Wed, 23 Jan 2019 04:27:14 +0000
Subject: Return UTF-8 error if libnitrokey returns an invalid string
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Previously, we used lossy UTF-8 conversion.  Yet the user should be
notified if we have a problem instead of silently changing the data.
Therefore, we now return an error if we enocunter an invalid UTF-8
string.  This leads to a change in `get_library_version`’s signature.
---
 CHANGELOG.md |  2 ++
 src/lib.rs   | 17 +++++++++++++----
 src/util.rs  |  9 +++++----
 tests/lib.rs |  2 +-
 4 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c800521..70bd7cb 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -22,6 +22,8 @@
   - Add `Pro::new` and `Storage::new` functions.
 - Implement `From<Pro>` and `From<Storage>` for `DeviceWrapper`.
 - Add `Error::Utf8Error` variant.
+  - Return `Result<Version>` instead of `Version` from `get_library_version`.
+  - Return `Error::Utf8Error` if libnitrokey returns an invalid UTF-8 string.
 
 # v0.3.4 (2019-01-20)
 - Fix authentication methods that assumed that `char` is signed.
diff --git a/src/lib.rs b/src/lib.rs
index 993ec92..a1edb6b 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -168,21 +168,30 @@ pub fn set_log_level(level: LogLevel) {
 
 /// Returns the libnitrokey library version.
 ///
+/// # Errors
+///
+/// - [`Utf8Error`][] if libnitrokey returned an invalid UTF-8 string
+///
 /// # Example
 ///
 /// ```
-/// let version = nitrokey::get_library_version();
+/// # fn main() -> Result<(), nitrokey::Error> {
+/// let version = nitrokey::get_library_version()?;
 /// println!("Using libnitrokey {}", version.git);
+/// #    Ok(())
+/// # }
 /// ```
-pub fn get_library_version() -> Version {
+///
+/// [`Utf8Error`]: enum.Error.html#variant.Utf8Error
+pub fn get_library_version() -> Result<Version> {
     // NK_get_library_version returns a static string, so we don’t have to free the pointer.
     let git = unsafe { nitrokey_sys::NK_get_library_version() };
     let git = if git.is_null() {
         String::new()
     } else {
-        util::owned_str_from_ptr(git)
+        util::owned_str_from_ptr(git)?
     };
     let major = unsafe { nitrokey_sys::NK_get_major_library_version() };
     let minor = unsafe { nitrokey_sys::NK_get_minor_library_version() };
-    Version { git, major, minor }
+    Ok(Version { git, major, minor })
 }
diff --git a/src/util.rs b/src/util.rs
index f8ad9c9..64dde39 100644
--- a/src/util.rs
+++ b/src/util.rs
@@ -28,17 +28,18 @@ pub enum LogLevel {
     DebugL2,
 }
 
-pub fn owned_str_from_ptr(ptr: *const c_char) -> String {
+pub fn owned_str_from_ptr(ptr: *const c_char) -> Result<String, Error> {
     unsafe { CStr::from_ptr(ptr) }
-        .to_string_lossy()
-        .into_owned()
+        .to_str()
+        .map(String::from)
+        .map_err(Error::from)
 }
 
 pub fn result_from_string(ptr: *const c_char) -> Result<String, Error> {
     if ptr.is_null() {
         return Err(Error::UnexpectedError);
     }
-    let s = owned_str_from_ptr(ptr);
+    let s = owned_str_from_ptr(ptr)?;
     unsafe { free(ptr as *mut c_void) };
     // An empty string can both indicate an error or be a valid return value.  In this case, we
     // have to check the last command status to decide what to return.
diff --git a/tests/lib.rs b/tests/lib.rs
index c92e224..d298048 100644
--- a/tests/lib.rs
+++ b/tests/lib.rs
@@ -1,6 +1,6 @@
 #[test]
 fn get_library_version() {
-    let version = nitrokey::get_library_version();
+    let version = nitrokey::get_library_version().unwrap();
 
     assert!(version.git.is_empty() || version.git.starts_with("v"));
     assert!(version.major > 0);
-- 
cgit v1.2.3