Bug #78379 [ReO->Csd]: Cast to object confuses GC, causes crash

Edit report at https://bugs.php.net/bug.php?id=78379&edit=1 ID: 78379 Updated by: nikic@php.net Reported by: tstarling@php.net Summary: Cast to object confuses GC, causes crash -Status: Re-Opened +Status: Closed Type: Bug Package: Reproducible crash Operating System: Linux PHP Version: 7.2Git-2019-08-06 (Git) Assigned To: dmitry Block user comment: N Private report: N New Comment: I believe the remaining issue here should be fixed by https://github.com/php/php-src/commit/5f8ed7765ab2a0ec372fce90701c51ce75312541. Previous Comments: ------------------------------------------------------------------------ [2019-08-13 08:01:04] nikic@php.net I've applied an additional fix at https://github.com/php/php-src/commit/18f2918a0fcf66562a5e7d964188c188660e9728 for a crash observed on 7.4. After looking at this more, the current solution is still incomplete. Looking at a GC trace for https://github.com/php/php-src/blob/6b1cc1252e73e51e53194c8c65e3d2302bc83dca/Zend/tests/bug78379_2.phpt we see that the first GC run does *not* collect the cycle, even though it should. We mark the array root grey first and miss a refcount decrement due to that (as we're not decrementing if it's a property array). I think the current code may be okay to prevent crashes for 7.2, but we probably need to implement the proper refcount based variant for 7.4 to actually make this work correctly. ------------------------------------------------------------------------ [2019-08-09 13:09:43] dmitry@php.net Should be fixed now. ------------------------------------------------------------------------ [2019-08-09 03:33:37] tstarling@php.net I guess it's good enough. The trace output is certainly better than it was. I did start work on a patch which tries to make property tables be children of objects. The problem I ran into is making sure property tables are not directly destroyed by the garbage collector, since zend_object_std_dtor() etc. doesn't check if the property array has already been destroyed before destroying it, leading to an assertion failure. Maybe it is a thing to try against master rather than in a bugfix for all branches. ------------------------------------------------------------------------ [2019-08-08 14:18:44] dmitry@php.net Proposed patches https://gist.github.com/dstogov/43a992d481f65ac16c454e1a292be38e seem to fix the second problem ------------------------------------------------------------------------ [2019-08-08 08:03:25] dmitry@php.net The committed patch https://github.com/php/php-src/commit/358379be22c4e20f4942737e0e90422977355c63 doesn't fix the second problem. ------------------------------------------------------------------------ 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=78379 -- Edit this bug report at https://bugs.php.net/bug.php?id=78379&edit=1