Bug #121122 Race in Performance Schema shared memory-stat capacity accounting can underflow size_t and corrupt high/low watermarks

• Submitted: 19 Aug 5:49
• Reporter: Yichang Song (OCA)
• Status: Open
• Category: MySQL Server: Performance Schema
• Severity: S3 (Non-critical)
• Version: 26.7.0
• OS: Any
• CPU Architecture: Any

[19 Aug 5:49] Yichang Song

Description:
PFS_memory_shared_stat aggregates Performance Schema memory statistics for
global, account, user, and host summaries. Its counters and capacity fields are
std::atomic<size_t>, but capacity consumption is implemented as separate
load/check and decrement/update operations.

This is not sufficient for concurrent callers. In particular, the following
functions can consume one or more capacity fields concurrently:

  PFS_memory_shared_stat::count_builtin_alloc()
  PFS_memory_shared_stat::count_builtin_free()
  PFS_memory_shared_stat::count_alloc()
  PFS_memory_shared_stat::count_free()
  PFS_memory_shared_stat::apply_alloc_delta()
  PFS_memory_shared_stat::apply_free_delta()

For example, count_alloc() previously used:

  if ((m_alloc_count_capacity >= 1) &&
      (m_alloc_size_capacity >= size)) {
    --m_alloc_count_capacity;
    m_alloc_size_capacity -= size;
    return nullptr;
  }

The condition and the decrements are separate atomic operations. If two threads
observe a count capacity of 1 and a size capacity equal to `size`, both can
enter this branch. The first decrements both capacities to zero, while the
second decrements zero and wraps the unsigned values to SIZE_MAX.

The resulting capacity is invalid. Capacity fields represent unconsumed
high-watermark or low-watermark changes. Once they wrap, later deltas may be
absorbed instead of propagated, so Performance Schema memory-summary
HIGH_*_USED or LOW_*_USED values can become inaccurate.

How to repeat:
The following interleaving demonstrates the bug in count_alloc(). Initialize a
PFS_memory_shared_stat instance as follows:

  m_alloc_count_capacity = 1
  m_alloc_size_capacity = 1024

Then concurrently call:

  stat.count_alloc(1024, &delta1);
  stat.count_alloc(1024, &delta2);

The problematic interleaving is:

Thread 1                                Thread 2
--------                                --------
read count capacity == 1
read size capacity == 1024
                                        read count capacity == 1
                                        read size capacity == 1024
decrement count capacity: 1 -> 0
decrement size capacity: 1024 -> 0
                                        decrement count capacity: 0 -> SIZE_MAX
                                        decrement size capacity: 0 -> SIZE_MAX

The same check-then-consume race exists in count_free(), apply_alloc_delta(),
and apply_free_delta(). The optimistic fetch_sub() plus compensating update in
count_builtin_alloc() and count_builtin_free() also does not make saturating
capacity consumption atomic.

Suggested fix:
Use a compare-exchange loop for all PFS_memory_shared_stat capacity
consumption. The helper should consume min(available, requested) atomically and
return requested - consumed:

```diff
diff --git a/storage/perfschema/pfs_stat.cc b/storage/perfschema/pfs_stat.cc
index e1e41d40373..80bba410940 100644
--- a/storage/perfschema/pfs_stat.cc
+++ b/storage/perfschema/pfs_stat.cc
@@ -33,6 +33,26 @@
   @{
 */
 
+/** Atomically consume capacity and return the unconsumed amount. */
+static size_t consume_capacity(std::atomic<size_t> &capacity,
+                               size_t requested) {
+  if (requested == 0) {
+    return 0;
+  }
+
+  size_t available = capacity.load();
+
+  while (available != 0) {
+    const size_t consumed = std::min(available, requested);
+
+    if (capacity.compare_exchange_weak(available, available - consumed)) {
+      return requested - consumed;
+    }
+  }
+
+  return requested;
+}
+
 void PFS_memory_safe_stat::reset() {
   m_used = false;
 
@@ -177,23 +197,8 @@ void PFS_memory_shared_stat::count_builtin_alloc(size_t size) {
   m_alloc_size += size;
   m_free_size_capacity += size;
 
-  size_t old_value;
-
-  /* Optimistic */
-  old_value = m_alloc_count_capacity.fetch_sub(1);
-
-  /* Adjustment */
-  if (old_value == 0) {
-    ++m_alloc_count_capacity;
-  }
-
-  /* Optimistic */
-  old_value = m_alloc_size_capacity.fetch_sub(size);
-
-  /* Adjustment */
-  if (old_value < size) {
-    m_alloc_size_capacity = 0;
-  }
+  (void)consume_capacity(m_alloc_count_capacity, 1);
+  (void)consume_capacity(m_alloc_size_capacity, size);
 }
 
 void PFS_memory_shared_stat::count_builtin_free(size_t size) {
@@ -204,23 +209,8 @@ void PFS_memory_shared_stat::count_builtin_free(size_t size) {
   m_free_size += size;
   m_alloc_size_capacity += size;
 
-  size_t old_value;
-
-  /* Optimistic */
-  old_value = m_free_count_capacity.fetch_sub(1);
-
-  /* Adjustment */
-  if (old_value == 0) {
-    ++m_free_count_capacity;
-  }
-
-  /* Optimistic */
-  old_value = m_free_size_capacity.fetch_sub(size);
-
-  /* Adjustment */
-  if (old_value < size) {
-    m_free_size_capacity = 0;
-  }
+  (void)consume_capacity(m_free_count_capacity, 1);
+  (void)consume_capacity(m_free_size_capacity, size);
 }
 
 PFS_memory_stat_alloc_delta *PFS_memory_shared_stat::count_alloc(
@@ -232,26 +222,17 @@ PFS_memory_stat_alloc_delta *PFS_memory_shared_stat::count_alloc(
   m_alloc_size += size;
   m_free_size_capacity += size;
 
-  if ((m_alloc_count_capacity >= 1) && (m_alloc_size_capacity >= size)) {
-    --m_alloc_count_capacity;
-    m_alloc_size_capacity -= size;
+  const size_t remaining_alloc_count =
+      consume_capacity(m_alloc_count_capacity, 1);
+  const size_t remaining_alloc_size =
+      consume_capacity(m_alloc_size_capacity, size);
+
+  if ((remaining_alloc_count == 0) && (remaining_alloc_size == 0)) {
     return nullptr;
   }
 
-  if (m_alloc_count_capacity >= 1) {
-    --m_alloc_count_capacity;
-    delta->m_alloc_count_delta = 0;
-  } else {
-    delta->m_alloc_count_delta = 1;
-  }
-
-  if (m_alloc_size_capacity >= size) {
-    m_alloc_size_capacity -= size;
-    delta->m_alloc_size_delta = 0;
-  } else {
-    delta->m_alloc_size_delta = size - m_alloc_size_capacity;
-    m_alloc_size_capacity = 0;
-  }
+  delta->m_alloc_count_delta = remaining_alloc_count;
+  delta->m_alloc_size_delta = remaining_alloc_size;
 
   return delta;
 }
@@ -265,26 +246,17 @@ PFS_memory_stat_free_delta *PFS_memory_shared_stat::count_free(
   m_free_size += size;
   m_alloc_size_capacity += size;
 
-  if ((m_free_count_capacity >= 1) && (m_free_size_capacity >= size)) {
-    --m_free_count_capacity;
-    m_free_size_capacity -= size;
+  const size_t remaining_free_count =
+      consume_capacity(m_free_count_capacity, 1);
+  const size_t remaining_free_size =
+      consume_capacity(m_free_size_capacity, size);
+
+  if ((remaining_free_count == 0) && (remaining_free_size == 0)) {
     return nullptr;
   }
 
-  if (m_free_count_capacity >= 1) {
-    --m_free_count_capacity;
-    delta->m_free_count_delta = 0;
-  } else {
-    delta->m_free_count_delta = 1;
-  }
-
-  if (m_free_size_capacity >= size) {
-    m_free_size_capacity -= size;
-    delta->m_free_size_delta = 0;
-  } else {
-    delta->m_free_size_delta = size - m_free_size_capacity;
-    m_free_size_capacity = 0;
-  }
+  delta->m_free_count_delta = remaining_free_count;
+  delta->m_free_size_delta = remaining_free_size;
 
   return delta;
 }
@@ -292,38 +264,14 @@ PFS_memory_stat_free_delta *PFS_memory_shared_stat::count_free(
 PFS_memory_stat_alloc_delta *PFS_memory_shared_stat::apply_alloc_delta(
     const PFS_memory_stat_alloc_delta *delta,
     PFS_memory_stat_alloc_delta *delta_buffer) {
-  size_t val;
-  size_t remaining_alloc_count = 0;
-  size_t remaining_alloc_size = 0;
-  bool has_remaining = false;
-
   m_used = true;
 
-  val = delta->m_alloc_count_delta;
-  if (val > 0) {
-    if (val <= m_alloc_count_capacity) {
-      m_alloc_count_capacity -= val;
-      remaining_alloc_count = 0;
-    } else {
-      remaining_alloc_count = val - m_alloc_count_capacity;
-      m_alloc_count_capacity = 0;
-      has_remaining = true;
-    }
-  }
-
-  val = delta->m_alloc_size_delta;
-  if (val > 0) {
-    if (val <= m_alloc_size_capacity) {
-      m_alloc_size_capacity -= val;
-      remaining_alloc_size = 0;
-    } else {
-      remaining_alloc_size = val - m_alloc_size_capacity;
-      m_alloc_size_capacity = 0;
-      has_remaining = true;
-    }
-  }
+  const size_t remaining_alloc_count = consume_capacity(
+      m_alloc_count_capacity, delta->m_alloc_count_delta);
+  const size_t remaining_alloc_size =
+      consume_capacity(m_alloc_size_capacity, delta->m_alloc_size_delta);
 
-  if (!has_remaining) {
+  if ((remaining_alloc_count == 0) && (remaining_alloc_size == 0)) {
     return nullptr;
   }
 
@@ -335,38 +283,14 @@ PFS_memory_stat_alloc_delta *PFS_memory_shared_stat::apply_alloc_delta(
 PFS_memory_stat_free_delta *PFS_memory_shared_stat::apply_free_delta(
     const PFS_memory_stat_free_delta *delta,
     PFS_memory_stat_free_delta *delta_buffer) {
-  size_t val;
-  size_t remaining_free_count = 0;
-  size_t remaining_free_size = 0;
-  bool has_remaining = false;
-
   m_used = true;
 
-  val = delta->m_free_count_delta;
-  if (val > 0) {
-    if (val <= m_free_count_capacity) {
-      m_free_count_capacity -= val;
-      remaining_free_count = 0;
-    } else {
-      remaining_free_count = val - m_free_count_capacity;
-      m_free_count_capacity = 0;
-      has_remaining = true;
-    }
-  }
-
-  val = delta->m_free_size_delta;
-  if (val > 0) {
-    if (val <= m_free_size_capacity) {
-      m_free_size_capacity -= val;
-      remaining_free_size = 0;
-    } else {
-      remaining_free_size = val - m_free_size_capacity;
-      m_free_size_capacity = 0;
-      has_remaining = true;
-    }
-  }
+  const size_t remaining_free_count =
+      consume_capacity(m_free_count_capacity, delta->m_free_count_delta);
+  const size_t remaining_free_size =
+      consume_capacity(m_free_size_capacity, delta->m_free_size_delta);
 
-  if (!has_remaining) {
+  if ((remaining_free_count == 0) && (remaining_free_size == 0)) {
     return nullptr;
   }

[19 Aug 5:55] Yichang Song

bugfix patch

(*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: pfs_memory_capacity_race_fix.diff (application/octet-stream, text), 6.87 KiB.