Skip to content

Keep compiled RuleBasedBreakIterator rules alive for the iterator - #23322

Open
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:fix/intl-rbbi-compiled-rules-lifetime
Open

Keep compiled RuleBasedBreakIterator rules alive for the iterator#23322
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:fix/intl-rbbi-compiled-rules-lifetime

Conversation

@iliaal

@iliaal iliaal commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

ICU's compiled-rules constructor aliases the caller buffer. PHP passed a temporary string and did not keep it, so setText/next after that string is released can use freed memory. The object now holds a zend_string copy, released in free_obj and addref'd on clone.

@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.

Again, should this target 8.4?
(Not looking at the code yet, just asking branch-wise)

The ICU compiled-rules constructor aliases the caller's buffer.
PHP passed the argument string and did not retain it, so a later
setText/next can use freed memory. Hold a zend_string copy on the
object and release it in free_obj; clone addrefs it.
@iliaal
iliaal changed the base branch from master to PHP-8.4 August 16, 2026 16:48
@iliaal
iliaal force-pushed the fix/intl-rbbi-compiled-rules-lifetime branch from 6e871a7 to 7678a87 Compare August 16, 2026 16:48
@iliaal

iliaal commented Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Yeah, 8.4 is needed here too

!!safe_reverse;
RULES;

$src = new IntlRuleBasedBreakIterator($rules);

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.

I think the fix is good enough, the test needs to truly trigger the case we re interested in. You need to force rules reclaims here.

@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.

Except David's review.

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.

3 participants