BUG#24296005 XPLUGIN UNIX SOCK NOT CLEANED AFTER SERVER CLEAN SHUTDOWN/XPLUGIN NOT INSTALLED

lkotula · lkotula · commit 6bdad5c8357e · 2016-08-04T21:27:26.000+02:00
Description:
  Unix socket file and lock file were not deleted by X Plugin.

Solution:
  In case of successful allocation of UNIX socket the plugin when
  exiting is going to call `unlink` on both socket and lock file.

RB: 13542
Reviewed-by: Grzegorz Szwarc &lt;grzegorz.szwarc@oracle.com&gt;
diff --git a/rapid/plugin/x/ngs/include/ngs_common/connection_vio.h b/rapid/plugin/x/ngs/include/ngs_common/connection_vio.h
@@ -107,6 +107,8 @@ class Connection_vio
   void close();
 
   static void close_socket(my_socket &fd);
+  static void unlink_unix_socket_file(const std::string &unix_socket_file);
+
   static my_socket accept(my_socket sock, struct sockaddr* addr, socklen_t& len, int& err, std::string& strerr);
 
   static my_socket create_and_bind_socket(const unsigned short port, std::string &error_message);
@@ -120,6 +122,7 @@ class Connection_vio
 
 private:
   static bool create_lockfile(const std::string &unix_socket_file, std::string &error_message);
+  static std::string get_lockfile_name(const std::string &unix_socket_file);
 
   friend class Ssl_context;
 
diff --git a/rapid/plugin/x/ngs/ngs_common/connection_vio.cc b/rapid/plugin/x/ngs/ngs_common/connection_vio.cc
@@ -353,6 +353,20 @@ void Connection_vio::close_socket(my_socket &fd)
   }
 }
 
+void Connection_vio::unlink_unix_socket_file(const std::string &unix_socket_file)
+{
+  if (unix_socket_file.empty())
+    return;
+
+  if (!m_system_operations)
+    return;
+
+  const std::string unix_socket_lockfile = get_lockfile_name(unix_socket_file);
+
+  (void) m_system_operations->unlink(unix_socket_file.c_str());
+  (void) m_system_operations->unlink(unix_socket_lockfile.c_str());
+}
+
 void Connection_vio::get_error(int& err, std::string& strerr)
 {
   err = socket_errno;
@@ -549,7 +563,7 @@ bool Connection_vio::create_lockfile(const std::string &unix_socket_file, std::s
   char buffer[8];
   const char x_prefix = 'X';
   const pid_t cur_pid= m_system_operations->getpid();
-  std::string lock_filename= unix_socket_file + ".lock";
+  std::string lock_filename= get_lockfile_name(unix_socket_file);
 
   compile_time_assert(sizeof(pid_t) == 4);
   int retries= 3;
@@ -685,6 +699,11 @@ bool Connection_vio::create_lockfile(const std::string &unix_socket_file, std::s
 #endif // defined(HAVE_SYS_UN_H)
 }
 
+std::string Connection_vio::get_lockfile_name(const std::string &unix_socket_file)
+{
+  return unix_socket_file+ ".lock";
+}
+
 Socket_operations_interface::Unique_ptr Connection_vio::m_socket_operations;
 System_operations_interface::Unique_ptr Connection_vio::m_system_operations;
 
diff --git a/rapid/plugin/x/src/xpl_listener_factory.cc b/rapid/plugin/x/src/xpl_listener_factory.cc
@@ -41,7 +41,8 @@ class Tcp_listener: public ngs::Listener_interface
 
   ~Tcp_listener()
   {
-    ::ngs::Connection_vio::close_socket(m_tcp_socket);
+    // close_listener() can be called multiple times, by user + from destructor
+    close_listener();
   }
 
   Sync_variable_state &get_state()
@@ -94,6 +95,11 @@ class Tcp_listener: public ngs::Listener_interface
 
   void close_listener()
   {
+    // Connection_vio::close_socket can be called multiple times
+    // it invalidates the content of m_tcp_socket thus at next call
+    // it does nothing
+    //
+    // Same applies to close_listener()
     ngs::Connection_vio::close_socket(m_tcp_socket);
   }
 
@@ -127,7 +133,8 @@ class Unix_socket_listener: public ngs::Listener_interface
 
   ~Unix_socket_listener()
   {
-    ngs::Connection_vio::close_socket(m_unix_socket);
+    // close_listener() can be called multiple times, by user + from destructor
+    close_listener();
   }
 
   Sync_variable_state &get_state()
@@ -179,7 +186,11 @@ class Unix_socket_listener: public ngs::Listener_interface
 
   void close_listener()
   {
+    const bool should_unlink_unix_socket = INVALID_SOCKET != m_unix_socket;
     ngs::Connection_vio::close_socket(m_unix_socket);
+
+    if (should_unlink_unix_socket)
+      ngs::Connection_vio::unlink_unix_socket_file(m_unix_socket_path);
   }
 
   void loop()
diff --git a/rapid/plugin/x/tests/mtr/r/connection_unixsocket_invalid.result b/rapid/plugin/x/tests/mtr/r/connection_unixsocket_invalid.result
@@ -24,7 +24,7 @@ ok
 Check if mysqlx_socket is empty
 result
 1
-ERROR: Connection refused, while connecting to SOCKET
+ERROR: No such file or directory connecting to SOCKET
 call mtr.add_suppression("Plugin mysqlx reported: .X Plugin failed to setup UNIX socket ./dev/null/mysqlx\.sock., with:");
 call mtr.add_suppression("Plugin mysqlx reported: .could not create UNIX socket lock file /dev/null/mysqlx\.sock\.lock");
 # restart: --loose-mysqlx-socket=/dev/null/mysqlx.sock
@@ -35,7 +35,7 @@ ok
 Check if mysqlx_socket is empty
 result
 1
-ERROR: Connection refused, while connecting to SOCKET
+ERROR: No such file or directory connecting to SOCKET
 ERROR: Not a directory, while connecting to SOCKET
 UNINSTALL PLUGIN mysqlx;
 # restart: --loose-mysqlx-socket=SOCKET
diff --git a/rapid/plugin/x/tests/mtr/t/connection_unixsocket.test b/rapid/plugin/x/tests/mtr/t/connection_unixsocket.test
@@ -82,6 +82,11 @@ Mysqlx.Crud.Find {
 
 EOF
 
+# Server was started with UNIX socket support
+# Check if UNIX socket file and lock file exist
+--file_exists $MASTER_X_MYSOCK
+--file_exists $MASTER_X_MYSOCK.lock
+
 --error 1
 --exec $MYSQLXTEST --socket=$MASTER_X_MYSOCK -uuser_127_0_0_1 --password='' --file=$MYSQL_TMP_DIR/basic_connection.tmp 2>&1
 --error 1
@@ -166,6 +171,15 @@ EOF
 let $restart_parameters = restart: --mysqlx-socket=$MYSQL_TMP_DIR/valid_mysqlx.sock;
 --source include/restart_mysqld.inc
 
+# MySQL Server was started with new UNIX socket file for X Plugin
+# Check if old UNIX socket file was removed and lockfile
+--error 1
+--file_exists $MASTER_X_MYSOCK
+--error 1
+--file_exists $MASTER_X_MYSOCK.lock
+
+# Check if the X Plugin is accessable through 
+# new file
 --exec $MYSQLXTEST --socket=$MYSQL_TMP_DIR/valid_mysqlx.sock -uroot --password='' --file=$MYSQL_TMP_DIR/check_unixsocket.tmp 2>&1
 
 ## Postamble
diff --git a/rapid/plugin/x/tests/mtr/t/connection_unixsocket_invalid.test b/rapid/plugin/x/tests/mtr/t/connection_unixsocket_invalid.test
@@ -42,7 +42,7 @@ let $restart_parameters = restart: --loose-mysqlx-socket=;
 --eval select (IF(VARIABLE_VALUE is NULL, "",VARIABLE_VALUE) = "") as 'result' FROM performance_schema.global_status WHERE VARIABLE_NAME like 'Mysqlx_socket';
 --enable_query_log
 
---replace_regex /ERROR: Connection refused, while connecting to .+\.sock/ERROR: Connection refused, while connecting to SOCKET/
+--replace_regex /ERROR: No such file or directory, while connecting to .+\.sock/ERROR: No such file or directory connecting to SOCKET/
 --error 1
 --exec $MYSQLXTEST --socket=$MASTER_X_MYSOCK -uroot --password='' --file=$MYSQL_TMP_DIR/check_unixsocket.tmp 2>&1
 
@@ -59,7 +59,7 @@ let $restart_parameters = restart: --loose-mysqlx-socket="/dev/null/mysqlx.sock"
 --eval select (IF(VARIABLE_VALUE is NULL, "",VARIABLE_VALUE) = "") as 'result' FROM performance_schema.global_status WHERE VARIABLE_NAME like 'Mysqlx_socket';
 --enable_query_log
 
---replace_regex /ERROR: Connection refused, while connecting to .+\.sock/ERROR: Connection refused, while connecting to SOCKET/
+--replace_regex /ERROR: No such file or directory, while connecting to .+\.sock/ERROR: No such file or directory connecting to SOCKET/
 --error 1
 --exec $MYSQLXTEST --socket=$MASTER_X_MYSOCK -uroot --password='' --file=$MYSQL_TMP_DIR/check_unixsocket.tmp 2>&1
 
@@ -192,7 +192,6 @@ let $restart_parameters = restart: --loose-mysqlx-socket=$MYSQL_TMP_DIR/long_dir
 --exec $MYSQLXTEST --socket=$MYSQL_TMP_DIR/long_dir012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/mysqlx.socket -uroot --password='' --file=$MYSQL_TMP_DIR/check_unixsocket.tmp 2>&1
 
 ## Postamble
---remove_file $MYSQL_TMP_DIR/create_socket_on_this_file.tmp
 --remove_file $MYSQL_TMP_DIR/check_unixsocket.tmp
 --remove_file $MYSQL_TMP_DIR/check_connection.tmp
 --rmdir $MYSQL_TMP_DIR/directory
diff --git a/rapid/unittest/gunit/xplugin/connection_vio_t.cc b/rapid/unittest/gunit/xplugin/connection_vio_t.cc
@@ -42,10 +42,10 @@ namespace ngs
       {
         m_connection_vio.reset(new Connection_vio(m_ssl_context, NULL));
 
-        m_mock_socket_operations = new Mock_socket_operations();
+        m_mock_socket_operations = new StrictMock<Mock_socket_operations>();
         m_connection_vio->set_socket_operations(m_mock_socket_operations);
 
-        m_mock_system_operations = new Mock_system_operations();
+        m_mock_system_operations = new StrictMock<Mock_system_operations>();
         m_connection_vio->set_system_operations(m_mock_system_operations);
       }
 
@@ -466,5 +466,34 @@ namespace ngs
       ASSERT_EQ(INVALID_SOCKET, result);
 #endif // !defined(HAVE_SYS_UN_H)
     }
+
+    TEST_F(Connection_vio_test, try_to_unlink_empty_string)
+    {
+      const std::string expected_unix_socket_file = "";
+
+      // Does nothing
+      Connection_vio::unlink_unix_socket_file(expected_unix_socket_file);
+    }
+
+    TEST_F(Connection_vio_test, try_to_unlink_when_system_interfaces_are_not_set)
+    {
+      const std::string expected_unix_socket_file = "existing file";
+
+      m_connection_vio->set_system_operations(NULL);
+
+      // Does nothing
+      Connection_vio::unlink_unix_socket_file(expected_unix_socket_file);
+    }
+
+    TEST_F(Connection_vio_test, try_to_unlink_existing_unix_socket_file)
+    {
+      const std::string expected_unix_socket_file = "expected file";
+      const std::string expected_lockfile = "expected file.lock";
+
+      EXPECT_CALL(*m_mock_system_operations, unlink(StrEq(expected_unix_socket_file)));
+      EXPECT_CALL(*m_mock_system_operations, unlink(StrEq(expected_lockfile)));
+
+      Connection_vio::unlink_unix_socket_file(expected_unix_socket_file);
+    }
   }
 }

Original file line number	Diff line number	Diff line change
`@@ -41,7 +41,8 @@ class Tcp_listener: public ngs::Listener_interface`
`41`	`41`
`42`	`42`	`~Tcp_listener()`
`43`	`43`	`{`
`44`		`- ::ngs::Connection_vio::close_socket(m_tcp_socket);`
	`44`	`+ // close_listener() can be called multiple times, by user + from destructor`
	`45`	`+ close_listener();`
`45`	`46`	`}`
`46`	`47`
`47`	`48`	`Sync_variable_state &get_state()`
`@@ -94,6 +95,11 @@ class Tcp_listener: public ngs::Listener_interface`
`94`	`95`
`95`	`96`	`void close_listener()`
`96`	`97`	`{`
	`98`	`+ // Connection_vio::close_socket can be called multiple times`
	`99`	`+ // it invalidates the content of m_tcp_socket thus at next call`
	`100`	`+ // it does nothing`
	`101`	`+ //`
	`102`	`+ // Same applies to close_listener()`
`97`	`103`	`ngs::Connection_vio::close_socket(m_tcp_socket);`
`98`	`104`	`}`
`99`	`105`
`@@ -127,7 +133,8 @@ class Unix_socket_listener: public ngs::Listener_interface`
`127`	`133`
`128`	`134`	`~Unix_socket_listener()`
`129`	`135`	`{`
`130`		`- ngs::Connection_vio::close_socket(m_unix_socket);`
	`136`	`+ // close_listener() can be called multiple times, by user + from destructor`
	`137`	`+ close_listener();`
`131`	`138`	`}`
`132`	`139`
`133`	`140`	`Sync_variable_state &get_state()`
`@@ -179,7 +186,11 @@ class Unix_socket_listener: public ngs::Listener_interface`
`179`	`186`
`180`	`187`	`void close_listener()`
`181`	`188`	`{`
	`189`	`+ const bool should_unlink_unix_socket = INVALID_SOCKET != m_unix_socket;`
`182`	`190`	`ngs::Connection_vio::close_socket(m_unix_socket);`
	`191`	`+`
	`192`	`+ if (should_unlink_unix_socket)`
	`193`	`+ ngs::Connection_vio::unlink_unix_socket_file(m_unix_socket_path);`
`183`	`194`	`}`
`184`	`195`
`185`	`196`	`void loop()`