Merge pull request #31393 from azat/fix-stderr-before-reopen

Check stderr is writable before reopining it (to avoid losing errors)

base/daemon/BaseDaemon.cpp

@@ -675,6 +675,34 @@ void BaseDaemon::initialize(Application & self)
     if ((!log_path.empty() && is_daemon) || config().has("logger.stderr"))
     {
         std::string stderr_path = config().getString("logger.stderr", log_path + "/stderr.log");
+
+        /// Check that stderr is writable before freopen(),
+        /// since freopen() will make stderr invalid on error,
+        /// and logging to stderr will be broken,
+        /// so the following code (that is used in every program) will not write anything:
+        ///
+        ///     int main(int argc, char ** argv)
+        ///     {
+        ///         try
+        ///         {
+        ///             DB::SomeApp app;
+        ///             return app.run(argc, argv);
+        ///         }
+        ///         catch (...)
+        ///         {
+        ///             std::cerr << DB::getCurrentExceptionMessage(true) << "\n";
+        ///             return 1;
+        ///         }
+        ///     }
+        if (access(stderr_path.c_str(), W_OK))
+        {
+            int fd;
+            if ((fd = creat(stderr_path.c_str(), 0600)) == -1 && errno != EEXIST)
+                throw Poco::OpenFileException("File " + stderr_path + " (logger.stderr) is not writable");
+            if (fd != -1)
+                ::close(fd);
+        }
+
         if (!freopen(stderr_path.c_str(), "a+", stderr))
             throw Poco::OpenFileException("Cannot attach stderr to " + stderr_path);
 

tests/queries/0_stateless/02116_clickhouse_stderr.reference

@@ -0,0 +1,3 @@
+Ensure that not existing file still write an error to stderr
+File /no/such/file (logger.stderr) is not writable
+Ensure that the file will be created

tests/queries/0_stateless/02116_clickhouse_stderr.sh

@@ -0,0 +1,31 @@
+#!/usr/bin/env bash
+
+CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
+# shellcheck source=../shell_config.sh
+. "$CUR_DIR"/../shell_config.sh
+
+(
+    echo 'Ensure that not existing file still write an error to stderr'
+
+    test_dir=$(mktemp -d -t clickhouse.XXXXXX)
+    mkdir -p "$test_dir"
+    cd "$test_dir" || exit 1
+    $CLICKHOUSE_SERVER_BINARY -- --logger.stderr=/no/such/file |& grep -o 'File /no/such/file (logger.stderr) is not writable'
+    rm -fr "${test_dir:?}"
+)
+
+(
+    echo 'Ensure that the file will be created'
+
+    test_dir=$(mktemp -d -t clickhouse.XXXXXX)
+    mkdir -p "$test_dir"
+    cd "$test_dir" || exit 1
+
+    stderr=$(mktemp -t clickhouse.XXXXXX)
+    $CLICKHOUSE_SERVER_BINARY -- --logger.stderr="$stderr" 2>/dev/null
+    # -s -- check that stderr was created and is not empty
+    test -s "$stderr" || exit 2
+    rm "$stderr"
+
+    rm -fr "${test_dir:?}"
+)