php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #64993 [patch] PDO::query() memory leak and reference problem if error
Submitted: 2013-06-08 08:26 UTC Modified: 2015-08-27 20:12 UTC
Votes:5
Avg. Score:4.4 ± 0.8
Reproduced:5 of 5 (100.0%)
Same Version:3 (60.0%)
Same OS:4 (80.0%)
From: rgagnon24 at gmail dot com Assigned: nikic (profile)
Status: Closed Package: PDO MySQL
PHP Version: 5.4.16 OS: Any
Private report: No CVE-ID: None
 [2013-06-08 08:26 UTC] rgagnon24 at gmail dot com
Description:
------------
If an error happens while executing PDO::query() causing FALSE to be returned, or a PDOException to be thrown, the result is that a reference is held to the PDO object and the database connection is not released until the script is terminated.

This creates a memory leak in PHP, and an exhaustion of resources on the Database Server being used, which could become a security issue. 

This leak will not be noticible when running PHP as a web page, but is painfully a problem when creating long-term CLI scripts that are always running, connecting to a database, performing operations, closing, sleeping, and repeating.  Eventually all allowable connections to your database server will be used up creating a denial of service problem, leading to other possible security issues.

The sample code is short, but when combined with a monitoring of the network connections to your database server, you will see that all connections remain in ESTABLISHED state (via netstat -an) even though the $dbh value is set to NULL on each iteration.

Because of the error in the PDO::query() call, setting $dbh to NULL does not have the effect of releasing the connection because there is a held reference to the object--namely the PDOStatement that is created internally within PDO::query() but never returned to the calling program due to the error.

Note that persistent connection pooling is not the problem here as the test script specifically instructs the code NOT to use persistent connections in order to demonstrate the memory leak.  To ensure this further, all "persistent" settings in php.ini were set to completely disable connection persistence.

The bug is patched easily with the attached patch for the ext/pdo/pdo_dbh.c file (1 line of code is moved).

The reason for moving the line in question is because during the error, the created "stmt" object CANNOT be returned to the calling program.  Thus, the destructor of the stmt cannot go off, and the reference count to the "dbh" object will never decrease to allow its destructor to do cleanup.

Furthermore, since the stmt object created prior to line 1125 of pdo_dbh.c has a reference to the dbh added to it.  A matching UNreference must be done if the object is not going to be returned.  Without the patch, the reference is only removed following the PDO_HANDLE_DBH_ERR() macro, when it should be de-referenced after EITHER PDO_HANDLE_DBH_ERR() OR PDO_HANDLE_STMT_ERR().

Since the "stmt" object is not returned during an error condition, the zval_dtor() call is required.  This will in turn allow the "stmt" to call php_pdo_dbh_delref() thus letting the dbh object die when it is set to null.

Test script:
---------------
$options = array(
   PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
   PDO::ATTR_PERSISTENT => false,
);
for($i = 0; $i < 10; $i++) {
   $dbh = new PDO("mysql:dbname=$dbname;host=$host", $user, $password, $options);
   $sql = "SELECT 1 FROM table_that_does_not_exist LIMIT 1";
   try {
      $rs = $dbh->query($sql, PDO::FETCH_ASSOC);
      $rs->closeCursor();
      print "Table exists\n";
   } catch (Exception $e) {
      print " MSG: ".$e->getMessage()."\n";
   }
   sleep(3);
   unset($rs);
   $dbh = null;
}

while (1) {
   print "sleep\n";
   sleep(10);
}


Expected result:
----------------
MSG: SQLSTATE[42S02]: Base table or view not found: 1146 Table '<DBNAME>.table_that_does_not_exist' doesn't exist
...
<repeat above 10 times>
...
sleep
sleep
sleep
...

The above output will appear, but you must ALSO check "netstat -an|grep :3306" at the same time the script is running..

What is expected is that a SINGLE "ESTABLISHED" connection is present, along with a varying number of TIME_WAIT state connections while the loop goes about opening and closing the PDO connection on each iteration.

Actual result:
--------------
What you see in "netstat -an|grep :3306" is multiple connections in ESTABLISHED state, that remain that way until the script is terminated with CTRL-C

Patches

php-5.4.16-pdo-reference.patch (last revision 2013-06-14 05:04 UTC by rgagnon24 at gmail dot com)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2013-06-08 08:30 UTC] rgagnon24 at gmail dot com
Have patch to upload, but it won't let me...

It is a small patch, so here is the diff inline:

=======================BEGIN=========================
--- ext/pdo/pdo_dbh.c.orig	2013-06-08 06:16:44.000000000 +0000
+++ ext/pdo/pdo_dbh.c	2013-06-08 07:00:54.000000000 +0000
@@ -1148,8 +1148,8 @@ static PHP_METHOD(PDO, query)
 		PDO_HANDLE_STMT_ERR();
 	} else {
 		PDO_HANDLE_DBH_ERR();
-		zval_dtor(return_value);
 	}
+	zval_dtor(return_value);
 
 	RETURN_FALSE;
 }
===================END=========================
 [2013-06-10 11:40 UTC] johannes@php.net
-Type: Security +Type: Bug
 [2013-06-10 11:40 UTC] johannes@php.net
This is no security issues. Users who want to hold a connection open can do this without this bug, too.
 [2013-06-14 05:13 UTC] rgagnon24 at gmail dot com
About the "security" type of bug filed.  I think I missed the "bug" option by one in the selector and got that one by accident.  I did want to mention it could help become a security/DOS problem with the bug, but not record it as a security issue.  I am testing now to see if with and without the bug if the "max_links" ini settings are still obeyed--which might make it a problem at that point as the bug would allow someone to workaround an admin setting.  For now this does not appear to be the case though.

In other news.....

This patch appears to also resolve the problem in bug 64549 that I also reported a while back.  Possibly the correct free'ing of the resources here eliminated the conditions that cause the error on that bug.

I have seen a couple of other bugs that are PDO related that I am going back to test with this patch to see if they may also be resolved as well.
 [2013-11-01 06:46 UTC] yohgaki@php.net
Your patch may solve your problem, but it may cause other problem.

        /* something broke */
        dbh->query_stmt = stmt;
        dbh->query_stmt_zval = *return_value;
        PDO_HANDLE_STMT_ERR();
    } else {
        PDO_HANDLE_DBH_ERR();
        zval_dtor(return_value);
    }

    RETURN_FALSE;


Since return_value is assigned to 

        dbh->query_stmt_zval = *return_value;

it seems we cannot free return_value.
 [2013-11-01 07:03 UTC] rgagnon24 at gmail dot com
Nice catch, yogaki.

However, is there any need to assign return_value to dbh->query_stmt_zval if FALSE is going to be sent back anyhow?

By the time execution is at either side of that "else", we are in an error condition.  The positive return happens just above the "/* something broke */" comment inside the "if (ret) {"
 [2014-01-01 12:31 UTC] felipe@php.net
-Package: PDO related +Package: PDO MySQL
 [2015-08-27 20:12 UTC] nikic@php.net
-Status: Open +Status: Closed -Assigned To: +Assigned To: nikic
 
PHP Copyright © 2001-2019 The PHP Group
All rights reserved.
Last updated: Fri May 24 03:01:26 2019 UTC