New Garbage Collector feature for WeakMap added in 8.3 #10932 causes GC buffer overflow which then causes segfault

[JacobBrownAustin]

New Garbage Collector feature for WeakMap added in 8.3 #10932 causes GC buffer overflow which then causes segfault

[arnaud-lb]

Thank you! Depending review time, this could be fixed in the next 8.3

[arnaud-lb]

I agree, or alternatively we should delay the error until the GC ends. In general I think we should delay errors handlers until a safe point is reached, like we do for timeouts and signals, but that's an other story.

I see that you were able to make a test for it. Was it able to reproduce the issue?

Yes it was

[arnaud-lb]

We are in RC for 8.3.4, but this should be included in 8.3.5

[arnaud-lb]

Thank you for this bug report!

Based on your GDB session we can figure that we are growing the buffer unnecessarily, as there are only 20K roots in it when gc_grow_root_buffer() triggers the GC buffer overflow (GC disabled) warning. I believe that #13572 fixes the root cause of this issue. Could you confirm that it fixes the issue for you?

The second issue is a side effect of triggering the GC buffer overflow (GC disabled) warning: We execute PHP code during GC because of the error handler, which corrupts the GC state. This is a different issue that should be fixed separately.

[JacobBrownAustin]

Wow! Thanks so much for the quick response and fix! I will see if I can make time to test it today (probably tonight) as getting our application working in PHP 8.3 is a high priority for us.

I see that you were able to make a test for it. Was it able to reproduce the issue?

I was suspecting that there were two issues because the segfault was really confusing to me, but I was able to reproduce it consistently with the custom error handle. To me, it didn't look safe at all to run custom error handler in the middle of a garbage collector function.

[JacobBrownAustin]

To me, it didn't look safe at all to run custom error handler in the middle of a garbage collector function.

When I was thinking about it yesterday, I would have just disabled the ability to call custom error handling at all when entering garbage collector, then reenable it once garbage collector is finished. Maybe you have a better idea that would work.

[JacobBrownAustin]

I just did a quick test during lunch and that fix does pass our integration tests.
I still need to undo all of the changes I made to my Docker image that adds debug symbols and such, so I'm going to try rebuilding it again later tonight so that the PHP build is just like the official Docker image, but with that one fix.

[JacobBrownAustin]

I've done some more testing with not-debug builds, and the test passes just fine with your fix, #13572 .
Thank you! Do you know what version of PHP we could expect this fix?