From 825ac5ba3cdf82bd0cc7382051a6c9a60d41fa76 Mon Sep 17 00:00:00 2001 From: Uri Shachar Date: Thu, 16 Jul 2020 16:35:41 +0300 Subject: [PATCH] Prevent race condition around strerror which is not thread safe The compile time checks might need some additional tuning - verified on Linux & OSX but not beyond that. --- src/fmacros.h | 3 +++ src/server.c | 47 +++++++++++++++++++++++++++++++-------- src/server.h | 3 +++ tests/integration/aof.tcl | 29 ++++++++++++++++++++++++ tests/support/server.tcl | 17 +++++++++----- 5 files changed, 84 insertions(+), 15 deletions(-) diff --git a/src/fmacros.h b/src/fmacros.h index 6e56c759dd5..82616765fd6 100644 --- a/src/fmacros.h +++ b/src/fmacros.h @@ -47,6 +47,9 @@ * On NetBSD, _XOPEN_SOURCE undefines _NETBSD_SOURCE and * thus hides inet_aton etc. */ +#elif defined(__APPLE__) +#define _DARWIN_C_SOURCE 1 +#define _XOPEN_SOURCE 600L #elif !defined(__NetBSD__) #define _XOPEN_SOURCE #endif diff --git a/src/server.c b/src/server.c index 4fdbf30ec0c..d4081a93850 100644 --- a/src/server.c +++ b/src/server.c @@ -1058,6 +1058,38 @@ void serverLogRaw(int level, const char *msg) { if (server.syslog_enabled) syslog(syslogLevelMap[level], "%s", msg); } +void serverLogSysError(int level, int error_code, const char *fmt, ...) { + va_list ap; + unsigned int offset = 0; + char msg[LOG_MAX_LEN]; + + if ((level&0xff) < server.verbosity) return; + + va_start(ap, fmt); + offset = vsnprintf(msg, sizeof(msg), fmt, ap); + va_end(ap); + + if (offset < (sizeof(msg) + sizeof(" ( )"))) { + char buf[LOG_MAX_LEN - offset]; + char *error_msg; +#if (_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && (!defined(_GNU_SOURCE) || defined(__ANDROID__)) + /* XSI-compliant version of strerror_r() */ + if (strerror_r(error_code, buf, sizeof(buf))) { + snprintf(buf, sizeof(buf), "Unknown error code %d", error_code); + } + error_msg = buf; +#else + /* GNU-specific version of strerror_r() */ + error_msg = strerror_r(error_code, buf, sizeof(buf)); +#endif + serverLog(level, "%s (%s)", msg, error_msg); + } + else { + serverLogRaw(level,msg); + serverLogRaw(level,"Could not format system error message due to insufficient space"); + } +} + /* Like serverLogRaw() but with printf-alike support. This is the function that * is used across the code. The raw version is only used in order to dump * the INFO output on crash. */ @@ -1765,9 +1797,8 @@ void checkChildrenDone(void) { } if (pid == -1) { - serverLog(LL_WARNING,"wait3() returned an error: %s. " + serverLogSysError(LL_WARNING, errno, "wait3() returned an error: " "rdb_child_pid = %d, aof_child_pid = %d, module_child_pid = %d", - strerror(errno), (int) server.rdb_child_pid, (int) server.aof_child_pid, (int) server.module_child_pid); @@ -2458,8 +2489,7 @@ void adjustOpenFilesLimit(void) { struct rlimit limit; if (getrlimit(RLIMIT_NOFILE,&limit) == -1) { - serverLog(LL_WARNING,"Unable to obtain the current NOFILE limit (%s), assuming 1024 and setting the max clients configuration accordingly.", - strerror(errno)); + serverLogSysError(LL_WARNING, errno, "Unable to obtain the current NOFILE limit, assuming 1024 and setting the max clients configuration accordingly."); server.maxclients = 1024-CONFIG_MIN_RESERVED_FDS; } else { rlim_t oldlimit = limit.rlim_cur; @@ -2510,9 +2540,9 @@ void adjustOpenFilesLimit(void) { "requiring at least %llu max file descriptors.", old_maxclients, (unsigned long long) maxfiles); - serverLog(LL_WARNING,"Server can't set maximum open files " - "to %llu because of OS error: %s.", - (unsigned long long) maxfiles, strerror(setrlimit_error)); + serverLogSysError(LL_WARNING, setrlimit_error, "Server can't set maximum open files " + "to %llu because of OS error", + (unsigned long long) maxfiles); serverLog(LL_WARNING,"Current maximum open files is %llu. " "maxclients has been reduced to %d to compensate for " "low ulimit. " @@ -2842,8 +2872,7 @@ void initServer(void) { server.aof_fd = open(server.aof_filename, O_WRONLY|O_APPEND|O_CREAT,0644); if (server.aof_fd == -1) { - serverLog(LL_WARNING, "Can't open the append-only file: %s", - strerror(errno)); + serverLogSysError(LL_WARNING, errno, "Can't open the append-only file (%s)", server.aof_filename); exit(1); } } diff --git a/src/server.h b/src/server.h index c4db4278e96..f901ca9158b 100644 --- a/src/server.h +++ b/src/server.h @@ -1949,8 +1949,11 @@ int prepareForShutdown(); #ifdef __GNUC__ void serverLog(int level, const char *fmt, ...) __attribute__((format(printf, 2, 3))); +void serverLogSysError(int level, int error_code, const char *fmt, ...) + __attribute__((format(printf, 3, 4))); #else void serverLog(int level, const char *fmt, ...); +void serverLogSysError(int level, int error_code, const char *fmt, ...); #endif void serverLogRaw(int level, const char *msg); void serverLogFromHandler(int level, const char *msg); diff --git a/tests/integration/aof.tcl b/tests/integration/aof.tcl index 2734de7f161..d60f0a5acfa 100644 --- a/tests/integration/aof.tcl +++ b/tests/integration/aof.tcl @@ -14,6 +14,13 @@ proc create_aof {code} { close $fp } +proc start_invalid_server_aof {overrides code} { + upvar defaults defaults srv srv server_path server_path + set config [concat $defaults $overrides] + set srv [start_server [concat "expectfail {1}" [list overrides $config]]] + uplevel 1 $code +} + proc start_server_aof {overrides code} { upvar defaults defaults srv srv server_path server_path set config [concat $defaults $overrides] @@ -23,6 +30,7 @@ proc start_server_aof {overrides code} { } tags {"aof"} { + ## Server can start when aof-load-truncated is set to yes and AOF ## is truncated, with an incomplete MULTI block. create_aof { @@ -31,6 +39,27 @@ tags {"aof"} { append_to_aof [formatCommand set bar world] } + ## Server can't start when aof file is inaccessible + exec chmod 0000 $aof_path + start_invalid_server_aof [list dir $server_path ] { + test "Bad format: Server should have logged an access error" { + set pattern "*Can't open the append-only file (appendonly.aof) (Permission denied)*" + set retry 10 + while {$retry} { + set result [exec tail -1 < [dict get $srv stdout]] + if {[string match $pattern $result]} { + break + } + incr retry -1 + after 1000 + } + if {$retry == 0} { + error "assertion:expected error not found on config file" + } + } + } + exec chmod 666 $aof_path + start_server_aof [list dir $server_path aof-load-truncated yes] { test "Unfinished MULTI: Server should start if load-truncated is yes" { assert_equal 1 [is_alive $srv] diff --git a/tests/support/server.tcl b/tests/support/server.tcl index 400017c5fe4..b4967fa8925 100644 --- a/tests/support/server.tcl +++ b/tests/support/server.tcl @@ -179,6 +179,7 @@ proc start_server {options {code undefined}} { set baseconfig "default.conf" set overrides {} set tags {} + set expectfail 0 # parse options foreach {option value} $options { @@ -190,6 +191,8 @@ proc start_server {options {code undefined}} { "tags" { set tags $value set ::tags [concat $::tags $value] } + "expectfail" { + set expectfail $value } default { error "Unknown option $option" } } @@ -274,11 +277,13 @@ proc start_server {options {code undefined}} { regexp {PID:\s(\d+)} [exec cat $stdout] _ _pid after $checkperiod incr maxiter -1 - if {$maxiter == 0} { - start_server_error $config_file "No PID detected in log $stdout" - puts "--- LOG CONTENT ---" - puts [exec cat $stdout] - puts "-------------------" + if {$maxiter == 0 } { + if {!$expectfail} { + start_server_error $config_file "No PID detected in log $stdout" + puts "--- LOG CONTENT ---" + puts [exec cat $stdout] + puts "-------------------" + } break } @@ -315,7 +320,7 @@ proc start_server {options {code undefined}} { puts "" } - if {!$serverisup} { + if {!$serverisup && !$expectfail} { set err {} append err [exec cat $stdout] "\n" [exec cat $stderr] start_server_error $config_file $err