Bug#21966621: USAGE OF AUTO_RW_LOCK_READ/WRITE ON ALREADY LOCKED OBJECT CAUSES DEADLOCK

Fault description:
There is an internal fault in pthread that causes a deadlock when mysql_rwlock_rdlock, mysql_rwlock_unlock functions are used in wrong way. The faulty was triggered by calling two time lock function on already locked object, following seqence:
pthread_rwlock_t *x;
...
pthread_rwlock_wrlock(x);  // R:0
pthread_rwlock_wrlock(x);  // R:EDEADLK
pthread_rwlock_unlock(x);
pthread_rwlock_unlock(x);  // Fault - pthread function doesn't check if the x object is still lock, it always decrement the lock-counter which causes an underflow

pthread_rwlock_wrlock(x); // Deadlock - lock counter != 0

The production code wasn't checking the result returned by locking function (EDEADLK, EINVAL) and it still was relasing mutex two times (RAII - destructor in object).

Incorrect behaviour:
The fault occured when pluging doesn't release the srv_session and leaves it to be released be server. When server is releasing the session its using Mutexed_map_thd_srv_session class to opearte on session list. Each method of this class is guarded by rwlock, thus function close_all_sessions_of_plugin_if_any() was calling Mutexed_map_thd_srv_session::do_for_all_matching (locking the mutex) and the callback was removing the session from the list by Mutexed_map_thd_srv_session::remove(second lock). Last call leaves the mutex in invalid state thus next attempt of accesing it will cause a deadlock.

Fix description:
The solution checks in constructor of Auto_rw_lock_read,Auto_rw_lock_write the result code mysql_rwlock_rdlock and if it wasn't locked then corresponding release function in destructor isn't called. The locking result could be also checked with assert to tell alert that the code is working wrongly.

Reviewed-by: [email protected]
RB: 10561

Author: lkotula

mysql-test/suite/test_service_sql_api/r/test_x_sessions_deinit.result

@@ -24,6 +24,19 @@ srv_session_open 1 - Success
 srv_session_close 1 - Success
 srv_session_close 1 - Failed
 deinit thread
+Follows threaded run and leaves open session (Bug#21966621)
+========================================================================
+init thread
+nb_sessions = 1
+srv_session_open 1 - Success
+deinit thread
+========================================================================
+init thread
+nb_sessions = 1
+srv_session_open 1 - Success
+srv_session_close 1 - Success
+srv_session_close 1 - Failed
+deinit thread
 Follows threaded run and leaves open session (Bug#21983102)
 ========================================================================
 init thread

plugin/test_service_sql_api/test_x_sessions_deinit.cc

@@ -222,6 +222,16 @@ static int test_session_service_plugin_deinit(void *p)
   WRITE_STR("Follows threaded run\n");
   test_in_spawned_thread(p, test_session);
 
+  WRITE_STR("Follows threaded run and leaves open session (Bug#21966621)\n");
+  // Bug#21966621 - Leave session open at srv thread exit which is doing the release in following way:
+  //                1) Lock LOCK_collection while doing release callback for every session
+  //                2) Second attempt of LOCK_collection fails while removing session by the callback.
+  //                While exiting both function LOCK_collection is left in invalid state because
+  //                it was released 2 times, lock 1 time
+  //                3) at next attempt of LOCK_collection usage causes deadlock
+  test_in_spawned_thread(p, test_session_open); // step 1, 2
+  test_in_spawned_thread(p, test_session);      // step 3
+
   WRITE_STR("Follows threaded run and leaves open session (Bug#21983102)\n");
   // Bug#21983102 - iterates through sessions list in which elements are removed
   //                thus the iterator becomes invalid causing random crashes and

sql/srv_session.cc

@@ -58,10 +58,10 @@ static bool srv_session_THRs_initialized= false;
 class Auto_rw_lock_read
 {
 public:
-  explicit Auto_rw_lock_read(mysql_rwlock_t *lock) : rw_lock(lock)
+  explicit Auto_rw_lock_read(mysql_rwlock_t *lock) : rw_lock(NULL)
   {
-    if (rw_lock)
-      mysql_rwlock_rdlock(rw_lock);
+    if (lock && 0 == mysql_rwlock_rdlock(lock))
+      rw_lock = lock;
   }
 
   ~Auto_rw_lock_read()
@@ -80,10 +80,10 @@ class Auto_rw_lock_read
 class Auto_rw_lock_write
 {
 public:
-  explicit Auto_rw_lock_write(mysql_rwlock_t *lock) : rw_lock(lock)
+  explicit Auto_rw_lock_write(mysql_rwlock_t *lock) : rw_lock(NULL)
   {
-    if (rw_lock)
-      mysql_rwlock_wrlock(rw_lock);
+    if (lock && 0 == mysql_rwlock_wrlock(lock))
+      rw_lock = lock;
   }
 
   ~Auto_rw_lock_write()