CWE-479 in CRT.c

[BenBE]

While compiling I noticed the following warning for CWE-479 from GCC 14:

depbase=`echo CRT.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
gcc-14 -DHAVE_CONFIG_H -I.  -DNDEBUG  -std=c99 -pedantic -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600  -I/usr/include/libnl3 -Wall -Wcast-align -Wcast-qual -Wextra -Wfloat-equal -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wmissing-prototypes -Wpointer-arith -Wshadow -Wstrict-prototypes -Wundef -Wunused -Wwrite-strings -Wnull-dereference -D_XOPEN_SOURCE_EXTENDED -DSYSCONFDIR="\"/usr/local/etc\"" -I"./linux" -fanalyzer -MT CRT.o -MD -MP -MF $depbase.Tpo -c -o CRT.o CRT.c &&\
mv -f $depbase.Tpo $depbase.Po
CRT.c: In function ‘CRT_handleSIGTERM’:
CRT.c:846:4: warning: call to ‘snprintf’ from within signal handler [CWE-479] [-Wanalyzer-unsafe-call-within-signal-handler]
  846 |    snprintf(err_buf, sizeof(err_buf),
      |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  847 |            "A signal %d (%s) was received, exiting without persisting settings to htoprc.\n",
      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  848 |            sgn, signal_str);
      |            ~~~~~~~~~~~~~~~~
  ‘CRT_installSignalHandlers’: events 1-2
    |
    |  952 | static void CRT_installSignalHandlers(void) {
    |      |             ^~~~~~~~~~~~~~~~~~~~~~~~~
    |      |             |
    |      |             (1) entry to ‘CRT_installSignalHandlers’
    |......
    |  966 |    signal(SIGINT, CRT_handleSIGTERM);
    |      |    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |    |
    |      |    (2) registering ‘CRT_handleSIGTERM’ as signal handler
    |
  event 3
    |
    |cc1:
    | (3): later on, when the signal is delivered to the process
    |
    +--> ‘CRT_handleSIGTERM’: events 4-7
           |
           |  835 | static void CRT_handleSIGTERM(int sgn) {
           |      |             ^~~~~~~~~~~~~~~~~
           |      |             |
           |      |             (4) entry to ‘CRT_handleSIGTERM’
           |......
           |  838 |    if (!CRT_settings->changed)
           |      |       ~      
           |      |       |
           |      |       (5) following ‘false’ branch...
           |......
           |  841 |    const char* signal_str = strsignal(sgn);
           |      |                             ~~~~~~~~~~~~~~
           |      |                             |
           |      |                             (6) ...to here
           |......
           |  846 |    snprintf(err_buf, sizeof(err_buf),
           |      |    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |    |
           |      |    (7) call to ‘snprintf’ from within signal handler
           |  847 |            "A signal %d (%s) was received, exiting without persisting settings to htoprc.\n",
           |      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |  848 |            sgn, signal_str);
           |      |            ~~~~~~~~~~~~~~~~
           |

[cgzones]

Out of curiosity I asked an AI:

Based on the search results, snprintf is not considered async-signal-safe in C. Here’s a breakdown of the reasons:

• Valgrind bug: The snprintf function may not work correctly when called from a signal handler due to valgrind’s internal implementation. This is a specific issue with valgrind, not a fundamental problem with snprintf.
• ABI requirements: The x86-64 ABI requires floating-point arguments to be passed in XMM registers, which must be 16-byte aligned. Signal handlers may not provide this alignment, making snprintf unreliable.
• Non-async-signal-safe functions: snprintf is not listed as an async-signal-safe function in the POSIX standard or other reliable sources. This implies that it may not be safe to use in signal handlers.
• Initialization of locale machinery: snprintf may need to initialize locale machinery, which is not async-signal-safe.

To work around these issues, consider the following alternatives:

• Use a limited, async-signal-safe version of snprintf: The safe_snprintf function provided by idning/safe_snprintf is a simple, limited version of snprintf that is designed to be async-signal-safe. It supports a limited set of formats and does not initialize locale machinery.
• Set a flag and handle it in the main program: Instead of calling snprintf directly from a signal handler, set a flag inside the signal handler and have the main program check and handle the flag accordingly.
• Use write instead of printf: On POSIX-conforming systems, you can use write(STDOUT_FILENO, ...) instead of printf to output data from a signal handler. This approach may require additional formatting effort.

In summary, while snprintf is not inherently async-signal-safe, there are workarounds and alternatives available to ensure safe and reliable signal handling in C programs.

[Explorer09]

https://stackoverflow.com/questions/67839938/why-is-snprintf-not-considered-async-safe

[Explorer09]

Since the CRT_handleSIGTERM function simply exits after printing the diagnostic message, I wonder if the problem can be fixed by simply blocking other signals before calling snprintf(3)?

I mean, will the warning go away if we fix that way?

[BenBE]

This wont do …

[Explorer09]

Related to this bug. The signal_safe_fprintf() function in Settings.c is not quite "signal safe" as the function says, and should be fixed as well.