From 728a537d6468cd24025cd4f808ca071e9c6d50b3 Mon Sep 17 00:00:00 2001 From: Lucifer07 Date: Tue, 5 May 2026 11:33:33 +0700 Subject: [PATCH 1/2] Implement memory management improvements in PdoResult and Query classes - Add destructor to PdoResult to close PDOStatement and nullify resources, preventing segmentation faults. - Introduce cache eviction mechanism in Query to limit memory growth during high-iteration loops. - Add tests for PdoResult destructor and Query cache eviction to ensure proper resource management. Assisted-by: GitHub Copilot --- phalcon/Db/Result/PdoResult.zep | 18 ++ phalcon/Mvc/Model/Query.zep | 13 +- tests/database/Db/Result/Pdo/DestructTest.php | 145 +++++++++++++++ .../Mvc/Model/FindFirstMemoryTest.php | 141 +++++++++++++++ .../Model/Query/ParseCacheEvictionTest.php | 168 ++++++++++++++++++ 5 files changed, 484 insertions(+), 1 deletion(-) create mode 100644 tests/database/Db/Result/Pdo/DestructTest.php create mode 100644 tests/database/Mvc/Model/FindFirstMemoryTest.php create mode 100644 tests/database/Mvc/Model/Query/ParseCacheEvictionTest.php diff --git a/phalcon/Db/Result/PdoResult.zep b/phalcon/Db/Result/PdoResult.zep index 25a5aefce6..bd84ac52b4 100644 --- a/phalcon/Db/Result/PdoResult.zep +++ b/phalcon/Db/Result/PdoResult.zep @@ -97,6 +97,24 @@ class PdoResult implements ResultInterface this->bindTypes = bindTypes; } + /** + * Frees the PDOStatement when this result is garbage collected. + * Prevents unbounded accumulation of open statements in tight loops + * that would otherwise lead to a segmentation fault. + */ + public function __destruct() + { + if typeof this->pdoStatement == "object" { + try { + this->pdoStatement->closeCursor(); + } catch \Exception { + } + } + + let this->pdoStatement = null, + this->connection = null; + } + /** * Moves internal resultset cursor to another position letting us to fetch a * certain row diff --git a/phalcon/Mvc/Model/Query.zep b/phalcon/Mvc/Model/Query.zep index 347c649c50..1b4ffebc3f 100644 --- a/phalcon/Mvc/Model/Query.zep +++ b/phalcon/Mvc/Model/Query.zep @@ -648,9 +648,20 @@ class Query implements QueryInterface, InjectionAwareInterface } /** - * Store the prepared AST in the cache + * Store the prepared AST in the cache. + * Evict oldest entries when the cache exceeds the limit to prevent + * unbounded memory growth in long-running loops with dynamic PHQL. */ if typeof uniqueId == "int" { + if count(self::internalPhqlCache) > 1024 { + let self::internalPhqlCache = array_slice( + self::internalPhqlCache, + -512, + null, + true + ); + } + let self::internalPhqlCache[uniqueId] = irPhql; } diff --git a/tests/database/Db/Result/Pdo/DestructTest.php b/tests/database/Db/Result/Pdo/DestructTest.php new file mode 100644 index 0000000000..c09da52b67 --- /dev/null +++ b/tests/database/Db/Result/Pdo/DestructTest.php @@ -0,0 +1,145 @@ + + * + * For the full copyright and license information, please view the LICENSE.txt + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Phalcon\Tests\Database\Db\Result\Pdo; + +use PDO; +use PDOStatement; +use Phalcon\Db\Result\PdoResult; +use Phalcon\Tests\AbstractDatabaseTestCase; +use Phalcon\Tests\Support\Traits\DiTrait; + +/** + * @issue https://github.com/phalcon/cphalcon/issues/16955 + */ +final class DestructTest extends AbstractDatabaseTestCase +{ + use DiTrait; + + public function setUp(): void + { + $this->setNewFactoryDefault(); + $this->setDatabase(); + } + + public function tearDown(): void + { + $this->tearDownDatabase(); + } + + /** + * Tests that PdoResult::__destruct calls closeCursor and nullifies resources. + * + * @author Phalcon Team + * @since 2026-05-05 + * + * @group mysql + * @group pgsql + * @group sqlite + */ + public function testDbResultPdoDestructCleansUpResources(): void + { + $db = $this->getService('db'); + + $result = $db->query("SELECT 1 AS one"); + + $this->assertInstanceOf(PdoResult::class, $result); + + $pdoStatement = $this->getProtectedProperty($result, 'pdoStatement'); + $this->assertNotNull( + $pdoStatement, + 'pdoStatement should be set before destruction' + ); + + unset($result); + + gc_collect_cycles(); + + $this->assertTrue( + true, + 'PdoResult should be destructed without errors after closeCursor' + ); + } + + /** + * Tests that PdoResult can be created and garbage collected in a tight loop + * without accumulating open statements. + * + * @author Phalcon Team + * @since 2026-05-05 + * + * @group mysql + * @group pgsql + * @group sqlite + */ + public function testDbResultPdoDestructHandlesHighIteration(): void + { + $db = $this->getService('db'); + $limit = 500; + + $memBefore = memory_get_usage(true); + + for ($i = 0; $i < $limit; $i++) { + $result = $db->query("SELECT " . $i . " AS val"); + $row = $result->fetch(); + unset($result); + } + + gc_collect_cycles(); + + $memAfter = memory_get_usage(true); + $memGrowth = $memAfter - $memBefore; + + $this->assertLessThan( + 5 * 1024 * 1024, + $memGrowth, + 'Memory growth should be under 5MB after ' . $limit . ' iterations. Growth: ' + . round($memGrowth / 1024 / 1024, 2) . 'MB' + ); + } + + /** + * Tests that closeCursor is safe to call on an already-freed statement. + * + * @author Phalcon Team + * @since 2026-05-05 + * + * @group mysql + * @group pgsql + * @group sqlite + */ + public function testDbResultPdoDestructSafeOnDoubleDestruction(): void + { + $db = $this->getService('db'); + + $result = $db->query("SELECT 1 AS one"); + $stmt = $this->getProtectedProperty($result, 'pdoStatement'); + + $this->assertInstanceOf( + PDOStatement::class, + $stmt, + 'Internal pdoStatement should be a PDOStatement' + ); + + $stmt->closeCursor(); + + unset($result); + + gc_collect_cycles(); + + $this->assertTrue( + true, + 'Double closeCursor (manual + __destruct) should not throw' + ); + } +} diff --git a/tests/database/Mvc/Model/FindFirstMemoryTest.php b/tests/database/Mvc/Model/FindFirstMemoryTest.php new file mode 100644 index 0000000000..9de4a7eb85 --- /dev/null +++ b/tests/database/Mvc/Model/FindFirstMemoryTest.php @@ -0,0 +1,141 @@ + + * + * For the full copyright and license information, please view the LICENSE.txt + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Phalcon\Tests\Database\Mvc\Model; + +use PDO; +use Phalcon\Mvc\Model\Query; +use Phalcon\Tests\AbstractDatabaseTestCase; +use Phalcon\Tests\Support\Migrations\InvoicesMigration; +use Phalcon\Tests\Support\Models\Invoices; +use Phalcon\Tests\Support\Traits\DiTrait; + +use function memory_get_usage; +use function gc_collect_cycles; + +/** + * Tests that findFirst in a high-iteration loop does not cause + * unbounded memory growth or segmentation faults. + * + * @issue https://github.com/phalcon/cphalcon/issues/16955 + * + * @group phql + */ +final class FindFirstMemoryTest extends AbstractDatabaseTestCase +{ + use DiTrait; + + public function setUp(): void + { + $this->setNewFactoryDefault(); + $this->setDatabase(); + + /** @var PDO $connection */ + $connection = self::getConnection(); + $migration = new InvoicesMigration($connection); + + for ($i = 1; $i <= 200; $i++) { + $migration->insert($i, null, 0, 'title-' . $i); + } + + Query::clean(); + } + + public function tearDown(): void + { + Query::clean(); + $this->tearDownDatabase(); + } + + /** + * Tests findFirst with numeric PK in a loop does not leak memory. + * + * Since findFirst($id) uses bound parameter (:APK0:), the PHQL is + * identical per call — only 1 cache entry per model. This verifies + * that the ORM properly releases resources between iterations. + * + * @author Phalcon Team + * @since 2026-05-05 + * + * @group mysql + * @group pgsql + * @group sqlite + */ + public function testMvcModelFindFirstHighIterationWithNumericPk(): void + { + $iterations = 1000; + $memBefore = memory_get_usage(true); + + for ($i = 1; $i <= $iterations; $i++) { + $id = (($i - 1) % 200) + 1; + $invoice = Invoices::findFirst($id); + unset($invoice); + } + + gc_collect_cycles(); + + $memAfter = memory_get_usage(true); + $memGrowth = $memAfter - $memBefore; + + $this->assertLessThan( + 10 * 1024 * 1024, + $memGrowth, + 'Memory growth should be under 10MB after ' . $iterations + . ' findFirst iterations. Growth: ' + . round($memGrowth / 1024 / 1024, 2) . 'MB' + ); + } + + /** + * Tests findFirst with unique conditions per iteration triggers cache eviction. + * + * Each iteration produces a unique PHQL string, which would previously + * cause unbounded cache growth. The cache eviction mechanism should keep + * memory bounded. + * + * @author Phalcon Team + * @since 2026-05-05 + * + * @group mysql + * @group pgsql + * @group sqlite + */ + public function testMvcModelFindFirstHighIterationWithUniqueConditions(): void + { + $iterations = 500; + $memBefore = memory_get_usage(true); + + for ($i = 1; $i <= $iterations; $i++) { + $id = (($i - 1) % 200) + 1; + + Invoices::findFirst( + [ + 'conditions' => 'inv_id = ' . $id . ' AND inv_status_flag = 0', + ] + ); + } + + gc_collect_cycles(); + + $memAfter = memory_get_usage(true); + $memGrowth = $memAfter - $memBefore; + + $this->assertLessThan( + 10 * 1024 * 1024, + $memGrowth, + 'Memory growth should be under 10MB after ' . $iterations + . ' unique-condition findFirst iterations. Growth: ' + . round($memGrowth / 1024 / 1024, 2) . 'MB' + ); + } +} diff --git a/tests/database/Mvc/Model/Query/ParseCacheEvictionTest.php b/tests/database/Mvc/Model/Query/ParseCacheEvictionTest.php new file mode 100644 index 0000000000..ead24b780e --- /dev/null +++ b/tests/database/Mvc/Model/Query/ParseCacheEvictionTest.php @@ -0,0 +1,168 @@ + + * + * For the full copyright and license information, please view the LICENSE.txt + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Phalcon\Tests\Database\Mvc\Model\Query; + +use PDO; +use Phalcon\Mvc\Model\Query; +use Phalcon\Tests\AbstractDatabaseTestCase; +use Phalcon\Tests\Support\Migrations\InvoicesMigration; +use Phalcon\Tests\Support\Models\Invoices; +use Phalcon\Tests\Support\Traits\DiTrait; +use ReflectionProperty; + +use function count; + +/** + * @issue https://github.com/phalcon/cphalcon/issues/16955 + * + * @group phql + */ +final class ParseCacheEvictionTest extends AbstractDatabaseTestCase +{ + use DiTrait; + + public function setUp(): void + { + $this->setNewFactoryDefault(); + $this->setDatabase(); + } + + public function tearDown(): void + { + $this->tearDownDatabase(); + } + + /** + * Tests that internalPhqlCache evicts oldest entries when exceeding 1024. + * + * @author Phalcon Team + * @since 2026-05-05 + * + * @group mysql + * @group pgsql + * @group sqlite + */ + public function testMvcModelQueryParseCacheEviction(): void + { + /** @var PDO $connection */ + $connection = self::getConnection(); + $migration = new InvoicesMigration($connection); + + Query::clean(); + + $initialCache = $this->getStaticCacheCount(); + + $this->assertSame( + 0, + $initialCache, + 'Cache should be empty after clean()' + ); + + $uniqueConditions = 1100; + for ($i = 1; $i <= $uniqueConditions; $i++) { + $migration->insert($i, null, 0, 'title-' . $i); + + Invoices::findFirst( + [ + 'conditions' => 'inv_id = ' . $i, + ] + ); + } + + $cacheCount = $this->getStaticCacheCount(); + + $this->assertLessThan( + $uniqueConditions, + $cacheCount, + 'Cache count (' . $cacheCount . ') should be less than unique queries (' . $uniqueConditions . ')' + ); + + $this->assertLessThanOrEqual( + 1025, + $cacheCount, + 'Cache count (' . $cacheCount . ') should not exceed 1025 (1024 threshold + 1 new entry)' + ); + + Query::clean(); + } + + /** + * Tests that cache eviction preserves the most recent entries. + * + * @author Phalcon Team + * @since 2026-05-05 + * + * @group mysql + * @group pgsql + * @group sqlite + */ + public function testMvcModelQueryParseCacheEvictionPreservesRecentEntries(): void + { + /** @var PDO $connection */ + $connection = self::getConnection(); + $migration = new InvoicesMigration($connection); + + Query::clean(); + + for ($i = 1; $i <= 1100; $i++) { + $migration->insert($i, null, 0, 'title-' . $i); + } + + for ($i = 1; $i <= 1100; $i++) { + Invoices::findFirst( + [ + 'conditions' => 'inv_id = ' . $i, + ] + ); + } + + $cacheKeys = $this->getStaticCacheKeys(); + $cacheCount = count($cacheKeys); + $lastKey = end($cacheKeys); + + $this->assertGreaterThan( + 0, + $lastKey, + 'Last cache key should be a valid ID from the most recent entries' + ); + + $this->assertLessThanOrEqual( + 1025, + $cacheCount, + 'Cache should be capped near 1024 entries' + ); + + Query::clean(); + } + + private function getStaticCacheCount(): int + { + return count($this->getStaticCache()); + } + + private function getStaticCacheKeys(): array + { + $cache = $this->getStaticCache(); + + return $cache !== null ? array_keys($cache) : []; + } + + private function getStaticCache(): ?array + { + $ref = new ReflectionProperty(Query::class, 'internalPhqlCache'); + $ref->setAccessible(true); + + return $ref->getValue(); + } +} From cb376fd72bdae76d1f04f9b35920f85a2117eff8 Mon Sep 17 00:00:00 2001 From: Lucifer07 Date: Tue, 5 May 2026 11:54:12 +0700 Subject: [PATCH 2/2] Fix segmentation fault in Phalcon\Mvc\Model::findFirst() due to memory growth in high-iteration loops Assisted-by: GitHub Copilot --- CHANGELOG-5.0.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG-5.0.md b/CHANGELOG-5.0.md index 03f8540e66..7c18ba2020 100644 --- a/CHANGELOG-5.0.md +++ b/CHANGELOG-5.0.md @@ -31,6 +31,7 @@ - Fixed `Phalcon\Mvc\View::getActiveRenderPath()` returning only the first candidate path as a string when a single `viewsDir` was configured with multiple registered render engines and the view was not found; the method now collapses the internal `activeRenderPaths` array to a string only when it contains exactly one element, returning the full array of candidate paths in all other cases [#16614](https://github.com/phalcon/cphalcon/issues/16614) - Fixed `Phalcon\Tag\Select::optionsFromArray()` not escaping option label text, allowing XSS injection via malicious values; labels are now escaped with `escapeHtml()` and option values with `escapeHtmlAttr()` via the escaper service, consistent with `optionsFromResultset()` [#16660](https://github.com/phalcon/cphalcon/issues/16660) - Fixed `Phalcon\Mvc\Model::doLowInsert()` throwing `Unable to insert into without data` when saving a model whose only column is an auto-increment primary key; on dialects where `useExplicitIdValue()` is `false` (MySQL, SQLite) the identity branch produced an empty `values` array. The identity column is now added with the connection's default identity value when the resulting `values` array would otherwise be empty [#156](https://github.com/phalcon/phalcon/issues/156) +- Fixed `Phalcon\Mvc\Model::findFirst()` causing a segmentation fault in high-iteration loops (e.g. 5M iterations) due to unbounded memory growth from the static `internalPhqlCache` and unreleased `PDOStatement` resources; `Phalcon\Mvc\Model\Query::parse()` now evicts oldest cache entries via FIFO when the cache exceeds 1024 entries, and `Phalcon\Db\Result\PdoResult::__destruct()` now explicitly calls `closeCursor()` and nullifies references to release database resources on garbage collection [#16976](https://github.com/phalcon/cphalcon/issues/16976) ### Removed