Skip to content

Commit

Permalink
Avoid fprintf in signal handlers
Browse files Browse the repository at this point in the history 
fprintf(3) is not safe to call ins signal handlers due to file buffering
and memory allocations.
Format messages via snprintf(3) and output them via write(2).
  • Loading branch information
@cgzones
cgzones committed Apr 8, 2024
1 parent 3477fbf commit 234aa6e
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 47 deletions.
48 changes: 30 additions & 18 deletions CRT.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -912,15 +914,15 @@ 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);
}
}

if (header)
fprintf(stderr, "\n<<<<<<<<<< stderr output <<<<<<<<<<\n");
full_write_str(STDERR_FILENO, "\n<<<<<<<<<< stderr output <<<<<<<<<<\n");

close(stderrRedirectNewFd);
stderrRedirectNewFd = -1;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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];
Expand All @@ -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"
Expand All @@ -1240,74 +1247,79 @@ 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"
);

const char* signal_str = strsignal(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. "
"This can usually be done by running the following command:\n"
"\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);
Expand All @@ -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);
Expand Down
80 changes: 51 additions & 29 deletions Settings.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -563,65 +563,86 @@ 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) {
static void writeFields(int (*outputFunc)(FILE*, const char*,...), 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);
outputFunc(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);
outputFunc(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);
outputFunc(fd, "%s%d", sep, (int) fields[i] - 1);
}
sep = " ";
}
fputc(separator, fd);
outputFunc(fd, "%c", separator);
}

static void writeList(FILE* fd, char** list, int len, char separator) {
static void writeList(int (*outputFunc)(FILE*, const char*,...), 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]);
outputFunc(fd, "%s%s", sep, list[i]);
sep = " ";
}
fputc(separator, fd);
outputFunc(fd, "%c", separator);
}

static void writeMeters(const Settings* this, FILE* fd, char separator, unsigned int column) {
static void writeMeters(const Settings* this, int (*outputFunc)(FILE*, const char*,...),
FILE* fd, char separator, unsigned int column) {
if (this->hColumns[column].len) {
writeList(fd, this->hColumns[column].names, this->hColumns[column].len, separator);
writeList(outputFunc, fd, this->hColumns[column].names, this->hColumns[column].len, separator);
} else {
fputc('!', fd);
fputc(separator, fd);
outputFunc(fd, "!%c", separator);
}
}

static void writeMeterModes(const Settings* this, FILE* fd, char separator, unsigned int column) {
static void writeMeterModes(const Settings* this, int (*outputFunc)(FILE*, const char*,...),
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]);
outputFunc(fd, "%s%d", sep, this->hColumns[column].modes[i]);
sep = " ";
}
} else {
fputc('!', fd);
outputFunc(fd, "!");
}

fputc(separator, fd);
outputFunc(fd, "%c", separator);
}

ATTR_FORMAT(printf, 2, 3)
static int signal_safe_fprintf(FILE* stream, const char* fmt, ...) {
char buf[512];

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;
int (*outputFunc)(FILE*, const char*,...);
if (onCrash) {
fd = stderr;
separator = ';';
outputFunc = signal_safe_fprintf;
} else {
/* create tempfile with mode 0600 */
mode_t cur_umask = umask(S_IXUSR | S_IRWXG | S_IRWXO);
Expand All @@ -634,20 +655,21 @@ int Settings_write(const Settings* this, bool onCrash) {
if (fd == NULL)
return -errno;
separator = '\n';
outputFunc = fprintf;
}

#define printSettingInteger(setting_, value_) \
fprintf(fd, setting_ "=%d%c", (int) (value_), separator)
outputFunc(fd, setting_ "=%d%c", (int) (value_), separator)
#define printSettingString(setting_, value_) \
fprintf(fd, setting_ "=%s%c", value_, separator)
outputFunc(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");
outputFunc(fd, "# Beware! This file is rewritten by htop when settings are changed in the interface.\n");
outputFunc(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);
outputFunc(fd, "fields="); writeFields(outputFunc, 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);
Expand Down Expand Up @@ -688,10 +710,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);
outputFunc(fd, "column_meters_%u=", i);
writeMeters(this, outputFunc, fd, separator, i);
outputFunc(fd, "column_meter_modes_%u=", i);
writeMeterModes(this, outputFunc, fd, separator, i);
}

// Legacy compatibility with older versions of htop
Expand All @@ -709,14 +731,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);
outputFunc(fd, "screen:%s=", ss->heading);
writeFields(outputFunc, 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);
outputFunc(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);
outputFunc(fd, "%s=Dynamic(%s)%c", ".tree_sort_key", treeSortKey, separator);
} else {
printSettingString(".sort_key", sortKey);
printSettingString(".tree_sort_key", treeSortKey);
Expand Down
4 changes: 4 additions & 0 deletions XUtils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down

0 comments on commit 234aa6e

Please sign in to comment.