php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login

go to bug id or search bugs for

Bug #76590 Using persistent strings as HashTable keys causes heap corruption
Submitted: 2018-07-06 15:38 UTC Modified: 2021-06-09 11:27 UTC
Votes:1
Avg. Score:3.0 ± 0.0
Reproduced:0 of 0 (0.0%)
From: dktapps at pmmp dot io Assigned: cmb (profile)
Status: Not a bug Package: Reproducible crash
PHP Version: 7.3.0alpha3 OS: *
Private report: No CVE-ID: None
View Developer Edit
 [2018-07-06 15:38 UTC] dktapps at pmmp dot io 
Description:
------------
While attempting to update ext/pthreads (Using the master branch of https://github.com/krakjoe/pthreads) for PHP 7.3.0 alphas, I encountered a range of heap corruption issues.

After extensive debugging, I discovered these issues were not caused by pthreads itself. These issues were caused by this commit: https://github.com/php/php-src/commit/5eb1f92f31cafc48384f9096012f421b37f6d425

To be specific, using persistent strings as hashtable keys now causes issues with heap corruption in "zend_array_destroy()" because it now assumes that keys are always non-persistent. pthreads then suffered from issues due to using persistent strings as htable keys for object property tables.

I have not further investigated the range of things that this affected.

Test script:
---------------
<?php

$worker = new Worker();
$worker->start();
$worker->stack(new Threaded);
$worker->shutdown();

?>

Expected result:
----------------
no output

Actual result:
--------------
zend_mm_heap corrupted

or if in debug mode:

Assertion failed: !(zval_gc_flags((s)->gc.u.type_info) & (1<<7)), file c:\php-sdk\php\vc15\x64\php-src\zend\zend_string.h, line 290

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2018-07-06 15:51 UTC] nikic@php.net
-Status: Open +Status: Feedback
 [2018-07-06 15:51 UTC] nikic@php.net 
Placing any kind of persistent entity withing a non-persistent entity (including persistent keys into a non-persistent array) is generally a violation of PHP's memory model. Doing so usually manifests in the form of subtle thread-safety issues caused by non-atomic reference counting. This is also why use of immutablized (persistent) entities is generally legal, as they are not subject to refcounting.

Please explain in more detail (and point to the relevant code), where you are using persistent keys in non-persistent hashtables, and why you believe this to be safe. Most likely you either need to use non-persistent strings or ensure that the strings are interned.
 [2018-07-06 16:49 UTC] nikic@php.net 
By the way, it may be helpful to run pthreads in ZEND_RC_DEBUG mode, which detects refcount changes on persistent entities after the start of the request cycle. Not sure if this is compatible with the particular magic of pthreads though.
 [2018-07-06 17:56 UTC] dktapps at pmmp dot io
-Status: Feedback +Status: Open
 [2018-07-06 17:56 UTC] dktapps at pmmp dot io 
In several cases persistent strings were used for globals (which it's reasonable to make interned instead), and there was one erroneous case of persistent string used where it shouldn't have been.

I'm still trying to get an idea of the full scope of this as I work through the issues.

There is one thing that I think may be a bug, which is the behaviour of zend_hash_str_update_ptr(). If the array is persistent, the key zend_string created will also be persistent. I hit issues with this because pthreads inserts copied functions into EG(function_table), which is assumed to have non-persistent keys. This is easily worked around though.
 [2021-06-09 10:49 UTC] cmb@php.net
-Status: Open +Status: Not a bug -Assigned To: +Assigned To: cmb
 [2021-06-09 10:49 UTC] cmb@php.net 
This doesn't look like an issue with php-src, and pthreads is no
longer maintained[1].

[1] <https://github.com/krakjoe/pthreads/issues/929>
 [2021-06-09 11:27 UTC] dktapps at pmmp dot io 
Hi, I forgot about this bug.

For posterity, this was in fact a pthreads bug, in the end it was quite easily fixed by just making a non-persistent copy of the strings. Since they were anyway being copied, it didn't make any difference to the functionality of pthreads. It also helped to locate a few bugs.

https://github.com/pmmp/pthreads/commit/f55ffa9250f3856c5b542f074b45cd56c5d2db82#diff-4f7c2c6c14c2fe93d57d6eaef5ff94c55baa844c2c3efafddb77a46de3a340aaR380
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved. 		Last updated: Sat Dec 21 15:01:29 2024 UTC