Bug#21485997: WRONG INSTRUMENTATION OF CLASS GTID_SET

Sujatha Sivakumar · Sujatha Sivakumar · commit 1fbfab58385e · 2015-09-03T10:42:59.000+05:30
Analysis:
========
At runtime, some GTID_SET objects are instrumented with a
performance schema mutex key of 0, which is incorrect (not
instrumented).

The root cause is the declaration of constructors, like:

Gtid_set(Sid_map *sid_map, Checkable_rwlock *sid_lock= NULL
#ifdef HAVE_PSI_INTERFACE
        ,PSI_mutex_key free_intervals_mutex_key= 0
#endif
        );

The issue is giving a default value of 0 when the mutex key
is not passed. This allows some caller to not pass any key,
causing a bug.


Fix:
===
Moved the PSI_mutex_key from Gtid_state class to Gtid_set
class.
diff --git a/client/mysqlbinlog.cc b/client/mysqlbinlog.cc
@@ -30,6 +30,7 @@
 
 #define MYSQL_CLIENT
 #undef MYSQL_SERVER
+#define DISABLE_PSI_MUTEX
 #include "client_priv.h"
 #include "my_default.h"
 #include <my_time.h>
diff --git a/mysql-test/suite/binlog/r/binlog_group_commit_gtid_order.result b/mysql-test/suite/binlog/r/binlog_group_commit_gtid_order.result
@@ -4,7 +4,7 @@ CREATE TABLE t2 (c1 INT) Engine=InnoDB;
 SET @free_intervals_mutex_before= (
 SELECT COUNT_STAR
 FROM performance_schema.events_waits_summary_global_by_event_name WHERE
-EVENT_NAME='wait/synch/mutex/sql/Gtid_state::gtid_executed::free_intervals_mutex');
+EVENT_NAME='wait/synch/mutex/sql/Gtid_set::gtid_executed::free_intervals_mutex');
 SET GLOBAL binlog_group_commit_sync_delay=1000000;
 # Create two new connections: con1 and con2
 # At con1
@@ -23,7 +23,7 @@ include/assert.inc ["No gaps should exist in gtid_executed after the second inse
 SET @free_intervals_mutex_after= (
 SELECT COUNT_STAR
 FROM performance_schema.events_waits_summary_global_by_event_name WHERE
-EVENT_NAME='wait/synch/mutex/sql/Gtid_state::gtid_executed::free_intervals_mutex');
+EVENT_NAME='wait/synch/mutex/sql/Gtid_set::gtid_executed::free_intervals_mutex');
 include/assert.inc ["gtid_executed::free_intervals_mutex wasn't used"]
 SET GLOBAL binlog_group_commit_sync_delay=0;
 DROP TABLE t1, t2;
diff --git a/mysql-test/suite/binlog/t/binlog_group_commit_gtid_order.test b/mysql-test/suite/binlog/t/binlog_group_commit_gtid_order.test
@@ -37,7 +37,7 @@ CREATE TABLE t2 (c1 INT) Engine=InnoDB;
 SET @free_intervals_mutex_before= (
   SELECT COUNT_STAR
   FROM performance_schema.events_waits_summary_global_by_event_name WHERE
-  EVENT_NAME='wait/synch/mutex/sql/Gtid_state::gtid_executed::free_intervals_mutex');
+  EVENT_NAME='wait/synch/mutex/sql/Gtid_set::gtid_executed::free_intervals_mutex');
 
 # Set group commit parameters
 --eval SET GLOBAL binlog_group_commit_sync_delay=$delay
@@ -110,7 +110,7 @@ SET DEBUG_SYNC='now SIGNAL go_con1';
 SET @free_intervals_mutex_after= (
   SELECT COUNT_STAR
   FROM performance_schema.events_waits_summary_global_by_event_name WHERE
-  EVENT_NAME='wait/synch/mutex/sql/Gtid_state::gtid_executed::free_intervals_mutex');
+  EVENT_NAME='wait/synch/mutex/sql/Gtid_set::gtid_executed::free_intervals_mutex');
 --let $assert_text= "gtid_executed::free_intervals_mutex wasn't used"
 --let $assert_cond= @free_intervals_mutex_before = @free_intervals_mutex_after
 --source include/assert.inc
diff --git a/mysql-test/suite/perfschema/r/dml_setup_instruments.result b/mysql-test/suite/perfschema/r/dml_setup_instruments.result
@@ -7,8 +7,8 @@ NAME	ENABLED	TIMED
 wait/synch/mutex/sql/Commit_order_manager::m_mutex	YES	YES
 wait/synch/mutex/sql/Cost_constant_cache::LOCK_cost_const	YES	YES
 wait/synch/mutex/sql/Event_scheduler::LOCK_scheduler_state	YES	YES
+wait/synch/mutex/sql/Gtid_set::gtid_executed::free_intervals_mutex	YES	YES
 wait/synch/mutex/sql/Gtid_state	YES	YES
-wait/synch/mutex/sql/Gtid_state::gtid_executed::free_intervals_mutex	YES	YES
 wait/synch/mutex/sql/hash_filo::lock	YES	YES
 wait/synch/mutex/sql/key_mts_gaq_LOCK	YES	YES
 wait/synch/mutex/sql/key_mts_temp_table_LOCK	YES	YES
diff --git a/sql/mysqld.cc b/sql/mysqld.cc
@@ -8253,7 +8253,7 @@ PSI_mutex_key key_BINLOG_LOCK_sync_queue;
 PSI_mutex_key key_BINLOG_LOCK_xids;
 PSI_mutex_key
   key_hash_filo_lock, key_LOCK_msr_map,
-  Gtid_state::key_gtid_executed_free_intervals_mutex,
+  Gtid_set::key_gtid_executed_free_intervals_mutex,
   key_LOCK_crypt, key_LOCK_error_log,
   key_LOCK_gdl, key_LOCK_global_system_variables,
   key_LOCK_manager,
@@ -8329,7 +8329,7 @@ static PSI_mutex_info all_server_mutexes[]=
   { &key_RELAYLOG_LOCK_xids, "MYSQL_RELAY_LOG::LOCK_xids", 0},
   { &key_hash_filo_lock, "hash_filo::lock", 0},
   { &key_LOCK_msr_map, "LOCK_msr_map", PSI_FLAG_GLOBAL},
-  { &Gtid_state::key_gtid_executed_free_intervals_mutex, "Gtid_state::gtid_executed::free_intervals_mutex", 0 },
+  { &Gtid_set::key_gtid_executed_free_intervals_mutex, "Gtid_set::gtid_executed::free_intervals_mutex", 0 },
   { &key_LOCK_crypt, "LOCK_crypt", PSI_FLAG_GLOBAL},
   { &key_LOCK_error_log, "LOCK_error_log", PSI_FLAG_GLOBAL},
   { &key_LOCK_gdl, "LOCK_gdl", PSI_FLAG_GLOBAL},
diff --git a/sql/rpl_gtid.h b/sql/rpl_gtid.h
@@ -1077,6 +1077,9 @@ struct Gtid
 class Gtid_set
 {
 public:
+#ifdef HAVE_PSI_INTERFACE
+  static PSI_mutex_key key_gtid_executed_free_intervals_mutex;
+#endif
   /**
     Constructs a new, empty Gtid_set.
 
@@ -1088,11 +1091,7 @@ class Gtid_set
     @param free_intervals_mutex_key Performance_schema instrumentation
     key to use for the free_intervals mutex.
   */
-  Gtid_set(Sid_map *sid_map, Checkable_rwlock *sid_lock= NULL
-#ifdef HAVE_PSI_INTERFACE
-           ,PSI_mutex_key free_intervals_mutex_key= 0
-#endif
-          );
+  Gtid_set(Sid_map *sid_map, Checkable_rwlock *sid_lock= NULL);
   /**
     Constructs a new Gtid_set that contains the groups in the given string, in the same format as add_gtid_text(char *).
 
@@ -1112,18 +1111,10 @@ class Gtid_set
     there will be a short period when the lock is not held at all.
   */
   Gtid_set(Sid_map *sid_map, const char *text, enum_return_status *status,
-           Checkable_rwlock *sid_lock= NULL
-#ifdef HAVE_PSI_INTERFACE
-           ,PSI_mutex_key free_intervals_mutex_key= 0
-#endif
-          );
+           Checkable_rwlock *sid_lock= NULL);
 private:
   /// Worker for the constructor.
-  void init(
-#ifdef HAVE_PSI_INTERFACE
-            PSI_mutex_key free_intervals_mutex_key
-#endif
-           );
+  void init();
 public:
   /// Destroy this Gtid_set.
   ~Gtid_set();
@@ -2349,9 +2340,6 @@ class Owned_gtids
 class Gtid_state
 {
 public:
-#ifdef HAVE_PSI_INTERFACE
-  static PSI_mutex_key key_gtid_executed_free_intervals_mutex;
-#endif
   /**
     Constructs a new Gtid_state object.
 
@@ -2364,11 +2352,7 @@ class Gtid_state
     sid_map(_sid_map),
     sid_locks(sid_lock),
     lost_gtids(sid_map, sid_lock),
-    executed_gtids(sid_map, sid_lock
-#ifdef HAVE_PSI_INTERFACE
-                   ,key_gtid_executed_free_intervals_mutex
-#endif
-                  ),
+    executed_gtids(sid_map, sid_lock),
     gtids_only_in_table(sid_map, sid_lock),
     previous_gtids_logged(sid_map, sid_lock),
     owned_gtids(sid_lock) {}
diff --git a/sql/rpl_gtid_set.cc b/sql/rpl_gtid_set.cc
@@ -55,54 +55,35 @@ const Gtid_set::String_format Gtid_set::commented_string_format=
 };
 
 
-Gtid_set::Gtid_set(Sid_map *_sid_map, Checkable_rwlock *_sid_lock
-#ifdef HAVE_PSI_INTERFACE
-                   ,PSI_mutex_key free_intervals_mutex_key
-#endif
-                  )
+Gtid_set::Gtid_set(Sid_map *_sid_map, Checkable_rwlock *_sid_lock)
   : sid_lock(_sid_lock), sid_map(_sid_map),
     m_intervals(key_memory_Gtid_set_Interval_chunk)
 {
-  init(
-#ifdef HAVE_PSI_INTERFACE
-       free_intervals_mutex_key
-#endif
-      );
+  init();
 }
 
 
 Gtid_set::Gtid_set(Sid_map *_sid_map, const char *text,
-                   enum_return_status *status, Checkable_rwlock *_sid_lock
-#ifdef HAVE_PSI_INTERFACE
-                   ,PSI_mutex_key free_intervals_mutex_key
-#endif
-                  )
+                   enum_return_status *status, Checkable_rwlock *_sid_lock)
   : sid_lock(_sid_lock), sid_map(_sid_map),
     m_intervals(key_memory_Gtid_set_Interval_chunk)
 {
   DBUG_ASSERT(_sid_map != NULL);
-  init(
-#ifdef HAVE_PSI_INTERFACE
-       free_intervals_mutex_key
-#endif
-      );
+  init();
   *status= add_gtid_text(text);
 }
 
 
-void Gtid_set::init(
-#ifdef HAVE_PSI_INTERFACE
-                    PSI_mutex_key free_intervals_mutex_key
-#endif
-                   )
+void Gtid_set::init()
 {
   DBUG_ENTER("Gtid_set::init");
   cached_string_length= -1;
   cached_string_format= NULL;
   chunks= NULL;
   free_intervals= NULL;
   if (sid_lock)
-    mysql_mutex_init(free_intervals_mutex_key, &free_intervals_mutex, NULL);
+    mysql_mutex_init(key_gtid_executed_free_intervals_mutex,
+                     &free_intervals_mutex, NULL);
 #ifndef DBUG_OFF
   n_chunks= 0;
 #endif
