Skip to content

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

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

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?

@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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct. That delete only runs when the ICU constructor sets U_FAILURE, which I could not reach from any locale input, so no .phpt goes red here. Dropped the test. 3cd491e

@LamentXU123

Copy link
Copy Markdown
Member

Otherwise this looks good.

@iliaal
iliaal force-pushed the fix/intl-gcal-adopt-df branch from b5943e9 to f157891 Compare August 17, 2026 11:31
GregorianCalendar(TimeZone*, ...) adopts the zone in the constructor.
On U_FAILURE the calendar destructor already deletes that zone, so the
extra delete tz is a double free. 8.5 and master carry the same delete,
ahead of the unique_ptr destroying the calendar rather than after it.
@iliaal
iliaal force-pushed the fix/intl-gcal-adopt-df branch from f157891 to 3cd491e Compare August 17, 2026 11:40
@iliaal
iliaal requested a review from LamentXU123 August 17, 2026 11:44
@LamentXU123

Copy link
Copy Markdown
Member

Yeah I am surprised that nobody discover this bug..

@LamentXU123

Copy link
Copy Markdown
Member

Note that in 8.5 and master, we use unique_ptr here. But the bug also exists just to have to adjust the code when merging.

LamentXU123 added a commit that referenced this pull request Aug 17, 2026
* PHP-8.5:
  Fix IntlGregorianCalendar double-free of an adopted TimeZone (#23321)
pull Bot pushed a commit to Ballalhossaintalukder/php-src that referenced this pull request Aug 17, 2026
* PHP-8.4:
  Fix IntlGregorianCalendar double-free of an adopted TimeZone (php#23321)
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