Repository navigation
Fix UB in signal handler - #38
Conversation
📝 WalkthroughWalkthroughThis PR centralizes shutdown around a lock-free process-wide ChangesGlobal Atomic Thread Termination & Thread Cleanup
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Microsoft Presidio Analyzer (2.2.362)memtouch.cppMicrosoft Presidio Analyzer failed to scan this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
f2f6191 to
a58b1ca
Compare
|
I've added Pascal and Philipp as reviewer's, but @tpressure might want to comment on whether the decision to use a single shared atomic termination flag is compatible with the longer term goals of this project. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
memtouch.cpp (2)
25-26: ⚡ Quick winAdd a
static_assertto guarantee lock-free signal-handler safety.The C++ standard (via
[support.signal]) only permits plain lock-free atomic operations inside an async signal handler. Functions in<stdatomic.h>(i.e.,<atomic>) are signal-safe only when "the atomic arguments are lock-free."std::atomic_flagis the only type guaranteed to be lock-free;std::atomic<bool>carries no such guarantee. IfTERMINATEhappens to use a lock internally on some target,sigint_handler'sstorecould deadlock with a thread that was interrupted while holding that lock.
std::atomic<bool>::is_always_lock_freeis aconstexprthat lets you encode the requirement as a hard compile-time failure:🛡️ Proposed fix
static std::atomic<bool> TERMINATE; +static_assert(std::atomic<bool>::is_always_lock_free, + "TERMINATE must be lock-free for async-signal-handler safety");🤖 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 25 - 26, Add a compile-time check to ensure the TERMINATE atomic is lock-free for signal-handler safety: add a static_assert referencing std::atomic<bool>::is_always_lock_free near the declaration of TERMINATE (or replace TERMINATE with a lock-free type such as std::atomic_flag and assert its lock-freedom) and update the sigint_handler usage accordingly so the assertion guarantees the atomic store in sigint_handler is allowed in async signal context.
77-80: 💤 Low valueInconsistent memory order — line 78 silently defaults to
seq_cst.Every other access to
TERMINATEexplicitly usesstd::memory_order_relaxed(lines 71, 214, and 277), but the load guarding the outer run-loop omits the order parameter, silently usingmemory_order_seq_cst. On weakly-ordered architectures this emits a superfluous fence, and the inconsistency can mislead a reader into thinking stronger ordering is semantically required here.♻️ Proposed fix
- while (not TERMINATE.load()) { + while (not TERMINATE.load(std::memory_order_relaxed)) {🤖 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 77 - 80, The load guarding the outer loop uses the default seq_cst ordering while other TERMINATE accesses use std::memory_order_relaxed, introducing an unnecessary fence and inconsistency; change the condition in the run-once branch to use TERMINATE.load(std::memory_order_relaxed) so it matches the other accesses and avoid the superfluous memory barrier (update the check around run_loop(num_pages) that references TERMINATE and ensure semantics remain the same).
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@memtouch.cpp`:
- Around line 25-26: Add a compile-time check to ensure the TERMINATE atomic is
lock-free for signal-handler safety: add a static_assert referencing
std::atomic<bool>::is_always_lock_free near the declaration of TERMINATE (or
replace TERMINATE with a lock-free type such as std::atomic_flag and assert its
lock-freedom) and update the sigint_handler usage accordingly so the assertion
guarantees the atomic store in sigint_handler is allowed in async signal
context.
- Around line 77-80: The load guarding the outer loop uses the default seq_cst
ordering while other TERMINATE accesses use std::memory_order_relaxed,
introducing an unnecessary fence and inconsistency; change the condition in the
run-once branch to use TERMINATE.load(std::memory_order_relaxed) so it matches
the other accesses and avoid the superfluous memory barrier (update the check
around run_loop(num_pages) that references TERMINATE and ensure semantics remain
the same).
faa5661 to
4e111ca
Compare
scholzp
left a comment
scholzp
left a comment
There was a problem hiding this comment.
LGTM!
I think as long as we don't want to terminate threads on an individual basis, this solution is a good step forward. Well done! :)
| static constexpr int PATTERN(0xff); | ||
| static constexpr int DEFAULT_STAT_IVAL(1000); | ||
|
|
||
| static std::atomic<bool> TERMINATE; |
There was a problem hiding this comment.
please don't use upper case for variables, even if they are global. Just call it terminate or global_terminate if you want to make it more obvious.
There was a problem hiding this comment.
Also, you can auto initialize this to false with:
static std::atomic global_terminate {false};`
This saves you from initializing it in main()
There was a problem hiding this comment.
btw @olivereanderson: Since you are new to C++ (yes?). There are at least four ways of initializing variables in C++. Although ugly IMHO, the {} variant shown by thomas is the recommended and safe way. I can also recommend the book Effective Modern C++
| while (not terminate) { | ||
| run_loop(num_pages); | ||
| if (not run_once) { | ||
| while (not TERMINATE.load(std::memory_order_relaxed)) { |
There was a problem hiding this comment.
Is there any special reason why you picked memory_order_relaxed here? On x86, it does not really make a difference, but I would have went with memory_order_seq_cst, we don't need to micro optimize here
There was a problem hiding this comment.
? On x86, it does not really make a difference,
I think this is the wrong way of thinking, both in C++ and also Rust btw. You specify the memory ordering logically for your program and the abstract c++/rust machine implemented by the compiler - it is then the compilers job to map this to imperfect implementations of this abstract machine: specifically x86, arm or riscv systems.
Although practically irrelevant in many cases and likely also here, I expect that there are ways where the compiler can optimize things with a relaxed ordering even if it eventually emits x86 - for the reason mentioned above.'
I don't have any data but what I've learned from the past: Always think about the abstract machine the compiler implements first.
In other words: to my limited understanding: The C++ memory order is not "the instruction ordering for the CPU." It is a source-level synchronization contract.
There was a problem hiding this comment.
Is there any special reason why you picked memory_order_relaxed here?
Because I do not think that seq_cst is a good default.
While it is true that if a weaker ordering works then that is also the case for seq_cst and the converse statement is not true. I maintain that it is better to demonstrate that you understand why the ordering you picked worked. If you are able to do that then you might as well pick the weakest one that works.
On the other hand choosing seq_cst does not mean that the program will work as intended.
There was a problem hiding this comment.
Let me quote the common misconceptions section in Chapter 3 of Rust Atomics and Locks by Mara Bos here (bold emphasis mine):
Myth: Sequentially consistent memory ordering is a great default and is always correct.
Putting aside performance concerns, sequentially consistent memory ordering is often seen as the perfect type of memory ordering to default to, because of its strong guarantees. It’s true that if any other memory ordering is correct, SeqCst is also correct. This might make it sound like SeqCst is always correct. However, it’s entirely possible that a concurrent algorithm is simply incorrect, regardless of memory ordering.
More importantly, when reading code, SeqCst basically tells the reader: "this operation depends on the total order of every single SeqCst operation in the program," which is an incredibly far-reaching claim. The same code would likely be easier to review and verify if it used weaker memory ordering instead, if possible. For example, Release effectively tells the reader: "this relates to an acquire operation on the same variable," which involves far fewer considerations when forming an understanding of the code.
It is advisable to see SeqCst as a warning sign. Seeing it in the wild often means that either something complicated is going on, or simply that the author did not take the time to analyze their memory ordering related assumptions, both of which are reasons for extra scrutiny.
There was a problem hiding this comment.
I want to emphasize that I am 100% sure that you do not think that SeqCst always leads to correct behavior.
What I want to get across here is that I don't think SeqCst makes reasoning about what is going on any easier. In fact I would rather say the opposite!
There was a problem hiding this comment.
No need to lecture me about memory consistency models ;) My question was why you explicitly went for relaxed semantics.
There was a problem hiding this comment.
Then the short answer is that I do not like the complex semantics of seq_cst so I pick another ordering whenever I can. In this case it is easy enough to see that relaxed ordering does the job.
phip1611
left a comment
phip1611
left a comment
There was a problem hiding this comment.
LGTM. Please do not forget to initialize TERMINATE
4e111ca to
ffbd7d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@memtouch.cpp`:
- Around line 218-223: The cleanup_memory() function currently only checks
mem_base == nullptr before calling munmap, but mmap failure leaves mem_base ==
MAP_FAILED; update cleanup_memory() to guard munmap by verifying mem_base is
neither nullptr nor MAP_FAILED (use the MAP_FAILED symbol) and only call
munmap(mem_base, uint64_t(mem_size_mib) * 1024 * 1024) in that case; after a
successful munmap set mem_base = nullptr (and optionally mem_size_mib = 0) to
make cleanup idempotent; apply the same guard/update pattern to the other
similar cleanup block around the code referenced at lines ~228-231.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
5d82b94 to
df76fd5
Compare
|
We don't have sticky approvals for some reason. @tpressure could you please resolve the discussions you started? Or let me know if you don't want to merge this as it is now. |
df76fd5 to
d577ea8
Compare
Acually, I think in this case it was a good thing because too much people have merge permissions here. I will do another round of review later today. |
We do not want cleanup_memory to call munmap on a null pointer or if mmap failed. Also set mem_base to be the null pointer before returning if the munmap call succeeded. Signed-off-by: Oliver Anderson <oliver.anderson@cyberus-technology.de> On-behalf-of: SAP oliver.anderson@sap.com
We move cleanup_memory into WorkerThread's destructor in order to enable early return in the `run` method in our next commit. Signed-off-by: Oliver Anderson <oliver.anderson@cyberus-technology.de> On-behalf-of: SAP oliver.anderson@sap.com
The way the signal handler is currently implemented we end up writing to bool's read by other threads without the use of atomics or synchronization. This is undefined behavior: It is subject to data races and/or some compilers (either existing or to appear in the future) may end up optimizing the reads out entirely. We fix this by using a single static `std::atomic<bool>` that the worker and statistics threads read from and the signal handler writes to. The less invasive solution would be to replace each individual thread's `terminate` bool with an `std::atomic<bool>`, but I could not get the constructors to accept that. Moreover the compiler messages were rather arcane and hard to understand for someone who just started writing C++. There is probably something fancy going on with copy constructors and/or constexpr or what have you. Overall I personally prefer this simpler solution, while admitting that it is less flexible if individual thread termination should be desired in the future. I am going with the YAGNI principle here. Signed-off-by: Oliver Anderson <oliver.anderson@cyberus-technology.de> On-behalf-of: SAP <oliver.anderson@sap.com>
d577ea8 to
e21e653
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@memtouch.cpp`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| 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); | ||
| } |
There was a problem hiding this comment.
🧩 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.cppRepository: 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.
Random comment from the side's What's the reason to dismiss this issue if the goal is fixing UB in signal handlers? |
Thanks for the question. The answer is simple: I missed the issue (or thought that it was an older thing that had already been resolved). Related: Is merely the existence of non-async-signal-safe code in a signal handler UB, or does it depend on its usage? I am aware of us using an |
Just executing it is enough unless stated otherwise in the documentation.
The manpage of sigaction says:
|
The way the signal handler is currently implemented we end up writing to bool's read by other threads without the use of atomics or synchronization. This is undefined behavior: It is subject to data races and/or some compilers (either existing or to appear in the future) may end up optimizing the reads out entirely.
We fix this by using a single static
std::atomic<bool>that the worker and statistics threads read from and the signal handler writes to.The less invasive solution would be to replace each individual thread's
terminatebool with anstd::atomic<bool>, but I could not get the constructors to accept that. Moreover the compiler messages were rather arcane and hard to understand for someone who just started writing C++. There is probably something fancy going on with copy constructors and/or constexpr or what have you.Overall I personally prefer this simpler solution, while admitting that it is less flexible if individual thread termination should be desired in the future. I am going with the YAGNI principle here.
This was first brought up in this previous discussion.
Summary by CodeRabbit