BUG#35998554: get_group_member_info_*() does not distinguish member not found from error

It was discovered that some methods from `Group_member_info_manager` class did return the same value for distinct situations, which could be masquerading errors. One case is ``` Group_member_info * Group_member_info_manager::get_group_member_info_by_member_id( const Gcs_member_identifier &id); ``` that does return a allocated copy of the `member_info` when the member exists on the `Group_member_info_manager`, or returns `nullprt` when the member does not exist. Though `nullptr` can also be returned when the memory allocation of the copy fails, which means that the method caller is unable to distinguish: * member not found; * no memory available to construct the object. On both situations the method caller will interpret `nullptr` as member not found which can leverage incorrect decisions. The same pattern exists on the methods: * Group_member_info_manager::get_group_member_info() * Group_member_info_manager::get_group_member_info_by_index() * Group_member_info_manager::get_group_member_info_by_member_id() * Group_member_info_manager::get_primary_member_info() To solve the above issue, the four methods were refactored to: 1) return a boolean value to state if the member was found; 2) receive a out parameter that is a local reference to the method caller that is updated when the member is found. The use of the reference eliminates the allocation of the memory. Change-Id: Ic10263943fc63f40cdda506a59f10962aa208d92

Author: nacarvalho

plugin/group_replication/include/member_info.h

@@ -42,6 +42,7 @@
 
 #include "my_inttypes.h"
 #include "my_sys.h"
+#include "mysql/binlog/event/nodiscard.h"
 #include "plugin/group_replication/include/gcs_plugin_messages.h"
 #include "plugin/group_replication/include/member_version.h"
 #include "plugin/group_replication/include/plugin_psi.h"
@@ -253,6 +254,14 @@ class Group_member_info : public Plugin_gcs_message {
  */
  void operator delete(void *ptr) noexcept { my_free(ptr); }
 
+ /**
+ Group_member_info empty constructor
+
+ @param[in] psi_mutex_key_arg mutex key
+ */
+ Group_member_info(PSI_mutex_key psi_mutex_key_arg =
+ key_GR_LOCK_group_member_info_update_lock);
+
  /**
  Group_member_info constructor
 
@@ -274,13 +283,13 @@ class Group_member_info : public Plugin_gcs_message {
  check
  @param[in] member_weight_arg member_weight
  @param[in] lower_case_table_names_arg lower case table names
- @param[in] psi_mutex_key_arg mutex key
  @param[in] default_table_encryption_arg default_table_encryption
  @param[in] recovery_endpoints_arg recovery endpoints
  @param[in] view_change_uuid_arg view change uuid
  advertised
  @param[in] allow_single_leader flag indicating whether or
  not to use single-leader behavior
+ @param[in] psi_mutex_key_arg mutex key
  */
  Group_member_info(const char *hostname_arg, uint port_arg,
  const char *uuid_arg, int write_set_extraction_algorithm,
@@ -707,8 +716,8 @@ class Group_member_info : public Plugin_gcs_message {
  uint port;
  std::string uuid;
  Group_member_status status;
- Gcs_member_identifier *gcs_member_id;
- Member_version *member_version;
+ Gcs_member_identifier *gcs_member_id{nullptr};
+ Member_version *member_version{nullptr};
  std::string executed_gtid_set;
  std::string purged_gtid_set;
  std::string retrieved_gtid_set;
@@ -784,21 +793,32 @@ class Group_member_info_manager_interface {
  /**
  Retrieves a registered Group member by its uuid
 
- @param[in] uuid uuid to retrieve
- @return reference to a copy of Group_member_info. NULL if not managed.
- The return value must deallocated by the caller.
+ @param[in] uuid uuid to retrieve
+ @param[out] member_info_arg a member info reference local to the
+ method caller that is updated when the
+ member is found.
+
+ @return true if the member is not found.
+ false if the member is found.
  */
- virtual Group_member_info *get_group_member_info(const std::string &uuid) = 0;
+ [[NODISCARD]] virtual bool get_group_member_info(
+ const std::string &uuid, Group_member_info &member_info_arg) = 0;
 
  /**
  Retrieves a registered Group member by an index function.
  One is free to determine the index function. Nevertheless, it should have
  the same result regardless of the member of the group where it is called
 
- @param[in] idx the index
- @return reference to a Group_member_info. NULL if not managed
+ @param[in] idx the index
+ @param[out] member_info_arg a member info reference local to the
+ method caller that is updated when the
+ member is found.
+
+ @return true if the member is not found.
+ false if the member is found.
  */
- virtual Group_member_info *get_group_member_info_by_index(int idx) = 0;
+ [[NODISCARD]] virtual bool get_group_member_info_by_index(
+ int idx, Group_member_info &member_info_arg) = 0;
 
  /**
  Return lowest member version.
@@ -811,12 +831,16 @@ class Group_member_info_manager_interface {
  /**
  Retrieves a registered Group member by its backbone GCS identifier.
 
- @param[in] id the GCS identifier
- @return reference to a copy of Group_member_info. NULL if not managed.
- The return value must be deallocated by the caller.
+ @param[in] id the GCS identifier
+ @param[out] member_info_arg a member info reference local to the
+ method caller that is updated when the
+ member is found.
+
+ @return true if the member is not found.
+ false if the member is found.
  */
- virtual Group_member_info *get_group_member_info_by_member_id(
- const Gcs_member_identifier &id) = 0;
+ [[NODISCARD]] virtual bool get_group_member_info_by_member_id(
+ const Gcs_member_identifier &id, Group_member_info &member_info_arg) = 0;
 
  /**
  Returns the member-uuid of the given GCS identifier.
@@ -1003,11 +1027,15 @@ class Group_member_info_manager_interface {
  /**
  Return the group member info for the current group primary
 
- @note the returned reference must be deallocated by the caller.
+ @param[out] member_info_arg a member info reference local to the
+ method caller that is updated when the
+ member is found.
 
- @return reference to a Group_member_info. NULL if not managed
+ @return true if the member is not found.
+ false if the member is found.
  */
- virtual Group_member_info *get_primary_member_info() = 0;
+ [[NODISCARD]] virtual bool get_primary_member_info(
+ Group_member_info &member_info_arg) = 0;
 
  /**
  Check if majority of the group is unreachable
@@ -1132,14 +1160,17 @@ class Group_member_info_manager : public Group_member_info_manager_interface {
 
  bool is_member_info_present(const std::string &uuid) override;
 
- Group_member_info *get_group_member_info(const std::string &uuid) override;
+ [[NODISCARD]] bool get_group_member_info(
+ const std::string &uuid, Group_member_info &member_info_arg) override;
 
- Group_member_info *get_group_member_info_by_index(int idx) override;
+ [[NODISCARD]] bool get_group_member_info_by_index(
+ int idx, Group_member_info &member_info_arg) override;
 
  Member_version get_group_lowest_online_version() override;
 
- Group_member_info *get_group_member_info_by_member_id(
- const Gcs_member_identifier &id) override;
+ [[NODISCARD]] bool get_group_member_info_by_member_id(
+ const Gcs_member_identifier &id,
+ Group_member_info &member_info_arg) override;
 
  std::pair<bool, std::string> get_group_member_uuid_from_member_id(
  const Gcs_member_identifier &id) override;
@@ -1193,7 +1224,8 @@ class Group_member_info_manager : public Group_member_info_manager_interface {
 
  bool get_primary_member_uuid(std::string &primary_member_uuid) override;
 
- Group_member_info *get_primary_member_info() override;
+ [[NODISCARD]] bool get_primary_member_info(
+ Group_member_info &member_info_arg) override;
 
  bool is_majority_unreachable() override;
 

plugin/group_replication/src/gcs_event_handlers.cc

@@ -377,13 +377,10 @@ void Plugin_gcs_events_handler::handle_recovery_message(
  */
  disable_read_mode_for_compatible_members(true);
  } else {
- Group_member_info *member_info =
- group_member_mgr->get_group_member_info(member_uuid);
- if (member_info != nullptr) {
+ Group_member_info member_info;
+ if (!group_member_mgr->get_group_member_info(member_uuid, member_info)) {
  LogPluginErr(SYSTEM_LEVEL, ER_GRP_RPL_MEM_ONLINE,
- member_info->get_hostname().c_str(),
- member_info->get_port());
- delete member_info;
+ member_info.get_hostname().c_str(), member_info.get_port());
 
  /*
  The member is declared as online upon receiving this message
@@ -662,38 +659,42 @@ void Plugin_gcs_events_handler::on_suspicions(
  if (!members.empty()) {
  for (mit = members.begin(); mit != members.end(); mit++) {
  Gcs_member_identifier member = *mit;
- Group_member_info *member_info =
- group_member_mgr->get_group_member_info_by_member_id(member);
-
- if (member_info == nullptr) // Trying to update a non-existing member
- continue; /* purecov: inspected */
+ Group_member_info member_info;
+
+ if (group_member_mgr->get_group_member_info_by_member_id(member,
+ member_info)) {
+ LogPluginErr(WARNING_LEVEL, ER_GRP_RPL_MEMBER_INFO_DOES_NOT_EXIST,
+ "by the Gcs_member_identifier",
+ member.get_member_id().c_str(),
+ "REACHABLE/UNREACHABLE notification from group "
+ "communication engine");
+ continue; /* purecov: inspected */
+ }
 
  uit = std::find(tmp_unreachable.begin(), tmp_unreachable.end(), member);
  if (uit != tmp_unreachable.end()) {
- if (!member_info->is_unreachable()) {
+ if (!member_info.is_unreachable()) {
  LogPluginErr(WARNING_LEVEL, ER_GRP_RPL_MEM_UNREACHABLE,
- member_info->get_hostname().c_str(),
- member_info->get_port());
+ member_info.get_hostname().c_str(),
+ member_info.get_port());
  // flag as a member having changed state
  m_notification_ctx.set_member_state_changed();
- group_member_mgr->set_member_unreachable(member_info->get_uuid());
+ group_member_mgr->set_member_unreachable(member_info.get_uuid());
  }
  // remove to not check again against this one
  tmp_unreachable.erase(uit);
  } else {
- if (member_info->is_unreachable()) {
+ if (member_info.is_unreachable()) {
  LogPluginErr(WARNING_LEVEL, ER_GRP_RPL_MEM_REACHABLE,
- member_info->get_hostname().c_str(),
- member_info->get_port());
+ member_info.get_hostname().c_str(),
+ member_info.get_port());
  /* purecov: begin inspected */
  // flag as a member having changed state
  m_notification_ctx.set_member_state_changed();
- group_member_mgr->set_member_reachable(member_info->get_uuid());
+ group_member_mgr->set_member_reachable(member_info.get_uuid());
  /* purecov: end */
  }
  }
-
- delete member_info;
  }
  }
 
@@ -790,31 +791,30 @@ void Plugin_gcs_events_handler::get_hosts_from_view(
  members.begin();
 
  while (all_members_it != members.end()) {
- Group_member_info *member_info =
- group_member_mgr->get_group_member_info_by_member_id((*all_members_it));
+ Group_member_info member_info;
+ const bool member_not_found =
+ group_member_mgr->get_group_member_info_by_member_id((*all_members_it),
+ member_info);
  all_members_it++;
 
- if (member_info == nullptr) continue;
+ if (member_not_found) continue;
 
- hosts_string << member_info->get_hostname() << ":"
- << member_info->get_port();
+ hosts_string << member_info.get_hostname() << ":" << member_info.get_port();
 
  /**
  Check in_primary_mode has been added for safety.
  Since primary role is in single-primary mode.
  */
- if (member_info->in_primary_mode() &&
- member_info->get_role() == Group_member_info::MEMBER_ROLE_PRIMARY) {
+ if (member_info.in_primary_mode() &&
+ member_info.get_role() == Group_member_info::MEMBER_ROLE_PRIMARY) {
  if (primary_string.rdbuf()->in_avail() != 0) primary_string << ", ";
- primary_string << member_info->get_hostname() << ":"
- << member_info->get_port();
+ primary_string << member_info.get_hostname() << ":"
+ << member_info.get_port();
  }
 
  if (all_members_it != members.end()) {
  hosts_string << ", ";
  }
-
- delete member_info;
  }
  all_hosts.assign(hosts_string.str());
  primary_host.assign(primary_string.str());
@@ -1435,13 +1435,12 @@ int Plugin_gcs_events_handler::process_local_exchanged_data(
  Gcs_member_identifier *member_id = exchanged_data_it->first;
  if (data == nullptr) {
  /* purecov: begin inspected */
- Group_member_info *member_info =
-  group_member_mgr->get_group_member_info_by_member_id(*member_id);
- if (member_info != nullptr) {
+ Group_member_info member_info;
+ if (!group_member_mgr->get_group_member_info_by_member_id(*member_id,
+  member_info)) {
  LogPluginErr(ERROR_LEVEL, ER_GRP_RPL_DATA_NOT_PROVIDED_BY_MEM,
- member_info->get_hostname().c_str(),
- member_info->get_port());
- delete member_info;
+ member_info.get_hostname().c_str(),
+ member_info.get_port());
  }
  continue;
  /* purecov: end */
@@ -1690,10 +1689,9 @@ void Plugin_gcs_events_handler::update_member_status(
  for (vector<Gcs_member_identifier>::const_iterator it = members.begin();
  it != members.end(); ++it) {
  Gcs_member_identifier member = *it;
- Group_member_info *member_info =
- group_member_mgr->get_group_member_info_by_member_id(member);
-
- if (member_info == nullptr) {
+ Group_member_info member_info;
+ if (group_member_mgr->get_group_member_info_by_member_id(member,
+ member_info)) {
  // Trying to update a non-existing member
  continue;
  }
@@ -1704,18 +1702,16 @@ void Plugin_gcs_events_handler::update_member_status(
  // (the old_status_different_from is not defined or
  // the previous status is different from old_status_different_from)
  if ((old_status_equal_to == Group_member_info::MEMBER_END ||
- member_info->get_recovery_status() == old_status_equal_to) &&
+ member_info.get_recovery_status() == old_status_equal_to) &&
  (old_status_different_from == Group_member_info::MEMBER_END ||
- member_info->get_recovery_status() != old_status_different_from)) {
+ member_info.get_recovery_status() != old_status_different_from)) {
  /*
  The notification will be handled on the top level handle
  function that calls this one down the stack.
  */
- group_member_mgr->update_member_status(member_info->get_uuid(), status,
+ group_member_mgr->update_member_status(member_info.get_uuid(), status,
  m_notification_ctx);
  }
-
- delete member_info;
  }
 }
 

plugin/group_replication/src/group_actions/communication_protocol_action.cc

@@ -53,15 +53,13 @@ int Communication_protocol_action::set_consensus_leaders() const {
  Gcs_member_identifier const my_gcs_id =
  local_member_info->get_gcs_member_id();
  if (is_single_primary_mode) {
- Group_member_info *primary_info =
- group_member_mgr->get_primary_member_info();
- if (primary_info == nullptr) {
+ Group_member_info primary_info;
+ if (group_member_mgr->get_primary_member_info(primary_info)) {
  return 1;
  }
 
  Gcs_member_identifier const primary_gcs_id =
- primary_info->get_gcs_member_id();
- delete primary_info;
+ primary_info.get_gcs_member_id();
  bool const am_i_the_primary = (my_gcs_id == primary_gcs_id);
  my_role = (am_i_the_primary ? Group_member_info::MEMBER_ROLE_PRIMARY
  : Group_member_info::MEMBER_ROLE_SECONDARY);

plugin/group_replication/src/group_actions/multi_primary_migration_action.cc

@@ -82,12 +82,11 @@ int Multi_primary_migration_action::process_action_message(
  return 1; /* purecov: inspected */
  }
 
- Group_member_info *primary_info = group_member_mgr->get_primary_member_info();
- if (primary_info != nullptr) {
- primary_uuid.assign(primary_info->get_uuid());
- primary_gcs_id.assign(primary_info->get_gcs_member_id().get_member_id());
+ Group_member_info primary_info;
+ if (!group_member_mgr->get_primary_member_info(primary_info)) {
+ primary_uuid.assign(primary_info.get_uuid());
+ primary_gcs_id.assign(primary_info.get_gcs_member_id().get_member_id());
  is_primary = !primary_uuid.compare(local_member_info->get_uuid());
- delete primary_info;
  }
 
  group_events_observation_manager->register_group_event_observer(this);