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] johan...@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
