From 2fafa349a7d133b7237e0f4f94873a5941435b98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Sat, 6 Apr 2024 19:41:17 +0200 Subject: Avoid fprintf in signal handlers fprintf(3) is not safe to call ins signal handlers due to file buffering and memory allocations. Format messages in signal handlers via snprintf(3) and output them via write(2). --- CRT.c | 48 ++++++++++++++++++++++++++++++------------------ XUtils.h | 4 ++++ 2 files changed, 34 insertions(+), 18 deletions(-) diff --git a/CRT.c b/CRT.c index 3c6c8c49..149cd519 100644 --- a/CRT.c +++ b/CRT.c @@ -843,9 +843,11 @@ static void CRT_handleSIGTERM(int sgn) { if (!signal_str) signal_str = "unknown reason"; - fprintf(stderr, + char err_buf[512]; + snprintf(err_buf, sizeof(err_buf), "A signal %d (%s) was received, exiting without persisting settings to htoprc.\n", sgn, signal_str); + full_write_str(STDERR_FILENO, err_buf); _exit(0); } @@ -912,7 +914,7 @@ static void dumpStderr(void) { if (res > 0) { if (!header) { - fprintf(stderr, ">>>>>>>>>> stderr output >>>>>>>>>>\n"); + full_write_str(STDERR_FILENO, ">>>>>>>>>> stderr output >>>>>>>>>>\n"); header = true; } full_write(STDERR_FILENO, buffer, res); @@ -920,7 +922,7 @@ static void dumpStderr(void) { } if (header) - fprintf(stderr, "\n<<<<<<<<<< stderr output <<<<<<<<<<\n"); + full_write_str(STDERR_FILENO, "\n<<<<<<<<<< stderr output <<<<<<<<<<\n"); close(stderrRedirectNewFd); stderrRedirectNewFd = -1; @@ -1185,6 +1187,8 @@ static void print_backtrace(void) { unsigned int item = 0; + char err_buf[1024]; + while (unw_step(&cursor) > 0) { unw_word_t pc; unw_get_reg(&cursor, UNW_REG_IP, &pc); @@ -1213,7 +1217,8 @@ static void print_backtrace(void) { const bool is_signal_frame = unw_is_signal_frame(&cursor) > 0; const char* frame = is_signal_frame ? " {signal frame}" : ""; - fprintf(stderr, "%2u: %#14lx %s (%s+%#lx) [%p]%s\n", item++, pc, fname, symbolName, offset, ptr, frame); + snprintf(err_buf, sizeof(err_buf), "%2u: %#14lx %s (%s+%#lx) [%p]%s\n", item++, pc, fname, symbolName, offset, ptr, frame); + full_write_str(STDERR_FILENO, err_buf); } #elif defined(HAVE_EXECINFO_H) void* backtraceArray[256]; @@ -1229,7 +1234,9 @@ static void print_backtrace(void) { void CRT_handleSIGSEGV(int signal) { CRT_done(); - fprintf(stderr, "\n\n" + char err_buf[512]; + + snprintf(err_buf, sizeof(err_buf), "\n\n" "FATAL PROGRAM ERROR DETECTED\n" "============================\n" "Please check at https://htop.dev/issues whether this issue has already been reported.\n" @@ -1240,12 +1247,13 @@ void CRT_handleSIGSEGV(int signal) { " - Likely steps to reproduce (How did it happen?)\n", program ); + full_write_str(STDERR_FILENO, err_buf); #ifdef PRINT_BACKTRACE - fprintf(stderr, " - Backtrace of the issue (see below)\n"); + full_write_str(STDERR_FILENO, " - Backtrace of the issue (see below)\n"); #endif - fprintf(stderr, + full_write_str(STDERR_FILENO, "\n" ); @@ -1253,29 +1261,30 @@ void CRT_handleSIGSEGV(int signal) { if (!signal_str) { signal_str = "unknown reason"; } - fprintf(stderr, + snprintf(err_buf, sizeof(err_buf), "Error information:\n" "------------------\n" "A signal %d (%s) was received.\n" "\n", signal, signal_str ); + full_write_str(STDERR_FILENO, err_buf); - fprintf(stderr, + full_write_str(STDERR_FILENO, "Setting information:\n" "--------------------\n"); Settings_write(CRT_crashSettings, true); - fprintf(stderr, "\n\n"); + full_write_str(STDERR_FILENO, "\n\n"); #ifdef PRINT_BACKTRACE - fprintf(stderr, + full_write_str(STDERR_FILENO, "Backtrace information:\n" "----------------------\n" ); print_backtrace(); - fprintf(stderr, + snprintf(err_buf, sizeof(err_buf), "\n" "To make the above information more practical to work with, " "please also provide a disassembly of your %s binary. " @@ -1283,31 +1292,34 @@ void CRT_handleSIGSEGV(int signal) { "\n", program ); + full_write_str(STDERR_FILENO, err_buf); #ifdef HTOP_DARWIN - fprintf(stderr, " otool -tvV `which %s` > ~/%s.otool\n", program, program); + snprintf(err_buf, sizeof(err_buf), " otool -tvV `which %s` > ~/%s.otool\n", program, program); #else - fprintf(stderr, " objdump -d -S -w `which %s` > ~/%s.objdump\n", program, program); + snprintf(err_buf, sizeof(err_buf), " objdump -d -S -w `which %s` > ~/%s.objdump\n", program, program); #endif + full_write_str(STDERR_FILENO, err_buf); - fprintf(stderr, + full_write_str(STDERR_FILENO, "\n" "Please include the generated file in your report.\n" ); #endif - fprintf(stderr, + snprintf(err_buf, sizeof(err_buf), "Running this program with debug symbols or inside a debugger may provide further insights.\n" "\n" "Thank you for helping to improve %s!\n" "\n", program ); + full_write_str(STDERR_FILENO, err_buf); /* Call old sigsegv handler; may be default exit or third party one (e.g. ASAN) */ if (sigaction(signal, &old_sig_handler[signal], NULL) < 0) { /* This avoids an infinite loop in case the handler could not be reset. */ - fprintf(stderr, + full_write_str(STDERR_FILENO, "!!! Chained handler could not be restored. Forcing exit.\n" ); _exit(1); @@ -1317,7 +1329,7 @@ void CRT_handleSIGSEGV(int signal) { raise(signal); // Always terminate, even if installed handler returns - fprintf(stderr, + full_write_str(STDERR_FILENO, "!!! Chained handler did not exit. Forcing exit.\n" ); _exit(1); diff --git a/XUtils.h b/XUtils.h index 32bc282d..3703f8bc 100644 --- a/XUtils.h +++ b/XUtils.h @@ -117,6 +117,10 @@ ssize_t xReadfileat(openat_arg_t dirfd, const char* pathname, void* buffer, size ATTR_ACCESS3_R(2, 3) ssize_t full_write(int fd, const void* buf, size_t count); +static inline ssize_t full_write_str(int fd, const char* str) { + return full_write(fd, str, strlen(str)); +} + /* Compares floating point values for ordering data entries. In this function, NaN is considered "less than" any other floating point value (regardless of sign), and two NaNs are considered "equal" regardless of payload. */ -- cgit v1.2.3 From 81ff634427676192f226ce66cb6c6e64253b7df1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Sat, 6 Apr 2024 19:41:17 +0200 Subject: Avoid fprintf in Settings_write Settings_write() is called on a crash inside of a signal handler. Using fprintf(3) is not safe to call ins signal handlers due to file buffering and memory allocations. Format messages in signal handlers via snprintf(3) and output them via write(2) on crash. --- Settings.c | 82 ++++++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 53 insertions(+), 29 deletions(-) diff --git a/Settings.c b/Settings.c index e2c94e2b..7bbf6a45 100644 --- a/Settings.c +++ b/Settings.c @@ -563,65 +563,88 @@ static bool Settings_read(Settings* this, const char* fileName, unsigned int ini return didReadAny; } -static void writeFields(FILE* fd, const ProcessField* fields, Hashtable* columns, bool byName, char separator) { +typedef ATTR_FORMAT(printf, 2, 3) int (*OutputFunc)(FILE*, const char*,...); + +static void writeFields(OutputFunc of, FILE* fd, + const ProcessField* fields, Hashtable* columns, + bool byName, char separator) { const char* sep = ""; for (unsigned int i = 0; fields[i]; i++) { if (fields[i] < LAST_PROCESSFIELD && byName) { const char* pName = toFieldName(columns, fields[i], NULL); - fprintf(fd, "%s%s", sep, pName); + of(fd, "%s%s", sep, pName); } else if (fields[i] >= LAST_PROCESSFIELD && byName) { bool enabled; const char* pName = toFieldName(columns, fields[i], &enabled); if (enabled) - fprintf(fd, "%sDynamic(%s)", sep, pName); + of(fd, "%sDynamic(%s)", sep, pName); } else { // This "-1" is for compatibility with the older enum format. - fprintf(fd, "%s%d", sep, (int) fields[i] - 1); + of(fd, "%s%d", sep, (int) fields[i] - 1); } sep = " "; } - fputc(separator, fd); + of(fd, "%c", separator); } -static void writeList(FILE* fd, char** list, int len, char separator) { +static void writeList(OutputFunc of, FILE* fd, + char** list, int len, char separator) { const char* sep = ""; for (int i = 0; i < len; i++) { - fprintf(fd, "%s%s", sep, list[i]); + of(fd, "%s%s", sep, list[i]); sep = " "; } - fputc(separator, fd); + of(fd, "%c", separator); } -static void writeMeters(const Settings* this, FILE* fd, char separator, unsigned int column) { +static void writeMeters(const Settings* this, OutputFunc of, + FILE* fd, char separator, unsigned int column) { if (this->hColumns[column].len) { - writeList(fd, this->hColumns[column].names, this->hColumns[column].len, separator); + writeList(of, fd, this->hColumns[column].names, this->hColumns[column].len, separator); } else { - fputc('!', fd); - fputc(separator, fd); + of(fd, "!%c", separator); } } -static void writeMeterModes(const Settings* this, FILE* fd, char separator, unsigned int column) { +static void writeMeterModes(const Settings* this, OutputFunc of, + FILE* fd, char separator, unsigned int column) { if (this->hColumns[column].len) { const char* sep = ""; for (size_t i = 0; i < this->hColumns[column].len; i++) { - fprintf(fd, "%s%d", sep, this->hColumns[column].modes[i]); + of(fd, "%s%d", sep, this->hColumns[column].modes[i]); sep = " "; } } else { - fputc('!', fd); + of(fd, "!"); } - fputc(separator, fd); + of(fd, "%c", separator); +} + +ATTR_FORMAT(printf, 2, 3) +static int signal_safe_fprintf(FILE* stream, const char* fmt, ...) { + char buf[2048]; + + va_list vl; + va_start(vl, fmt); + int n = vsnprintf(buf, sizeof(buf), fmt, vl); + va_end(vl); + + if (n <= 0) + return n; + + return full_write_str(fileno(stream), buf); } int Settings_write(const Settings* this, bool onCrash) { FILE* fd; char separator; char* tmpFilename = NULL; + OutputFunc of; if (onCrash) { fd = stderr; separator = ';'; + of = signal_safe_fprintf; } else { /* create tempfile with mode 0600 */ mode_t cur_umask = umask(S_IXUSR | S_IRWXG | S_IRWXO); @@ -634,20 +657,21 @@ int Settings_write(const Settings* this, bool onCrash) { if (fd == NULL) return -errno; separator = '\n'; + of = fprintf; } #define printSettingInteger(setting_, value_) \ - fprintf(fd, setting_ "=%d%c", (int) (value_), separator) + of(fd, setting_ "=%d%c", (int) (value_), separator) #define printSettingString(setting_, value_) \ - fprintf(fd, setting_ "=%s%c", value_, separator) + of(fd, setting_ "=%s%c", value_, separator) if (!onCrash) { - fprintf(fd, "# Beware! This file is rewritten by htop when settings are changed in the interface.\n"); - fprintf(fd, "# The parser is also very primitive, and not human-friendly.\n"); + of(fd, "# Beware! This file is rewritten by htop when settings are changed in the interface.\n"); + of(fd, "# The parser is also very primitive, and not human-friendly.\n"); } printSettingString("htop_version", VERSION); printSettingInteger("config_reader_min_version", CONFIG_READER_MIN_VERSION); - fprintf(fd, "fields="); writeFields(fd, this->screens[0]->fields, this->dynamicColumns, false, separator); + of(fd, "fields="); writeFields(of, fd, this->screens[0]->fields, this->dynamicColumns, false, separator); printSettingInteger("hide_kernel_threads", this->hideKernelThreads); printSettingInteger("hide_userland_threads", this->hideUserlandThreads); printSettingInteger("hide_running_in_container", this->hideRunningInContainer); @@ -688,10 +712,10 @@ int Settings_write(const Settings* this, bool onCrash) { printSettingString("header_layout", HeaderLayout_getName(this->hLayout)); for (unsigned int i = 0; i < HeaderLayout_getColumns(this->hLayout); i++) { - fprintf(fd, "column_meters_%u=", i); - writeMeters(this, fd, separator, i); - fprintf(fd, "column_meter_modes_%u=", i); - writeMeterModes(this, fd, separator, i); + of(fd, "column_meters_%u=", i); + writeMeters(this, of, fd, separator, i); + of(fd, "column_meter_modes_%u=", i); + writeMeterModes(this, of, fd, separator, i); } // Legacy compatibility with older versions of htop @@ -709,14 +733,14 @@ int Settings_write(const Settings* this, bool onCrash) { const char* sortKey = toFieldName(this->dynamicColumns, ss->sortKey, NULL); const char* treeSortKey = toFieldName(this->dynamicColumns, ss->treeSortKey, NULL); - fprintf(fd, "screen:%s=", ss->heading); - writeFields(fd, ss->fields, this->dynamicColumns, true, separator); + of(fd, "screen:%s=", ss->heading); + writeFields(of, fd, ss->fields, this->dynamicColumns, true, separator); if (ss->dynamic) { printSettingString(".dynamic", ss->dynamic); if (ss->sortKey && ss->sortKey != PID) - fprintf(fd, "%s=Dynamic(%s)%c", ".sort_key", sortKey, separator); + of(fd, "%s=Dynamic(%s)%c", ".sort_key", sortKey, separator); if (ss->treeSortKey && ss->treeSortKey != PID) - fprintf(fd, "%s=Dynamic(%s)%c", ".tree_sort_key", treeSortKey, separator); + of(fd, "%s=Dynamic(%s)%c", ".tree_sort_key", treeSortKey, separator); } else { printSettingString(".sort_key", sortKey); printSettingString(".tree_sort_key", treeSortKey); -- cgit v1.2.3