Skip to content

Fix UB in signal handler #38

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tpressure merged 3 commits into from
May 7, 2026
Merged

Fix UB in signal handler #38

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d51c0e8
memtouch: Improve safety of cleanup_memory
olivereanderson May 7, 2026
2c74fa0
memtouch: Move cleanup_memory into WorkerThread's destructor
olivereanderson May 7, 2026
e21e653
memtouch: Fix UB in the signal handler
olivereanderson May 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 63 additions & 18 deletions memtouch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include <argparse.hpp>

#include <algorithm>
#include <atomic>
#include <chrono>
#include <cstring>
#include <fstream>
Expand All @@ -21,16 +22,26 @@ static constexpr uint64_t PAGE_SIZE(4096);
static constexpr int PATTERN(0xff);
static constexpr int DEFAULT_STAT_IVAL(1000);

static std::atomic<bool> global_terminate{false};

using namespace std;

struct Statistics {
Statistics() = default;
Statistics() = default;
Statistics(const Statistics&) = delete;
Statistics& operator=(const Statistics&) = delete;

Statistics(Statistics&& o) noexcept {
write_rate.store(o.write_rate.load());
read_rate.store(o.read_rate.load());
}

Statistics& operator=(Statistics&& o) noexcept {
write_rate.store(o.write_rate.load());
read_rate.store(o.read_rate.load());
return *this;
}

std::atomic<float> read_rate{0};
std::atomic<float> write_rate{0};
};
Expand All @@ -49,6 +60,43 @@ class WorkerThread {
page_log_ival(page_log_ival_),
stats() {}

~WorkerThread() { cleanup_memory(); }

WorkerThread(const WorkerThread&) = delete;
WorkerThread& operator=(const WorkerThread&) = delete;

WorkerThread(WorkerThread&& o) noexcept
: id(o.id),
run_once(o.run_once),
mem_size_mib(o.mem_size_mib),
rw_ratio(o.rw_ratio),
page_log_ival(o.page_log_ival),
mem_base(o.mem_base),
read_buffer(),
stats(std::move(o.stats)) {
o.mem_base = nullptr;
}

WorkerThread& operator=(WorkerThread&& o) noexcept {
if (this == &o) {
return *this;
}

cleanup_memory();

id = o.id;
run_once = o.run_once;
mem_size_mib = o.mem_size_mib;
rw_ratio = o.rw_ratio;
page_log_ival = o.page_log_ival;
mem_base = o.mem_base;
stats = std::move(o.stats);

o.mem_base = nullptr;

return *this;
}

bool pre_run() {
if (not allocate_memory()) {
printf("Worker %d: Unable to allocate memory\n", id);
Expand All @@ -65,21 +113,19 @@ class WorkerThread {

// Warmup, write every page
for (uint64_t page{0}; page < num_pages; ++page) {
if (terminate) {
if (global_terminate.load(std::memory_order_relaxed)) {
break;
}
write_page(page);
}

if (run_once) {
kill();
return;
}

while (not terminate) {
while (not global_terminate.load(std::memory_order_relaxed)) {
run_loop(num_pages);
}

cleanup_memory();
}

int64_t measure_time_ns(std::function<void()> func) {
Expand Down Expand Up @@ -169,9 +215,15 @@ class WorkerThread {
}

void cleanup_memory() {
if (mem_base == nullptr || mem_base == MAP_FAILED) {
return;
}

if (munmap(mem_base, uint64_t(mem_size_mib) * 1024 * 1024) != 0) {
printf("Unable to unmap memory\n");
return;
}
mem_base = nullptr;
}

bool allocate_memory() {
Expand All @@ -181,8 +233,6 @@ class WorkerThread {
return mem_base != MAP_FAILED;
}

void kill() { terminate = true; }

float write_rate() const { return stats.write_rate.load(); }

float read_rate() const { return stats.read_rate.load(); }
Expand All @@ -194,8 +244,6 @@ class WorkerThread {
unsigned rw_ratio;
uint64_t page_log_ival;

bool terminate{false};
Comment thread
tpressure marked this conversation as resolved.
Resolved

void* mem_base{nullptr};

char read_buffer[PAGE_SIZE];
Expand All @@ -214,7 +262,7 @@ class StatisticsThread {
}

void run() {
while (not terminate) {
while (not global_terminate.load(std::memory_order_relaxed)) {
float read_rate{0};
float write_rate{0};

Expand All @@ -234,8 +282,6 @@ class StatisticsThread {
}
}

void kill() { terminate = true; }

void set_interval(unsigned ival_ms) { logging_ival_ms = ival_ms; }

string get_iso8601_time() {
Expand Down Expand Up @@ -267,7 +313,6 @@ class StatisticsThread {
private:
vector<WorkerThread>& workers;

bool terminate{false};
unsigned logging_ival_ms{DEFAULT_STAT_IVAL};

ofstream log_file{};
Expand All @@ -280,11 +325,11 @@ StatisticsThread stat_thread(worker_storage);

void sigint_handler([[maybe_unused]] int s) {
printf("Terminating...\n");
for (auto& worker : worker_storage) {
worker.kill();
}

stat_thread.kill();
static_assert(std::atomic<bool>::is_always_lock_free,
"Unable to ensure async-signal-handler safety: std::atomic<bool> should always "
"be lock free, but the assertion failed.");
global_terminate.store(true, std::memory_order_relaxed);
}
Comment on lines 326 to 333

@coderabbitai coderabbitai Bot May 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify signal handlers do not call non-async-signal-safe I/O.
rg -n -C3 'void\s+sigint_handler|printf\s*\(' memtouch.cpp

Repository: cobaltcore-dev/memtouch

Length of output: 1746

Remove printf from the signal handler; it is not async-signal-safe.

Line 327 calls printf inside sigint_handler. The printf function uses internal stdio locks and is not in the POSIX list of async-signal-safe functions. Calling it in a signal handler can deadlock if the interrupted code held the stdio lock. Remove the printf call and rely on the lock-free atomic store to signal termination.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@memtouch.cpp` around lines 326 - 333, Remove the non-async-signal-safe printf
call from the sigint_handler and only perform the lock-free atomic update:
delete the printf("Terminating...\n"); line and keep the static_assert and the
global_terminate.store(true, std::memory_order_relaxed); so the handler only
uses async-signal-safe operations (atomic store) to signal termination.


void setup_signals() {
Expand Down