WIP: Prevent race condition around strerror which is not thread safe

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

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);
         }
     }

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);

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]

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