Avoid calling snprintf in SIGTERM handler

[Explorer09]

By Implementing our own routine for formatting the number.

This could resolve #1563 but I'm sure people would say this solution is ugly.

[fasterit]

Please just use write(). The 25 lines of formatting magic are a bit over-the-top imo.

[Explorer09]

Please just use write(). The 25 lines of formatting magic are a bit over-the-top imo.

I wish an atomic write() for the "caught signal X" line. Formatting that string in a buffer before writing to stderr is considered a feature for me, because that line of output would not be broken by any signal.

[BenBE]

I agree with @fasterit here. Please shorten that formatting logic. What can (and probably should) be done is reordering things to work like:

1. Short static buffer for the signal ID num2string conversion
2. strcat the three parts into a full buffer
3. write the final resulting string.

Please don't inline the num2string conversion, but use a function which is provided with the target buffer+size. Use "???" if the conversion doesn't fit that buffer.

[Explorer09]

I am thinking there could be another way to solve the CWE-479 problem, as that warning wasn't present in htop's fatal error reporting code, and yet the fatal error code also reports what signal it has received.

[fasterit]

We could vendor a stripped down version of https://github.com/idning/safe_snprintf . The license there should be GPLv2 as the upstream MySQL code is.

[fasterit]

Another implementation: https://chromium.googlesource.com/chromium/src/base/+/master/strings/safe_sprintf.h (BSD3 license, C++ implementation)

[BenBE]

I don't think we should start putting C++ code in htop. The C implementation you linked also has some quirks which aren't quite right (aside from missing the format attributes, it's not fully validating the format subset that it supports).

[Explorer09]

@fasterit What I said is that maybe we don't need another implementation of snprintf(3) at all.

As mentioned, GCC 14 doesn't warn about CWE-479 for CRT_handleSIGSEGV (maybe this is a bug - a missed warning), but warns for CRT_handleSIGTERM. If the signal safety of snprintf does matter for both signal handler routines, then the CWE-479 should apply to both and not just one of them. And we should fix CRT_handleSIGSEGV as well since snprintf is also used extensibly there.

The problem of CWE-479 happens when another signal is delivered during the handling of CRT_handleSIGSEGV or CRT_handleSIGTERM, causing either function or snprintf within to "reenter" in an asynchronous way. Since both handlers in htop are only there to report messages, the simplest and the most proper way (I think) is to block signals when any of the messages is being constructed.

[BenBE]

I think that GCC14 only warns about one of those is a bug due to a missed warning. So when we are fixing this it will need to be both. And as mentioned, I can fully live with a minimalistic itoa+5xstrncat solution, if the custom-rolled itoa is not inlined.

Basic requirements:

1. Gets the job done
2. Signal-safe
3. Single atomic write to stderr

If we can re-use this custom-rolled itoa in further places, where it also improves performance I'd be the last to reject such an implementation. But that's not a priority.

[Explorer09]

@BenBE I was thinking deeper that, when another signal arrives during a handler, what kind of message htop's output should look like.

To prevent the situation where we implemented our own itoa+strncat combinations only to find out we've done it wrong and the message is still corrupt like this:

FATAL PROGRAM ERROR DETECTED
FATAL PROGRAM ERROR DETECTED
============================
Please check at https://htop.dev/issues whether this issue has already been reported.
If no similar issue has been reported before, please create a new issue with the following information:
FATAL PROGRAM ERROR DETECTED
============================
Please check at https://htop.dev/issues whether this issue has already been reported.
If no similar issue has been reported before, please create a new issue with the following information:

[BenBE]

I can live with that. And for nested signals we could just go ahead and ignore the second signal.

// Imagine this being all atomically sound …
atomic_int8_t signal_nested = 0;
void sighandler() {
   signal_nested++;

   if(signal_nested > 1) return;

   // Actual handler code
}

[Explorer09]

@BenBE Oops. I was confused with the CWE-479 issue here because there was actually two sub-issues:

• The use of snprintf in signal handlers was unsafe - It's nothing to do with what messages would be printed on error, but Glibc documentation says that snprintf may internally call malloc ("AS-Unsafe heap" and "AS-Unsafe heap" implies "AS-Unsafe lock"), so it could deadlock internally when malloc is "reentered" due to signals.
• The error message could be garbled when htop signal handers are "reentered".

I was too focused on the latter and yet forget the former issue at hand.

[fasterit]

I don't think we should start putting C++ code in htop. The C implementation you linked also has some quirks which aren't quite right (aside from missing the format attributes, it's not fully validating the format subset that it supports).

I was not implying to use C++ in htop, I still think looking at how other people solved the problem is inspirational.

So why not write a minimal good safe_snprintf for our use case?

[BenBE]

I don't think we should start putting C++ code in htop. The C implementation you linked also has some quirks which aren't quite right (aside from missing the format attributes, it's not fully validating the format subset that it supports).

I was not implying to use C++ in htop, I still think looking at how other people solved the problem is inspirational.

Didn't interpret it as such. Just remarked that I think it would be overkill to start doing C++, just to be able to handle printing some error message right before we emergency exit our process … ;-)

So why not write a minimal good safe_snprintf for our use case?

All for it. No objections. Just the requirement for correctness (i.e. implemented features should be correct, unsupported features should be properly rejected in the implementation). Needed features:

• string output
• integer output (signed/unsigned) for different sizes (int32, int64)
• hex display
• no [bf]loat support
• optional width/padding support
• signal-safe

[Explorer09]

So why not write a minimal good safe_snprintf for our use case?

I believe there should be an existing safe_snprintf solution for us already, that can be embedded in htop code, or we could go for a "itoa + various strncat" solution.

However I wish to give up on this PR and let someone else who is motivated to work on this. It's not my interest in implementing a snprintf or itoa just for this, and I personally consider this issue in htop a low priority.