Skip to content

phql_internal_parse_phql() memory leak fix#16854

Merged
niden merged 4 commits into
phalcon:5.0.xfrom
nope7777:phql_internal_parse_phql-memory-leak-fix
Apr 3, 2026
Merged

phql_internal_parse_phql() memory leak fix#16854
niden merged 4 commits into
phalcon:5.0.xfrom
nope7777:phql_internal_parse_phql-memory-leak-fix

Conversation

@nope7777

@nope7777 nope7777 commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

Hello!

  • Type: bug fix
  • Link to issue: -

In raising this pull request, I confirm the following:

  • I have read and understood the Contributing Guidelines
  • I have checked that another pull request for this purpose does not exist
  • I wrote some tests for this PR
  • I have updated the relevant CHANGELOG
  • I have created a PR for the documentation about this change

In long-running processes (like queue consumers) I call Phalcon\Mvc\Model\Manager::__destruct() to clear the PHQL cache. This helps to prevent rapid memory growth, but memory usage still continues to slowly increase over time.

It appears that the issue is caused by a call to Z_TRY_ADDREF_P inside phql_internal_parse_phql:

int phql_internal_parse_phql(zval **result, char *phql, unsigned int phql_length, zval **error_msg)
{
	// ...

	cache_level = phalcon_globals_ptr->orm.cache_level;
	if (cache_level >= 0) {
		phql_key = zend_inline_hash_func(phql, phql_length + 1);
		if (phalcon_globals_ptr->orm.parser_cache != NULL) {
			if ((temp_ast = zend_hash_index_find(phalcon_globals_ptr->orm.parser_cache, phql_key)) != NULL) {
				ZVAL_ZVAL(*result, temp_ast, 1, 0);
				Z_TRY_ADDREF_P(*result);
				return SUCCESS;
			}
		}
	}

In this case, ZVAL_ZVAL already increments the reference counter, and then it is incremented once again by Z_TRY_ADDREF_P. As a result, the reference count increases incorrectly and memory is not released as expected.

Memory consumption after applying the fix:

image

The issue can be reproduced locally using the following example:

<?php

use Phalcon\Di\FactoryDefault;
use Phalcon\Mvc\Model;
use Phalcon\Db\Adapter\Pdo\Sqlite;

require './vendor/autoload.php';

$di = new FactoryDefault();

$dbFile = __DIR__ . '/database.sqlite';

if (!file_exists($dbFile)) {
    touch($dbFile);
}

$di->setShared('db', function () use ($dbFile) {
    return new Sqlite([
        'dbname' => $dbFile
    ]);
});

$db = $di->get('db');

$tableExists = $db->fetchOne("
SELECT name FROM sqlite_master 
WHERE type='table' AND name='users'
");

if (!$tableExists) {
    echo "Creating table...\n";

    $db->execute("
        CREATE TABLE users (
            id INTEGER PRIMARY KEY AUTOINCREMENT,
            name TEXT,
            email TEXT
        )
    ");

    $db->execute("
        INSERT INTO users (name,email) VALUES
        ('John','john@test.com'),
        ('Anna','anna@test.com')
    ");
}

class User extends Model
{
    public $id;
    public $name;
    public $email;

    public function initialize()
    {
        $this->setSource('users');
    }
}

while (true) {
    $memory = memory_get_usage(true) / 1024 / 1024;
    $memory = round($memory, 2) . ' MB';
    var_dump($memory);

    $di->get('modelsManager')->createBuilder()
        ->addFrom(User::class)
        ->getQuery()
        ->execute();

    $di->get('modelsManager')->createBuilder()
        ->addFrom(User::class)
        ->getQuery()
        ->execute();

    $di->getShared('modelsManager')->__destruct();
    gc_collect_cycles();
}

Thanks

@niden niden requested a review from Jeckerson March 9, 2026 20:30
@nope7777

Copy link
Copy Markdown
Contributor Author

It looks like the Windows pipeline failures are caused by php/setup-php-sdk@v0.11, not by the changes in this PR. The issue appears to be fixed in v0.12 via php/setup-php-sdk#18.

@Jeckerson Jeckerson changed the base branch from master to 5.0.x April 2, 2026 13:47
@Jeckerson

Copy link
Copy Markdown
Member

@nope7777 could you please rebase with 5.0.x branch?

@nope7777 nope7777 force-pushed the phql_internal_parse_phql-memory-leak-fix branch from 5467cc0 to 100fd62 Compare April 3, 2026 15:46
@niden niden merged commit 236585f into phalcon:5.0.x Apr 3, 2026
43 checks passed
@niden

niden commented Apr 3, 2026

Copy link
Copy Markdown
Member

Thank you @nope7777 Appreciate the research and fix.

@niden niden mentioned this pull request Apr 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants