Merge branch '110-mixed_strings'

Make sure all C API strings are deallocable.

Tested with Python's 3.6.5 'pytest -sv test_{pro,storage}.py' on Storage v0.50,
Ubuntu 18.04

Closes #110

6 files changed, 109 insertions, 49 deletions

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 06fcfac..3dacb48 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -185,6 +185,7 @@ IF (COMPILE_TESTS)
                 unittest/test1.cc
                 unittest/test_issues.cc
                 unittest/test_multiple_devices.cc
+                unittest/test_strdup.cpp
         )
 
     foreach(testsourcefile ${TESTS} )
diff --git a/NK_C_API.cc b/NK_C_API.cc
index b245940..8e005b8 100644
--- a/NK_C_API.cc
+++ b/NK_C_API.cc
@@ -71,7 +71,7 @@ uint8_t * get_with_array_result(T func){
 }
 
 template <typename T>
-const char* get_with_string_result(T func){
+char* get_with_string_result(T func){
     NK_last_command_status = 0;
     try {
         return func();
@@ -85,7 +85,7 @@ const char* get_with_string_result(T func){
     catch (const DeviceCommunicationException &deviceException){
       NK_last_command_status = 256-deviceException.getType();
     }
-    return "";
+    return strndup("", MAXIMUM_STR_REPLY_LENGTH);
 }
 
 template <typename T>
@@ -243,17 +243,17 @@ extern "C" {
 	}
 
 
-	NK_C_API const char * NK_status() {
+	NK_C_API char * NK_status() {
 		auto m = NitrokeyManager::instance();
 		return get_with_string_result([&]() {
 			string && s = m->get_status_as_string();
-			char * rs = strndup(s.c_str(), max_string_field_length);
+			char * rs = strndup(s.c_str(), MAXIMUM_STR_REPLY_LENGTH);
 			clear_string(s);
 			return rs;
 		});
 	}
 
-	NK_C_API const char * NK_device_serial_number() {
+	NK_C_API char * NK_device_serial_number() {
 		auto m = NitrokeyManager::instance();
 		return get_with_string_result([&]() {
 			string && s = m->get_serial_number();
@@ -263,11 +263,11 @@ extern "C" {
 		});
 	}
 
-	NK_C_API const char * NK_get_hotp_code(uint8_t slot_number) {
+	NK_C_API char * NK_get_hotp_code(uint8_t slot_number) {
 		return NK_get_hotp_code_PIN(slot_number, "");
 	}
 
-	NK_C_API const char * NK_get_hotp_code_PIN(uint8_t slot_number, const char *user_temporary_password) {
+	NK_C_API char * NK_get_hotp_code_PIN(uint8_t slot_number, const char *user_temporary_password) {
 		auto m = NitrokeyManager::instance();
 		return get_with_string_result([&]() {
 			string && s = m->get_HOTP_code(slot_number, user_temporary_password);
@@ -277,12 +277,12 @@ extern "C" {
 		});
 	}
 
-	NK_C_API const char * NK_get_totp_code(uint8_t slot_number, uint64_t challenge, uint64_t last_totp_time,
+	NK_C_API char * NK_get_totp_code(uint8_t slot_number, uint64_t challenge, uint64_t last_totp_time,
 		uint8_t last_interval) {
 		return NK_get_totp_code_PIN(slot_number, challenge, last_totp_time, last_interval, "");
 	}
 
-	NK_C_API const char * NK_get_totp_code_PIN(uint8_t slot_number, uint64_t challenge, uint64_t last_totp_time,
+	NK_C_API char * NK_get_totp_code_PIN(uint8_t slot_number, uint64_t challenge, uint64_t last_totp_time,
 		uint8_t last_interval, const char *user_temporary_password) {
 		auto m = NitrokeyManager::instance();
 		return get_with_string_result([&]() {
@@ -327,14 +327,14 @@ extern "C" {
 		});
 	}
 
-	NK_C_API const char* NK_get_totp_slot_name(uint8_t slot_number) {
+	NK_C_API char* NK_get_totp_slot_name(uint8_t slot_number) {
 		auto m = NitrokeyManager::instance();
 		return get_with_string_result([&]() {
 			const auto slot_name = m->get_totp_slot_name(slot_number);
 			return slot_name;
 		});
 	}
-	NK_C_API const char* NK_get_hotp_slot_name(uint8_t slot_number) {
+	NK_C_API char* NK_get_hotp_slot_name(uint8_t slot_number) {
 		auto m = NitrokeyManager::instance();
 		return get_with_string_result([&]() {
 			const auto slot_name = m->get_hotp_slot_name(slot_number);
@@ -417,20 +417,20 @@ extern "C" {
 		});
 	}
 
-	NK_C_API const char *NK_get_password_safe_slot_name(uint8_t slot_number) {
+	NK_C_API char *NK_get_password_safe_slot_name(uint8_t slot_number) {
 		auto m = NitrokeyManager::instance();
 		return get_with_string_result([&]() {
 			return m->get_password_safe_slot_name(slot_number);
 		});
 	}
 
-	NK_C_API const char *NK_get_password_safe_slot_login(uint8_t slot_number) {
+	NK_C_API char *NK_get_password_safe_slot_login(uint8_t slot_number) {
 		auto m = NitrokeyManager::instance();
 		return get_with_string_result([&]() {
 			return m->get_password_safe_slot_login(slot_number);
 		});
 	}
-	NK_C_API const char *NK_get_password_safe_slot_password(uint8_t slot_number) {
+	NK_C_API char *NK_get_password_safe_slot_password(uint8_t slot_number) {
 		auto m = NitrokeyManager::instance();
 		return get_with_string_result([&]() {
 			return m->get_password_safe_slot_password(slot_number);
@@ -589,14 +589,14 @@ extern "C" {
 		});
 	}
 
-	NK_C_API const char* NK_get_status_storage_as_string() {
+	NK_C_API char* NK_get_status_storage_as_string() {
 		auto m = NitrokeyManager::instance();
 		return get_with_string_result([&]() {
 			return m->get_status_storage_as_string();
 		});
 	}
 
-	NK_C_API const char* NK_get_SD_usage_data_as_string() {
+	NK_C_API char* NK_get_SD_usage_data_as_string() {
 		auto m = NitrokeyManager::instance();
 		return get_with_string_result([&]() {
 			return m->get_SD_usage_data_as_string();
@@ -631,7 +631,7 @@ extern "C" {
 		});
 	}
 
-	NK_C_API const char* NK_list_devices_by_cpuID() {
+	NK_C_API char* NK_list_devices_by_cpuID() {
 		auto nm = NitrokeyManager::instance();
 		return get_with_string_result([&]() {
 			auto v = nm->list_devices_by_cpuID();
@@ -640,7 +640,7 @@ extern "C" {
 				res += a+";";
 			}
 			if (res.size()>0) res.pop_back(); // remove last delimiter char
-			return strndup(res.c_str(), 8192); //this buffer size sets limit to over 200 devices ID's
+			return strndup(res.c_str(), MAXIMUM_STR_REPLY_LENGTH);
 		});
 	}
 
diff --git a/NK_C_API.h b/NK_C_API.h
index 3a6aa2a..222e5e1 100644
--- a/NK_C_API.h
+++ b/NK_C_API.h
@@ -28,13 +28,15 @@
 #ifdef _MSC_VER
 #define NK_C_API __declspec(dllexport)
 #else
-#define NK_C_API 
+#define NK_C_API
 #endif
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 
+  static const int MAXIMUM_STR_REPLY_LENGTH = 8192;
+
         /**
          * The Nitrokey device models supported by the API.
          */
@@ -91,13 +93,13 @@ extern "C" {
 	 * Return the debug status string. Debug purposes.
 	 * @return command processing error code
 	 */
-	NK_C_API const char * NK_status();
+	NK_C_API char * NK_status();
 
 	/**
 	 * Return the device's serial number string in hex.
 	 * @return string device's serial number in hex
 	 */
-	NK_C_API const char * NK_device_serial_number();
+	NK_C_API char * NK_device_serial_number();
 
 	/**
 	 * Get last command processing status. Useful for commands which returns the results of their own and could not return
@@ -183,14 +185,14 @@ extern "C" {
 	 * @param slot_number TOTP slot number, slot_number<15
 	 * @return char[20] the name of the slot
 	 */
-	NK_C_API const char * NK_get_totp_slot_name(uint8_t slot_number);
+	NK_C_API char * NK_get_totp_slot_name(uint8_t slot_number);
 
 	/**
 	 *
 	 * @param slot_number HOTP slot number, slot_number<3
 	 * @return char[20] the name of the slot
 	 */
-	NK_C_API const char * NK_get_hotp_slot_name(uint8_t slot_number);
+	NK_C_API char * NK_get_hotp_slot_name(uint8_t slot_number);
 
 	/**
 	 * Erase HOTP slot data from the device
@@ -249,7 +251,7 @@ extern "C" {
 	 * @param slot_number HOTP slot number, slot_number<3
 	 * @return HOTP code
 	 */
-	NK_C_API const char * NK_get_hotp_code(uint8_t slot_number);
+	NK_C_API char * NK_get_hotp_code(uint8_t slot_number);
 
 	/**
 	 * Get HOTP code from the device (PIN protected)
@@ -258,7 +260,7 @@ extern "C" {
 	 * otherwise should be set to empty string - ''
 	 * @return HOTP code
 	 */
-	NK_C_API const char * NK_get_hotp_code_PIN(uint8_t slot_number, const char *user_temporary_password);
+	NK_C_API char * NK_get_hotp_code_PIN(uint8_t slot_number, const char *user_temporary_password);
 
 	/**
 	 * Get TOTP code from the device
@@ -268,7 +270,7 @@ extern "C" {
 	 * @param last_interval last interval
 	 * @return TOTP code
 	 */
-	NK_C_API const char * NK_get_totp_code(uint8_t slot_number, uint64_t challenge, uint64_t last_totp_time,
+	NK_C_API char * NK_get_totp_code(uint8_t slot_number, uint64_t challenge, uint64_t last_totp_time,
 		uint8_t last_interval);
 
 	/**
@@ -281,7 +283,7 @@ extern "C" {
 	 * otherwise should be set to empty string - ''
 	 * @return TOTP code
 	 */
-	NK_C_API const char * NK_get_totp_code_PIN(uint8_t slot_number, uint64_t challenge,
+	NK_C_API char * NK_get_totp_code_PIN(uint8_t slot_number, uint64_t challenge,
 		uint64_t last_totp_time, uint8_t last_interval,
 		const char *user_temporary_password);
 
@@ -343,21 +345,21 @@ extern "C" {
 	 * @param slot_number password safe slot number, slot_number<16
 	 * @return slot name
 	 */
-	NK_C_API const char *NK_get_password_safe_slot_name(uint8_t slot_number);
+	NK_C_API char *NK_get_password_safe_slot_name(uint8_t slot_number);
 
 	/**
 	 * Get password safe slot login
 	 * @param slot_number password safe slot number, slot_number<16
 	 * @return login from the PWS slot
 	 */
-	NK_C_API const char *NK_get_password_safe_slot_login(uint8_t slot_number);
+	NK_C_API char *NK_get_password_safe_slot_login(uint8_t slot_number);
 
 	/**
 	 * Get the password safe slot password
 	 * @param slot_number password safe slot number, slot_number<16
 	 * @return password from the PWS slot
 	 */
-	NK_C_API const char *NK_get_password_safe_slot_password(uint8_t slot_number);
+	NK_C_API char *NK_get_password_safe_slot_password(uint8_t slot_number);
 
 	/**
 	 * Write password safe data to the slot
@@ -582,7 +584,7 @@ extern "C" {
 	 * Storage only
 	 * @return string with devices attributes
 	 */
-	NK_C_API const char* NK_get_status_storage_as_string();
+	NK_C_API char* NK_get_status_storage_as_string();
 
 	/**
 	 * Get SD card usage attributes as string.
@@ -590,7 +592,7 @@ extern "C" {
 	 * Storage only
 	 * @return string with SD card usage attributes
 	 */
-	NK_C_API const char* NK_get_SD_usage_data_as_string();
+	NK_C_API char* NK_get_SD_usage_data_as_string();
 
 	/**
 	 * Get progress value of current long operation.
@@ -614,7 +616,7 @@ extern "C" {
  * @example Example of returned data: '00005d19:dacc2cb4_p_0001:0010:02;000037c7:4cf12445_p_0001:000f:02;0001:000c:02'
  * @return string delimited id's of connected devices
  */
-	NK_C_API const char* NK_list_devices_by_cpuID();
+	NK_C_API char* NK_list_devices_by_cpuID();
 
 
 /**
diff --git a/NitrokeyManager.cc b/NitrokeyManager.cc
index 2ca183c..085bf78 100644
--- a/NitrokeyManager.cc
+++ b/NitrokeyManager.cc
@@ -630,12 +630,12 @@ using nitrokey::misc::strcpyT;
       auto resp = WriteToTOTPSlot::CommandTransaction::run(device, payload);
     }
 
-    const char * NitrokeyManager::get_totp_slot_name(uint8_t slot_number) {
+    char * NitrokeyManager::get_totp_slot_name(uint8_t slot_number) {
         if (!is_valid_totp_slot_number(slot_number)) throw InvalidSlotException(slot_number);
         slot_number = get_internal_slot_number_for_totp(slot_number);
         return get_slot_name(slot_number);
     }
-    const char * NitrokeyManager::get_hotp_slot_name(uint8_t slot_number) {
+    char * NitrokeyManager::get_hotp_slot_name(uint8_t slot_number) {
         if (!is_valid_hotp_slot_number(slot_number)) throw InvalidSlotException(slot_number);
         slot_number = get_internal_slot_number_for_hotp(slot_number);
         return get_slot_name(slot_number);
@@ -643,7 +643,7 @@ using nitrokey::misc::strcpyT;
 
   static const int max_string_field_length = 2*1024; //storage's status string is ~1k
 
-  const char * NitrokeyManager::get_slot_name(uint8_t slot_number)  {
+  char * NitrokeyManager::get_slot_name(uint8_t slot_number)  {
         auto payload = get_payload<GetSlotName>();
         payload.slot_number = slot_number;
         auto resp = GetSlotName::CommandTransaction::run(device, payload);
@@ -749,7 +749,7 @@ using nitrokey::misc::strcpyT;
         LockDevice::CommandTransaction::run(device);
     }
 
-    const char *NitrokeyManager::get_password_safe_slot_name(uint8_t slot_number) {
+    char * NitrokeyManager::get_password_safe_slot_name(uint8_t slot_number) {
         if (!is_valid_password_safe_slot_number(slot_number)) throw InvalidSlotException(slot_number);
         auto p = get_payload<GetPasswordSafeSlotName>();
         p.slot_number = slot_number;
@@ -759,7 +759,7 @@ using nitrokey::misc::strcpyT;
 
     bool NitrokeyManager::is_valid_password_safe_slot_number(uint8_t slot_number) const { return slot_number < 16; }
 
-    const char *NitrokeyManager::get_password_safe_slot_login(uint8_t slot_number) {
+    char * NitrokeyManager::get_password_safe_slot_login(uint8_t slot_number) {
         if (!is_valid_password_safe_slot_number(slot_number)) throw InvalidSlotException(slot_number);
         auto p = get_payload<GetPasswordSafeSlotLogin>();
         p.slot_number = slot_number;
@@ -767,7 +767,7 @@ using nitrokey::misc::strcpyT;
         return strndup((const char *) response.data().slot_login, max_string_field_length);
     }
 
-    const char *NitrokeyManager::get_password_safe_slot_password(uint8_t slot_number) {
+    char * NitrokeyManager::get_password_safe_slot_password(uint8_t slot_number) {
         if (!is_valid_password_safe_slot_number(slot_number)) throw InvalidSlotException(slot_number);
         auto p = get_payload<GetPasswordSafeSlotPassword>();
         p.slot_number = slot_number;
@@ -1059,7 +1059,7 @@ using nitrokey::misc::strcpyT;
       stick20::ChangeUpdatePassword::CommandTransaction::run(device, p);
     }
 
-    const char * NitrokeyManager::get_status_storage_as_string(){
+    char * NitrokeyManager::get_status_storage_as_string(){
       auto p = stick20::GetDeviceStatus::CommandTransaction::run(device);
       return strndup(p.data().dissect().c_str(), max_string_field_length);
     }
@@ -1069,7 +1069,7 @@ using nitrokey::misc::strcpyT;
       return p.data();
     }
 
-    const char * NitrokeyManager::get_SD_usage_data_as_string(){
+    char * NitrokeyManager::get_SD_usage_data_as_string(){
       auto p = stick20::GetSDCardOccupancy::CommandTransaction::run(device);
       return strndup(p.data().dissect().c_str(), max_string_field_length);
     }
diff --git a/libnitrokey/NitrokeyManager.h b/libnitrokey/NitrokeyManager.h
index 1f4cec4..d4630b0 100644
--- a/libnitrokey/NitrokeyManager.h
+++ b/libnitrokey/NitrokeyManager.h
@@ -93,8 +93,8 @@ char * strndup(const char* str, size_t maxlen);
         string get_status_as_string();
         string get_serial_number();
 
-        const char * get_totp_slot_name(uint8_t slot_number);
-        const char * get_hotp_slot_name(uint8_t slot_number);
+        char * get_totp_slot_name(uint8_t slot_number);
+        char * get_hotp_slot_name(uint8_t slot_number);
 
         void change_user_PIN(const char *current_PIN, const char *new_PIN);
         void change_admin_PIN(const char *current_PIN, const char *new_PIN);
@@ -108,9 +108,9 @@ char * strndup(const char* str, size_t maxlen);
 
         void lock_device();
 
-        const char *get_password_safe_slot_name(uint8_t slot_number);
-        const char *get_password_safe_slot_password(uint8_t slot_number);
-        const char *get_password_safe_slot_login(uint8_t slot_number);
+        char * get_password_safe_slot_name(uint8_t slot_number);
+        char * get_password_safe_slot_password(uint8_t slot_number);
+        char * get_password_safe_slot_login(uint8_t slot_number);
 
         void
     write_password_safe_slot(uint8_t slot_number, const char *slot_name, const char *slot_login,
@@ -187,10 +187,10 @@ char * strndup(const char* str, size_t maxlen);
 
         void send_startup(uint64_t seconds_from_epoch);
 
-        const char * get_status_storage_as_string();
+        char * get_status_storage_as_string();
         stick20::DeviceConfigurationResponsePacket::ResponsePayload get_status_storage();
 
-        const char *get_SD_usage_data_as_string();
+        char * get_SD_usage_data_as_string();
         std::pair<uint8_t,uint8_t> get_SD_usage_data();
 
 
@@ -227,7 +227,7 @@ char * strndup(const char* str, size_t maxlen);
         uint8_t get_internal_slot_number_for_hotp(uint8_t slot_number) const;
         uint8_t get_internal_slot_number_for_totp(uint8_t slot_number) const;
         bool erase_slot(uint8_t slot_number, const char *temporary_password);
-        const char * get_slot_name(uint8_t slot_number);
+        char * get_slot_name(uint8_t slot_number);
 
         template <typename ProCommand, PasswordKind StoKind>
         void change_PIN_general(const char *current_PIN, const char *new_PIN);
diff --git a/unittest/test_strdup.cpp b/unittest/test_strdup.cpp
new file mode 100644
index 0000000..f980eb9
--- /dev/null
+++ b/unittest/test_strdup.cpp
@@ -0,0 +1,57 @@
+/*
+ * Copyright (c) 2015-2018 Nitrokey UG
+ *
+ * This file is part of libnitrokey.
+ *
+ * libnitrokey is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * any later version.
+ *
+ * libnitrokey is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with libnitrokey. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * SPDX-License-Identifier: LGPL-3.0
+ */
+
+// issue: https://github.com/Nitrokey/libnitrokey/issues/110
+// tests according to the issue's author, Robin Krahl (robinkrahl)
+// suggested run command: valgrind --tool=memcheck --leak-check=full ./test_strdup
+
+#include <cstdio>
+#include <memory.h>
+#include "NK_C_API.h"
+#include "catch.hpp"
+
+
+static const int SHORT_STRING_LENGTH = 10;
+
+TEST_CASE("Test strdup memory free error", "[BASIC]")
+{
+  NK_set_debug(false);
+  char *c = NK_status(); /* error --> string literal */
+  REQUIRE(c != nullptr);
+  REQUIRE(strnlen(c, SHORT_STRING_LENGTH) == 0);
+  puts(c);
+  free(c);
+}
+
+TEST_CASE("Test strdup memory leak", "[BASIC]")
+{
+  NK_set_debug(false);
+  bool connected = NK_login_auto() == 1;
+  if (!connected) return;
+
+  REQUIRE(connected);
+  char *c = NK_status();  /* no error --> dynamically allocated */
+  REQUIRE(c != nullptr);
+  REQUIRE(strnlen(c, SHORT_STRING_LENGTH) > 0);
+  puts(c);
+  free(c);
+}
+