Skip to content

Fix IntlGregorianCalendar double-free of an adopted TimeZone - #23321

Open
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:fix/intl-gcal-adopt-df
Open

Fix IntlGregorianCalendar double-free of an adopted TimeZone#23321
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:fix/intl-gcal-adopt-df

Conversation

@iliaal

@iliaal iliaal commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

The timezone-and-locale IntlGregorianCalendar constructor passes the TimeZone to an adopting ICU constructor. On failure PHP deleted that zone again after delete gcal already ran the calendar destructor.

@LamentXU123 LamentXU123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this target 8.4?

GregorianCalendar(TimeZone*, ...) adopts the zone in the constructor.
On U_FAILURE the calendar destructor already deletes that zone. Remove
the extra delete tz. The 8.5/master unique_ptr path has the same extra
delete after the smart pointer already destroyed the calendar.
@iliaal
iliaal changed the base branch from master to PHP-8.4 August 16, 2026 16:36
@iliaal
iliaal force-pushed the fix/intl-gcal-adopt-df branch from 5ca1d40 to b5943e9 Compare August 16, 2026 16:36
@iliaal

iliaal commented Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Yeah, updated to 8.4

echo $cal->getType(), "\n";

$cal2 = IntlGregorianCalendar::createInstance('UTC', 'en_US');
echo $cal2->getTimeZone()->getID(), "\n";

@LamentXU123 LamentXU123 Aug 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO The added test does not exercise the fix.

You see, both constructions succeed, while the removed deletion runs only under U_FAILURE(status). The test produces identical expected output on an affected PHP 8.6.0beta1 build.

See here

@LamentXU123

Copy link
Copy Markdown
Member

Otherwise this looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants