Edit report at https://bugs.php.net/bug.php?id=64993&edit=1
ID: 64993
Comment by: rgagnon24 at gmail dot com
Reported by: rgagnon24 at gmail dot com
Summary: [patch] PDO::query() memory leak and reference
problem if error
Status: Open
Type: Bug
Package: PDO related
Operating System: Any
PHP Version: 5.4.16
Block user comment: N
Private report: N
New Comment:
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.
Previous Comments:
------------------------------------------------------------------------
[2013-06-14 05:09:14] rgagnon24 at gmail dot com
Related To: Bug #64549
------------------------------------------------------------------------
[2013-06-10 11:40:52] 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-08 08:30:13] 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-08 08:26:24] 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
------------------------------------------------------------------------
--
Edit this bug report at https://bugs.php.net/bug.php?id=64993&edit=1