From 3c8092cd937d6f449b1959eab9e7e15549970d85 Mon Sep 17 00:00:00 2001
From: Robin Krahl <robin.krahl@ireas.org>
Date: Wed, 16 Jan 2019 02:52:58 +0000
Subject: Fix result_from_string for empty return values

An empty string returned from a libnitrokey function can either indicate
an error or be a valid return value.  Previously, we assumed that it
indicates an error.  But instead, we should check the last command
status and use it to decide whether to return the empty string or an
error code.

This breaks the unit tests that assume that empty strings cause errors.
These will be fixed in the next patches.
---
 CHANGELOG.md | 2 ++
 TODO.md      | 1 -
 src/util.rs  | 7 +++++--
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 51d2b1e..bae77e0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,6 +3,8 @@
   `Storage` struct.
 - Use `rand_os` instead of `rand` for random data creation.
   - (Re-)add `CommandError::RngError` variant.
+- Account for the possibility that an empty string returned by libnitrokey can
+  not only indicate an error but also be a valid return value.
 
 # v0.3.2 (2019-01-12)
 - Make three additional error codes known: `CommandError::StringTooLong`,
diff --git a/TODO.md b/TODO.md
index 7d7ff1a..28bd3b8 100644
--- a/TODO.md
+++ b/TODO.md
@@ -11,7 +11,6 @@
 - Find a nicer syntax for the `write_config` test.
 - Prevent construction of internal types.
 - More specific error checking in the tests.
-- Differentiate empty strings and errors (see `result_from_string`).
 - Check integer conversions.
 - Consider implementing `Into<CommandError>` for `(Device, CommandError)`
 - Lock password safe in `PasswordSafe::drop()` (see [nitrokey-storage-firmware
diff --git a/src/util.rs b/src/util.rs
index 54062a5..567c478 100644
--- a/src/util.rs
+++ b/src/util.rs
@@ -83,10 +83,13 @@ pub fn result_from_string(ptr: *const c_char) -> Result<String, CommandError> {
     unsafe {
         let s = owned_str_from_ptr(ptr);
         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.
         if s.is_empty() {
-            return Err(get_last_error());
+            get_last_result().map(|_| s)
+        } else {
+            Ok(s)
         }
-        return Ok(s);
     }
 }
 
-- 
cgit v1.2.3