Skip to content

Commit

Permalink
Merge branch '214-do-not-return-local-stack-address'
Browse files Browse the repository at this point in the history
Use static string object for keeping the c_str message for the caller.
Strings collection used as an alternative to memory leaks done via strdup().
To add, TargetBufferSmallerThanSource Exception should never happen in a correctly written library client, as this completely depends on the implementation and not on the communication with the device

Downside: if missed and when occurring too often, the memory taken by the exception messages can take too much memory.
Potential improvement: replace std::vector with a kind of a ring buffer, or add simple wrapping logic over it.

This should avoid breaking ABI by using static memory. Note: there might be multiple exceptions' strings collections (for each compilation unit including this header).
Correct that for libnitrokey 4, by keeping string as a member of the exception.

Fixes #214
Fixes #217
  • Loading branch information
szszszsz committed Nov 15, 2022
2 parents 0378956 + b740cb4 commit a82aa1e
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 5 deletions.
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ set_target_properties(nitrokey PROPERTIES

OPTION(ERROR_ON_WARNING "Stop compilation on warning found (not supported for MSVC)" OFF)
if (NOT MSVC)
set(COMPILE_FLAGS "-Wall -Wno-unused-function -Wcast-qual -Woverloaded-virtual -Wsign-compare -Wformat -Wformat-security")
set(COMPILE_FLAGS "-Wall -Wno-unused-function -Wcast-qual -Woverloaded-virtual -Wsign-compare -Wformat -Wformat-security -Wreturn-local-addr")
IF(NOT APPLE)
if (ERROR_ON_WARNING)
set(COMPILE_FLAGS "${COMPILE_FLAGS} -Werror")
Expand Down
14 changes: 11 additions & 3 deletions libnitrokey/LibraryException.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#ifndef LIBNITROKEY_LIBRARYEXCEPTION_H
#define LIBNITROKEY_LIBRARYEXCEPTION_H

#include <mutex>
#include <exception>
#include <cstdint>
#include <string>
Expand All @@ -32,6 +33,12 @@ class LibraryException: std::exception {
virtual uint8_t exception_id()= 0;
};

// Use static string object for keeping the c_str message for the caller.
// Strings collection used as an alternative to memory leaks done via strdup().
// TargetBufferSmallerThanSource Exception should never happen in a correctly written library client.
static std::vector<std::string> g_exception_messages;
static std::mutex g_exception_message_mutex;

class TargetBufferSmallerThanSource: public LibraryException {
public:
virtual uint8_t exception_id() override {
Expand All @@ -47,11 +54,12 @@ class TargetBufferSmallerThanSource: public LibraryException {
) : source_size(source_size_), target_size(target_size_) {}

virtual const char *what() const noexcept override {
std::lock_guard<std::mutex> lock(g_exception_message_mutex);
std::string s = " ";
auto ts = [](size_t x){ return std::to_string(x); };
std::string msg = std::string("Target buffer size is smaller than source: [source size, buffer size]")
+s+ ts(source_size) +s+ ts(target_size);
return msg.c_str();
g_exception_messages.emplace_back(std::string("Target buffer size is smaller than source: [source size, buffer size]")
+ s + ts(source_size) + s + ts(target_size));
return g_exception_messages.back().c_str();
}

};
Expand Down
2 changes: 1 addition & 1 deletion libnitrokey/stick10_commands_0.8.h
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ namespace nitrokey {
std::stringstream ss;
#ifdef LOG_VOLATILE_DATA
ss << "data:" << std::endl
<< ::nitrokey::misc::hexdump(reinterpret_cast<const uint8_t *>&data), sizeof data);
<< ::nitrokey::misc::hexdump(reinterpret_cast<const uint8_t *> (&data), sizeof data);
#else
ss << " Volatile data not logged" << std::endl;
#endif
Expand Down

0 comments on commit a82aa1e

Please sign in to comment.