Bug #96552 Replication filter iterator needs to be re-assigned after erase() in looping

• Submitted: 15 Aug 2019 12:55
• Modified: 4 Nov 2019 9:42
• Reporter: Xing Ai (OCA)
• Status: Won't fix
• Category: MySQL Server: Replication
• Severity: S5 (Performance)
• Version: 8.0
• OS: Any
• CPU Architecture: Any
• Tags: multisource replication, SQL

[15 Aug 2019 12:55] Xing Ai

Description:
This problem is found through code review instead of testing.

The source file "https://github.com/mysql/mysql-server/blob/8.0/sql/rpl_msr.cc" has iterator erase problem in looping. This
 may cause undefined behavior or stability problem.

The iterator becomes invalid after its element is removed. "it++" would cause undefined behavior due to such invalidation.

How to repeat:
In the code snippet below, the line of "channel_to_filter.erase(it);" has problem.

void Rpl_channel_filters::delete_filter(Rpl_filter *rpl_filter) {
  DBUG_ENTER("Rpl_channel_filters::delete_filter");

  /* Traverse the filter map. */
  m_channel_to_filter_lock->wrlock();
  for (filter_map::iterator it = channel_to_filter.begin();
       it != channel_to_filter.end(); it++) {
    if (it->second == rpl_filter) {
      /* Find the replication filter and delete it. */
      delete it->second;
      channel_to_filter.erase(it);
#ifdef WITH_PERFSCHEMA_STORAGE_ENGINE
      reset_pfs_view();
#endif /* WITH_PERFSCHEMA_STORAGE_ENGINE */
      m_channel_to_filter_lock->unlock();
      DBUG_VOID_RETURN;
    }
  }
  m_channel_to_filter_lock->unlock();

  DBUG_VOID_RETURN;
}

Suggested fix:
The fix of one line change is very subtle as below.

void Rpl_channel_filters::delete_filter(Rpl_filter *rpl_filter) {
  DBUG_ENTER("Rpl_channel_filters::delete_filter");

  /* Traverse the filter map. */
  m_channel_to_filter_lock->wrlock();
  for (filter_map::iterator it = channel_to_filter.begin();
       it != channel_to_filter.end(); it++) {
    if (it->second == rpl_filter) {
      /* Find the replication filter and delete it. */
      delete it->second;
      it = channel_to_filter.erase(it);
#ifdef WITH_PERFSCHEMA_STORAGE_ENGINE
      reset_pfs_view();
#endif /* WITH_PERFSCHEMA_STORAGE_ENGINE */
      m_channel_to_filter_lock->unlock();
      DBUG_VOID_RETURN;
    }
  }
  m_channel_to_filter_lock->unlock();

  DBUG_VOID_RETURN;
}

[19 Aug 2019 10:49] Bogdan Kecman

Hi,

Seems you are right. Thanks for submitting the report.

In order to submit contributions you must first sign the Oracle Contribution Agreement (OCA).
For additional information please check http://www.oracle.com/technetwork/community/oca-486395.html.
If you have any questions, please contact the MySQL community team.

[21 Aug 2019 13:07] Xing Ai

Thanks for looking into the bug. I'll sign the OCA.

[25 Oct 2019 1:36] Pedro Figueiredo

Posted by developer:
 
Hello.

Thank you for taking the time to look into this code block.
The iterator increment is never executed since the function returns when the element is removed so, no impact on the invalidated iterator.

Nonetheless, for the sake of correctness and to avoid future issues, kept the code, with a few changes.

Thank you for your time.

[4 Nov 2019 9:42] Pedro Figueiredo

Posted by developer:
 
Hello.

After further analysing the issue, decided to keep the current form since it's the most optimized implementation of the heuristics.

Thank you for your time.