Bug#26447825: MYSQLD COMPLETELY SILENT EVEN IN FAILURE IF LOG-ERROR IS

NOT WRITABLE

Problem: C-library function freopen() is not atomic. It closes the
stream given as argument before trying (and thereby checking if it is
possible) to open the file which the stream is to be associated with. So
in the event that the open() fails, the user is left without a working
stream where the error message for the failed open can go. Likewise, any
buffered messages not yet written to the stream are also lost.

Solution: Stop using freopen() and re-implement my_freopen() using
fopen(), fileno() and dup2(). That way any error from open() (or
fileno()) is caught before doing anything which affects the stream.
According to the man-page, dup2() is atomic.

Author: Dyre Tjeldvoll

mysys/my_fopen.c

@@ -1,4 +1,4 @@
-/* Copyright (c) 2000, 2015, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2000, 2017, Oracle and/or its affiliates. All rights reserved.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -143,7 +143,68 @@ static FILE *my_win_freopen(const char *path, const char *mode, FILE *stream)
 
   return stream;
 }
+#else
+
+/**
+  Replacement for freopen() which will not close the stream until the
+  new file has been successfully opened. This gives the ability log
+  the reason for reopen's failure to the stream, and also to flush any
+  buffered messages already written to the stream, before terminating
+  the process.
+
+  After a new stream to path has been opened, its file descriptor is
+  obtained, and dup2() is used to associate the original stream with
+  the new file. The newly opened stream and associated file descriptor
+  is then closed with fclose().
+
+  @param path file which the stream should be opened to
+  @param mode FILE mode to use when opening new file
+  @param stream stream to reopen
+
+  @retval FILE stream being passed in if successful,
+  @retval nullptr otherwise
+ */
+static FILE *my_safe_freopen(const char *path, const char *mode, FILE *stream)
+{
+  int streamfd= -1;
+  FILE *pathstream= NULL;
+  int pathfd= -1;
+  int ds= -1;
+
+  DBUG_ASSERT(path != NULL && stream != NULL);
+  streamfd= fileno(stream);
+  if (streamfd == -1)
+  {
+    /* We have not done anything to the stream, but if we cannot
+       get its fd, it is probably in a bad state anyway... */
+    return NULL;
+  }
+  pathstream= fopen(path, mode);
+  if (pathstream == NULL)
+  {
+    /* Failed to open file for some reason. stream should still be
+       usable. */
+    return NULL;
+  }
 
+  pathfd= fileno(pathstream);
+  if (pathfd == -1)
+  {
+    fclose(pathstream);
+    return NULL;
+  }
+  do
+  {
+    ds= fflush(stream);
+    if (ds == 0)
+    {
+      ds= dup2(pathfd, streamfd);
+    }
+  }
+  while (ds == -1 && errno == EINTR);
+  fclose(pathstream);
+  return (ds == -1 ? NULL : stream);
+}
 #endif
 
 
@@ -168,7 +229,7 @@ FILE *my_freopen(const char *path, const char *mode, FILE *stream)
 #if defined(_WIN32)
   result= my_win_freopen(path, mode, stream);
 #else
-  result= freopen(path, mode, stream);
+  result= my_safe_freopen(path, mode, stream);
 #endif
 
   return result;

sql/log.cc

@@ -1,4 +1,4 @@
-/* Copyright (c) 2000, 2016, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2000, 2017, Oracle and/or its affiliates. All rights reserved.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -1997,7 +1997,13 @@ bool open_error_log(const char *filename)
   while (retries-- && errors);
 
   if (errors)
+  {
+    char errbuf[MYSYS_STRERROR_SIZE];
+    sql_print_error("Could not open file '%s' for error logging: %s",
+                    filename,  my_strerror(errbuf, sizeof(errbuf), errno));
+    flush_error_log_messages();
     return true;
+  }
 
   /* The error stream must be unbuffered. */
   setbuf(stderr, NULL);

unittest/gunit/CMakeLists.txt

@@ -290,6 +290,7 @@ SET(TESTS
   mysys_lf
   mysys_my_atomic
   mysys_my_b_vprintf
+  mysys_my_freopen
   mysys_my_loadpath
   mysys_my_malloc
   mysys_my_pwrite

unittest/gunit/mysys_my_freopen-t.cc

@@ -0,0 +1,122 @@
+/* Copyright (c) 2013, 2017, Oracle and/or its affiliates. All rights reserved.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; version 2 of the License.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA */
+
+#include "my_config.h"
+
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+#include <stddef.h>
+#ifdef HAVE_UNISTD_H
+#include <unistd.h>
+#endif
+
+#include "my_sys.h"
+
+#ifndef _WIN32
+
+namespace mysys_my_freopen_unittest {
+FILE *null_file= NULL;
+
+class MysysMyFreopenTest : public ::testing::Test
+{
+public:
+  FILE *stream;
+  char name[32];
+  MysysMyFreopenTest() :
+    stream(NULL)
+  {
+    strncpy(name, "MyFreopen_XXXXXX", 32);
+  }
+  virtual void SetUp()
+  {
+    int fd= mkstemp(name);
+    stream= fdopen(fd, "a");
+  }
+  virtual void TearDown()
+  {
+    if (stream != NULL)
+    {
+      fclose(stream);
+      unlink(name);
+    }
+  }
+};
+
+
+// Test case demonstates that freopen is not atomic
+TEST_F(MysysMyFreopenTest, FreopenFailure)
+{
+  EXPECT_EQ(null_file, freopen("/", "a", stream));
+  const char *txt= "This text should end up in old stream file";
+  EXPECT_EQ(EOF, fputs(txt, stream));
+  EXPECT_NE(0, ferror(stream));
+  EXPECT_EQ(0, fflush(stream));
+
+  char buf[64]= "nada";
+  FILE *instream= fopen(name, "r");
+  EXPECT_NE(null_file, instream);
+  EXPECT_EQ(static_cast<char*>(NULL), fgets(buf, 64, instream));
+  EXPECT_EQ(0, ferror(instream));
+  EXPECT_NE(0, feof(instream));
+  EXPECT_STREQ("nada", buf);
+  EXPECT_EQ(0, fclose(instream));
+}
+
+// Positive test case for the new version of my_freopen
+TEST_F(MysysMyFreopenTest, MyFreopenOK)
+{
+  char fname[32] = "MyFreopenOK_XXXXXX";
+  int fd= mkstemp(fname);
+
+  EXPECT_EQ(stream, my_freopen(fname, "a", stream));
+  const char *txt= "This text should end up in fname";
+  int txtlen= strlen(txt);
+  EXPECT_EQ(32, txtlen);
+  EXPECT_LT(0, fputs(txt, stream));
+  EXPECT_EQ(0, fflush(stream));
+
+  char buf[64];
+  FILE *instream= fdopen(fd, "r");
+  EXPECT_NE(null_file, instream);
+  EXPECT_EQ(buf, fgets(buf, 64, instream));
+  EXPECT_STREQ(txt, buf);
+  EXPECT_EQ(0, fclose(instream));
+  EXPECT_EQ(0, unlink(fname));
+}
+
+
+// Negative test case for my_reopen. Shows that even if my_reopen
+// fails, it is still possible to write to the stream.
+TEST_F(MysysMyFreopenTest, MyFreopenFailure)
+{
+  EXPECT_EQ(null_file, my_freopen("/", "a", stream));
+  const char *txt= "This text should end up in old stream file";
+  EXPECT_LT(0, fputs(txt, stream));
+  EXPECT_EQ(0, fflush(stream));
+
+  char buf[64];
+  FILE *instream= fopen(name, "r");
+  EXPECT_NE(null_file, instream);
+  EXPECT_EQ(buf, fgets(buf, 64, instream));
+  EXPECT_STREQ(txt, buf);
+  EXPECT_EQ(0, fclose(instream));
+}
+}
+#endif