From ad8a47e5e1a3d7e59ecea145e55c182ba1059f37 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Tue, 10 Mar 2026 13:51:38 -0700 Subject: [PATCH] fix: scope partition maintenance to target table and lock create path Refs #271 Signed-off-by: Thomas Vincent --- functions.php | 177 ++++++++++++---- .../issue254_partition_table_locking_test.php | 189 ++++++++++++++++++ 2 files changed, 326 insertions(+), 40 deletions(-) create mode 100644 tests/regression/issue254_partition_table_locking_test.php diff --git a/functions.php b/functions.php index 9bd3223..3a9d746 100644 --- a/functions.php +++ b/functions.php @@ -187,86 +187,184 @@ function syslog_partition_manage() { } /** - * This function will create a new partition for the specified table. + * Validate tables that support partition maintenance. + * + * Any value added to the allowlist MUST match ^[a-z_]+$ so it is safe + * for identifier interpolation in DDL statements (MySQL does not support + * parameter binding for identifiers). + */ +function syslog_partition_table_allowed($table) { + if (!in_array($table, array('syslog', 'syslog_removed'), true)) { + return false; + } + + /* Defense-in-depth: reject values unsafe for identifier interpolation. */ + if (!preg_match('/^[a-z_]+$/', $table)) { + return false; + } + + return true; +} + +/** + * Create a new partition for the specified table. + * + * @return bool true on success, false on lock failure or disallowed table. */ function syslog_partition_create($table) { global $syslogdb_default; - /* determine the format of the table name */ - $time = time(); - $cformat = 'd' . date('Ymd', $time); - $lnow = date('Y-m-d', $time+86400); + if (!syslog_partition_table_allowed($table)) { + return false; + } - $exists = syslog_db_fetch_row("SELECT * - FROM `information_schema`.`partitions` - WHERE table_schema='" . $syslogdb_default . "' - AND partition_name='" . $cformat . "' - AND table_name='syslog' - ORDER BY partition_ordinal_position"); + /* Hash to guarantee the lock name stays within MySQL's 64-byte limit. */ + $lock_name = substr(hash('sha256', $syslogdb_default . '.syslog_partition_create.' . $table), 0, 60); - if (!cacti_sizeof($exists)) { - cacti_log("SYSLOG: Creating new partition '$cformat'", false, 'SYSTEM'); + /* + * 10-second timeout is sufficient: partition maintenance runs once per + * poller cycle (typically 5 minutes), so sustained contention is not + * expected. A failure is logged so monitoring can detect repeated misses. + */ + $locked = syslog_db_fetch_cell_prepared('SELECT GET_LOCK(?, 10)', array($lock_name)); - syslog_debug("Creating new partition '$cformat'"); + if ($locked === null) { + /* NULL means the GET_LOCK call itself failed, not just contention. */ + cacti_log("SYSLOG: GET_LOCK call failed for partition create on '$table'", false, 'SYSTEM'); + return false; + } - syslog_db_execute("ALTER TABLE `" . $syslogdb_default . "`.`$table` REORGANIZE PARTITION dMaxValue INTO ( - PARTITION $cformat VALUES LESS THAN (TO_DAYS('$lnow')), - PARTITION dMaxValue VALUES LESS THAN MAXVALUE)"); + if ((int)$locked !== 1) { + cacti_log("SYSLOG: Unable to acquire partition create lock for '$table'", false, 'SYSTEM'); + return false; } + + try { + /* determine the format of the table name */ + $time = time(); + $cformat = 'd' . date('Ymd', $time); + $lnow = date('Y-m-d', $time+86400); + + $exists = syslog_db_fetch_row_prepared("SELECT * + FROM `information_schema`.`partitions` + WHERE table_schema = ? + AND partition_name = ? + AND table_name = ? + ORDER BY partition_ordinal_position", + array($syslogdb_default, $cformat, $table)); + + if (!cacti_sizeof($exists)) { + cacti_log("SYSLOG: Creating new partition '$cformat'", false, 'SYSTEM'); + + syslog_debug("Creating new partition '$cformat'"); + + /* + * MySQL does not support parameter binding for DDL identifiers + * or partition definitions. $table is safe because it passed + * syslog_partition_table_allowed() (two-value allowlist plus + * regex guard). $cformat and $lnow derive from date() and + * contain only digits, hyphens, and the letter 'd'. + */ + syslog_db_execute("ALTER TABLE `" . $syslogdb_default . "`.`$table` REORGANIZE PARTITION dMaxValue INTO ( + PARTITION $cformat VALUES LESS THAN (TO_DAYS('$lnow')), + PARTITION dMaxValue VALUES LESS THAN MAXVALUE)"); + } + } finally { + syslog_db_fetch_cell_prepared('SELECT RELEASE_LOCK(?)', array($lock_name)); + } + + return true; } /** - * This function will remove all old partitions for the specified table. + * Remove old partitions for the specified table. */ function syslog_partition_remove($table) { global $syslogdb_default; + if (!syslog_partition_table_allowed($table)) { + cacti_log("SYSLOG: partition_remove called with disallowed table '$table'", false, 'SYSTEM'); + return 0; + } + + $lock_name = substr(hash('sha256', $syslogdb_default . '.syslog_partition_remove.' . $table), 0, 60); + + $locked = syslog_db_fetch_cell_prepared('SELECT GET_LOCK(?, 10)', array($lock_name)); + + if ($locked === null) { + cacti_log("SYSLOG: GET_LOCK call failed for partition remove on '$table'", false, 'SYSTEM'); + return 0; + } + + if ((int)$locked !== 1) { + cacti_log("SYSLOG: Unable to acquire partition remove lock for '$table'", false, 'SYSTEM'); + return 0; + } + $syslog_deleted = 0; - $number_of_partitions = syslog_db_fetch_assoc("SELECT * - FROM `information_schema`.`partitions` - WHERE table_schema='" . $syslogdb_default . "' AND table_name='syslog' - ORDER BY partition_ordinal_position"); - $days = read_config_option('syslog_retention'); + try { + $number_of_partitions = syslog_db_fetch_assoc_prepared("SELECT * + FROM `information_schema`.`partitions` + WHERE table_schema = ? AND table_name = ? + ORDER BY partition_ordinal_position", + array($syslogdb_default, $table)); + + $days = read_config_option('syslog_retention'); - syslog_debug("There are currently '" . sizeof($number_of_partitions) . "' Syslog Partitions, We will keep '$days' of them."); + syslog_debug("There are currently '" . sizeof($number_of_partitions) . "' Syslog Partitions, We will keep '$days' of them."); - if ($days > 0) { - $user_partitions = sizeof($number_of_partitions) - 1; - if ($user_partitions >= $days) { - $i = 0; - while ($user_partitions > $days) { - $oldest = $number_of_partitions[$i]; + if ($days > 0) { + $user_partitions = sizeof($number_of_partitions) - 1; + if ($user_partitions >= $days) { + $i = 0; + while ($user_partitions > $days) { + $oldest = $number_of_partitions[$i]; - cacti_log("SYSLOG: Removing old partition '" . $oldest['PARTITION_NAME'] . "'", false, 'SYSTEM'); + cacti_log("SYSLOG: Removing old partition '" . $oldest['PARTITION_NAME'] . "'", false, 'SYSTEM'); - syslog_debug("Removing partition '" . $oldest['PARTITION_NAME'] . "'"); + syslog_debug("Removing partition '" . $oldest['PARTITION_NAME'] . "'"); - syslog_db_execute("ALTER TABLE `" . $syslogdb_default . "`.`$table` DROP PARTITION " . $oldest['PARTITION_NAME']); + syslog_db_execute("ALTER TABLE `" . $syslogdb_default . "`.`$table` DROP PARTITION " . $oldest['PARTITION_NAME']); - $i++; - $user_partitions--; - $syslog_deleted++; + $i++; + $user_partitions--; + $syslog_deleted++; + } } } + } finally { + syslog_db_fetch_cell_prepared('SELECT RELEASE_LOCK(?)', array($lock_name)); } return $syslog_deleted; } +/* + * syslog_partition_check is a read-only SELECT against information_schema. + * It does not execute DDL, so it does not need the named lock that + * syslog_partition_create and syslog_partition_remove acquire. External + * serialization is provided by the poller cycle calling + * syslog_partition_manage(). + */ function syslog_partition_check($table) { global $syslogdb_default; + if (!syslog_partition_table_allowed($table)) { + return false; + } + if (defined('SYSLOG_CONFIG')) { include(SYSLOG_CONFIG); } /* find date of last partition */ - $last_part = syslog_db_fetch_cell("SELECT PARTITION_NAME + $last_part = syslog_db_fetch_cell_prepared("SELECT PARTITION_NAME FROM `information_schema`.`partitions` - WHERE table_schema='" . $syslogdb_default . "' AND table_name='syslog' + WHERE table_schema = ? AND table_name = ? ORDER BY partition_ordinal_position DESC - LIMIT 1,1;"); + LIMIT 1,1", + array($syslogdb_default, $table)); $lformat = str_replace('d', '', $last_part); $cformat = date('Ymd'); @@ -2421,4 +2519,3 @@ function alert_replace_variables($alert, $results, $hostname = '') { return $command; } - diff --git a/tests/regression/issue254_partition_table_locking_test.php b/tests/regression/issue254_partition_table_locking_test.php new file mode 100644 index 0000000..dfccddb --- /dev/null +++ b/tests/regression/issue254_partition_table_locking_test.php @@ -0,0 +1,189 @@ + 0) { + fwrite(STDERR, "Found $raw_partition_queries raw (non-prepared) information_schema partition queries; all must use _prepared.\n"); + exit(1); +} + +/* ---- syslog_partition_remove must also use GET_LOCK / RELEASE_LOCK in a finally block ---- */ + +if (!preg_match('/function\s+syslog_partition_remove\s*\(\s*\$table\s*\)\s*\{(.{0,2500})\n\}/s', $functions, $m_remove_lock)) { + fwrite(STDERR, "syslog_partition_remove function body not found for lock check.\n"); + exit(1); +} +if (!preg_match('/GET_LOCK/', $m_remove_lock[1])) { + fwrite(STDERR, "syslog_partition_remove does not acquire a lock before ALTER TABLE.\n"); + exit(1); +} +if (!preg_match('/finally\s*\{[^}]*RELEASE_LOCK/s', $m_remove_lock[1])) { + fwrite(STDERR, "syslog_partition_remove does not release its lock in a finally block.\n"); + exit(1); +} + +/* ---- Lock names must differ between create and remove (per-operation scoping) ---- */ + +if (!preg_match('/syslog_partition_create\.\'\s*\.\s*\$table/', $functions)) { + fwrite(STDERR, "syslog_partition_create lock name does not include function scope.\n"); + exit(1); +} +if (!preg_match('/syslog_partition_remove\.\'\s*\.\s*\$table/', $functions)) { + fwrite(STDERR, "syslog_partition_remove lock name does not include function scope.\n"); + exit(1); +} + +/* ---- syslog_partition_create must return early (no DDL) when allowlist fails ---- */ + +if (!preg_match('/function\s+syslog_partition_create\s*\(\s*\$table\s*\)\s*\{(.{0,300})/s', $functions, $m_create_guard)) { + fwrite(STDERR, "syslog_partition_create function not found.\n"); + exit(1); +} +if (!preg_match('/!syslog_partition_table_allowed[^}]*return\s+false;/s', $m_create_guard[1])) { + fwrite(STDERR, "syslog_partition_create does not return early for disallowed tables.\n"); + exit(1); +} + +/* ---- syslog_partition_check and syslog_partition_remove must use _prepared for info_schema ---- */ + +if (!preg_match('/function\s+syslog_partition_check\s*\(\s*\$table\s*\)\s*\{(.{0,800})/s', $functions, $m_check_prep)) { + fwrite(STDERR, "syslog_partition_check function not found for _prepared check.\n"); + exit(1); +} +if (!preg_match('/syslog_db_fetch_cell_prepared[^)]*information_schema[^)]*table_name\s*=\s*\?/s', $m_check_prep[1])) { + fwrite(STDERR, "syslog_partition_check does not use _prepared with table_name placeholder.\n"); + exit(1); +} + +echo "issue254_partition_table_locking_test passed\n";