php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #53567 PDO's mishandling of peristence, leading to a segfault
Submitted: 2010-12-17 22:06 UTC Modified: 2014-12-30 10:41 UTC
Votes:5
Avg. Score:4.8 ± 0.4
Reproduced:4 of 5 (80.0%)
Same Version:2 (50.0%)
Same OS:4 (100.0%)
From: algernon at balabit dot hu Assigned:
Status: No Feedback Package: PDO PgSQL
PHP Version: Irrelevant OS: Linux
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: algernon at balabit dot hu
New email:
PHP Version: OS:

 

 [2010-12-17 22:06 UTC] algernon at balabit dot hu
Description:
------------
We observed a segmentation fault in PHP, which we tracked back to PDO, and in 
particular, the way it handles persistence. I will go into detail just a tiny 
bit later - we'll need a suitable environment to reproduce the bug first.

For the record, we used the stock PHP5 packages that come with Ubuntu Lucid, but 
a quick look at SVN trunk as of yesterday suggests that the issue is present 
even in trunk. We did not test that, though.

We were using lighttpd with PHP over FastCGI: example configs can be found all 
around, there's nothing irregular in our setup. The key here, is FastCGI 
(though, other scenarios where the zend_hash_* stuff is used might also be 
affected).

When PDO notices persistence is enabled, it will put the whole dbh structure 
into the persistent cache, near the end of the constructor. The next time the 
constructor is called, it will grab the dbh structure from the cache as-is.

This would work, except that the dbh structure contains ->query_stmt and -
>query_stmt_zval, which can potentially point to free'd memory: the last 
statement is stored in these variables, but in case we issue an invalid SQL 
query, and get an uncaught exception back, PHP will bail out, without calling 
the PDO destructor, thus, ->query_stmt and ->query_stmt_zval will point to 
garbage.

This again wouldn't be a problem, if dbh wouldn't get reused. But remember: it 
has been stored in the cache, and the next time the PDO constructor gets called, 
it will grab dbh out of the cache, with two variables pointing to garbage.

And then all hell breaks loose.

To reproduce the problem, the test script below can be used. For it to work, one 
will need any kind of database (the script uses postgres, but any driver should 
work) - the database can be empty, neither query requires any tables. The script 
will make two queries: a correct one, and a bad one. We need the bad one to 
trigger an exception.

Once the script is in place, I used the three terminals with a wget loop each 
(there might be easier ways to trigger the bug, but this ain't that bad, 
either):

On the first terminal:
$ while true ; do wget -O /dev/null 'http://localhost/?sleep=10' ; sleep 2 ; 
done

On the second and third:
$ while true ; do wget -O /dev/null 'http://localhost/?foo=bar' ; sleep 2 ; done

Within a couple of seconds, I started to get HTTP 500 errors and core files.

The fix (see the patch attached), is to zero out ->query_stmt and -
>query_stmt_zval after retrieving a DBH object from the persistent storage.

(Many thanks to my collegues Attila Magyar <athos@balabit.hu> & Bal√°zs Scheidler 
<bazsi@balabit.hu> for doing the bulk of debugging & researching the issue)

Test script:
---------------
<?php
class A
{
  public function __construct() {
    $this->pdo = new PDO('pgsql:dbname=sftest;', 'user', 's1krl+', array(PDO::ATTR_PERSISTENT => true));
    $this->pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
  }
  public function __destruct()  {
    unset($this->pdo);
  }
  public function trigger()  {
    $result = $this->pdo->query('SELECT 42 AS fortytwo;');
    sleep ($_GET['sleep']);
    $result = $this->pdo->query ('SELECT ICanHasFail();');
  }
  private $pdo;
}
$a = new A();
$a->trigger();
?>

Expected result:
----------------
The expected result would be anything but core files and segfaults.

Actual result:
--------------
Core was generated by `/usr/bin/php-cgi'.
Program terminated with signal 11, Segmentation fault.
#0  0x00000000006bb7a2 in zend_objects_store_del_ref_by_handle_ex (handle=0, 
handlers=0x2a66400) at /build/buildd/php5-5.3.2/Zend/zend_gc.h:190
190             root->prev->next = root->next;
(gdb) bt
#0  0x00000000006bb7a2 in zend_objects_store_del_ref_by_handle_ex (handle=0, 
handlers=0x2a66400) at /build/buildd/php5-5.3.2/Zend/zend_gc.h:190
#1  0x00000000006bb813 in zend_objects_store_del_ref (zobject=0x2a8d1e8) at 
/build/buildd/php5-5.3.2/Zend/zend_objects_API.c:172
#2  0x00007fe3bbc877b3 in pdo_dbh_attribute_set (dbh=0x2a8d140, attr=12, 
value=0x2a68c90) at /build/buildd/php5-5.3.2/ext/pdo/pdo_dbh.c:825
#3  0x00007fe3bbc88275 in zim_PDO_dbh_constructor (ht=4, return_value=0x2a66400, 
return_value_ptr=0xd92340, this_ptr=0x7fffae890f68, return_value_used=0) at 
/build/buildd/php5-5.3.2/ext/pdo/pdo_dbh.c:409
#4  0x00000000006e719a in zend_do_fcall_common_helper_SPEC 
(execute_data=0x2a96318) at /build/buildd/php5-5.3.2/Zend/zend_vm_execute.h:313
#5  0x00000000006be480 in execute (op_array=0x2a68f20) at /build/buildd/php5-
5.3.2/Zend/zend_vm_execute.h:104
#6  0x00000000006961ad in zend_execute_scripts (type=0, retval=0x7fffae891390, 
file_count=3) at /build/buildd/php5-5.3.2/Zend/zend.c:1266
#7  0x0000000000641df8 in php_execute_script (primary_file=0x2) at 
/build/buildd/php5-5.3.2/main/main.c:2288
#8  0x0000000000723d44 in main (argc=32767, argv=0x0) at /build/buildd/php5-
5.3.2/sapi/cgi/cgi_main.c:2110
(gdb) fr 3
#3  0x00007fe3bbc88275 in zim_PDO_dbh_constructor (ht=4, return_value=0x2a66400, 
return_value_ptr=0xd92340, this_ptr=0x7fffae890f68, return_value_used=0) at 
/build/buildd/php5-5.3.2/ext/pdo/pdo_dbh.c:409
409                                     && HASH_KEY_IS_LONG == 
zend_hash_get_current_key(Z_ARRVAL_P(options), &str_key, &long_key, 0)) {
(gdb) print dbh->query_stmt_zval
$2 = {value = {lval = 4, dval = 1.9762625833649862e-323, str = {val = 0x4 
<Address 0x4 out of bounds>, len = -1142328224}, ht = 0x4, obj = {handle = 4, 
handlers = 0x7fe3bbe97460}}, refcount__gc = 2, 
  type = 5 '\005', is_ref__gc = 1 '\001'}

Patches

pdo-persistence-fix (last revision 2010-12-17 21:08 UTC by algernon at balabit dot hu)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2010-12-18 17:26 UTC] cataphract@php.net
I'm unsure about the attached patch.

If there's the exception and PHP bails out, zeroing the zval field is of course correct. But what if the PDO error doesn't generate exceptions (no PDO::ERRMODE_EXCEPTION) and you create another object? You'll be zeroing the field from cache and leaking the object associated with the zval because there's no call to zval_dtor, right?

Leaked objects are reclaimed on request shutdown, but another solution would be preferable.
 [2010-12-18 18:19 UTC] algernon at balabit dot hu
You're right - that's indeed a scenario I didn't consider.

I don't have the source code before me at the moment, but an option would be to 
free and clear ->query_stmt and ->query_stmt_zval before putting the DBH object 
into the cache. However, I have a feeling that would have a few unwanted side 
effects too.

Thinking further... would it be possible to decouple ->query_stmt and -
>query_stmt zval from the DBH object, and store them somewhere else? If they 
never reach the cache, then there is no problem.

Actually, if we just copy DBH before putting it into the cache, with these two 
fields blanked out, that might be enough, perhaps. Though, that might lead to 
another kind of memory leak - without being able to have a quick look, I'm not 
quite sure.
 [2010-12-18 22:08 UTC] cataphract@php.net
I give up on this; the thing is badly designed. The persistent handles are shared as the data of PDO objects with same driver/user/pass and they include, through the zval, a "pointer" to non-persistent data (the storage of the statement object) that holds a reference; this non-persistent data may or may not be valid.

The way I see it, the only way to solve this would be to properly separate persistent and non-persistent data. The storage for the objects would always be non-persistent, but they could have a pointer to some persistent data, if necessary. The .query_stmt and .query_stmt_zval would always be in the non-persistent part, of course.

This, however, would require a redesign that only someone with familiarity with this extension should do.

A bit a latere, the segfault is not the only problem; these persistent storage is also shared between instances:

<?php
$db1 = new PDO('sqlite:ex.db3', '', '', array(PDO::ATTR_PERSISTENT => true));
$db2 = new PDO('sqlite:ex.db3', '', '', array(PDO::ATTR_PERSISTENT => true));

$db1->query ('SELECT ICanHasFail();');
var_dump($db2->errorInfo()); //gives error that occurred on $db1

And I imagine with transactions this could get hairy.
 [2010-12-18 22:12 UTC] cataphract@php.net
-Status: Open +Status: Verified
 [2014-01-01 12:46 UTC] felipe@php.net
-Package: PDO related +Package: PDO PgSQL
 [2014-07-09 05:14 UTC] yohgaki@php.net
-Status: Verified +Status: Feedback
 [2014-07-09 05:14 UTC] yohgaki@php.net
I've tried to reproduce this issue, but it seems working now with 5.5.14.
Do you still have this issue?
 [2014-12-30 10:41 UTC] php-bugs at lists dot php dot net
No feedback was provided. The bug is being suspended because
we assume that you are no longer experiencing the problem.
If this is not the case and you are able to provide the
information that was requested earlier, please do so and
change the status of the bug back to "Re-Opened". Thank you.
 
PHP Copyright © 2001-2021 The PHP Group
All rights reserved.
Last updated: Sat Nov 27 21:03:13 2021 UTC