Skip to content
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.

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

CWE-479 in CRT.c #1563

Open
BenBE opened this issue Nov 15, 2024 · 4 comments · May be fixed by #1570
Open

CWE-479 in CRT.c #1563

BenBE opened this issue Nov 15, 2024 · 4 comments · May be fixed by #1570
Labels
code quality ♻️ Code quality enhancement good first issue 🥇 Good for newcomers

Comments

@BenBE
Copy link
Member

BenBE commented Nov 15, 2024

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);
           |      |            ~~~~~~~~~~~~~~~~
           |
@BenBE BenBE added good first issue 🥇 Good for newcomers code quality ♻️ Code quality enhancement labels Nov 15, 2024
@cgzones
Copy link
Member

cgzones commented Nov 15, 2024

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
Copy link
Contributor

@Explorer09
Copy link
Contributor

Explorer09 commented Nov 25, 2024

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
Copy link
Member Author

BenBE commented Nov 25, 2024

This wont do …

@Explorer09 Explorer09 linked a pull request Dec 16, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality ♻️ Code quality enhancement good first issue 🥇 Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants