Fix leak on double DatePeriod::__construct() call#22643
Conversation
TimWolla
left a comment
There was a problem hiding this comment.
Just looked at the diff, didn't check for completeness. I trust you there.
| if (period_obj->interval) { | ||
| timelib_rel_time_dtor(period_obj->interval); | ||
| } | ||
| memset(period_obj, 0, XtOffsetOf(php_period_obj, std)); |
There was a problem hiding this comment.
| memset(period_obj, 0, XtOffsetOf(php_period_obj, std)); | |
| memset(period_obj, 0, offsetof(php_period_obj, std)); |
We only removed XtOffsetOf with 8.6, but offsetof() is reliably available even for 8.4.
There was a problem hiding this comment.
I remembered your change. Will have to be backported to 8.2, so I'd prefer just changing on master to match your other PR.
|
@kylekatarnls I hope it's ok to ping you directly. Just wanted to check with you whether the double constructor call is even intentional. This issue popped up recently in our CI, so I figured it might not be. |
|
Hello, indeed I got this change submitted to us recently: CarbonPHP/carbon@9227630 And merged it, as you can read from the description, the goal is to update the native properties when the period get modified and __construct can actually do it in PHP 8.5. We're considering CarbonPeriod mutable and have CarbonPeriodImmutable as an immutable version, DatePeriod becoming immutable (since 8.0 IIRC) made CarbonPeriod properties no longer compatible with the parent. So I guess since the construct is skipped, the effect of the change above will be lost. At some point, we might have to drop mutable objects all at once at a certain PHP version. |
|
@kylekatarnls Thank you for the explanation.
Clarification: This PR fixes the leak, but does not change behavior (in the regular case at least, when the constructor call succeeds). So your fix should keep working. |
See https://github.com/php/php-src/actions/runs/28911263674/job/85769105449.
Normally we might want to discuss whether such code is even sensible, but it seems carbon is relying on it, as evidenced by the CI failure above. A minimal carbon example that reproduces the error:
Just remembered, will have to rebase onto 8.2 to keep nightly green.