Bug #64993 [Com]: [patch] PDO::query() memory leak and reference problem if error

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: 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) {" Previous Comments: ------------------------------------------------------------------------ [2013-11-01 06:46:21] 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-06-14 05:13:59] 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-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========================= ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=64993 -- Edit this bug report at https://bugs.php.net/bug.php?id=64993&edit=1