Skip to content

Commit d6eb687

Browse files
iliaaliliaal
committed
Keep compiled RuleBasedBreakIterator rules alive for the iterator
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.
1 parent d09e5e2 commit d6eb687

5 files changed

Lines changed: 82 additions & 5 deletions

File tree

‎NEWS‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ PHP NEWS
1010
. Fixed a use-after-free when cloning a DOMNameSpaceNode after
1111
DOMDocument::xinclude(). (iliaal)
1212

13+
- Intl:
14+
. Fixed a use-after-free when IntlRuleBasedBreakIterator is constructed
15+
from compiled rules. (iliaal)
16+
1317
- Opcache:
1418
. Fixed opcache.protect_memory race under ZTS. (realFlowControl)
1519

‎ext/intl/breakiterator/breakiterator_class.cpp‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,9 @@ static zend_object *BreakIterator_clone_obj(zend_object *object)
109109
} else {
110110
bio_new->biter = new_biter;
111111
ZVAL_COPY(&bio_new->text, &bio_orig->text);
112+
if (bio_orig->compiled_rules) {
113+
bio_new->compiled_rules = zend_string_copy(bio_orig->compiled_rules);
114+
}
112115
}
113116
} else {
114117
zend_throw_error(NULL, "Cannot clone uninitialized BreakIterator");
@@ -163,6 +166,7 @@ static void breakiterator_object_init(BreakIterator_object *bio)
163166
{
164167
intl_error_init(BREAKITER_ERROR_P(bio));
165168
bio->biter = NULL;
169+
bio->compiled_rules = NULL;
166170
ZVAL_UNDEF(&bio->text);
167171
}
168172
/* }}} */
@@ -177,6 +181,10 @@ static void BreakIterator_objects_free(zend_object *object)
177181
delete bio->biter;
178182
bio->biter = NULL;
179183
}
184+
if (bio->compiled_rules) {
185+
zend_string_release(bio->compiled_rules);
186+
bio->compiled_rules = NULL;
187+
}
180188
intl_error_reset(BREAKITER_ERROR_P(bio));
181189

182190
zend_object_std_dtor(&bio->zo);

‎ext/intl/breakiterator/breakiterator_class.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ typedef struct {
3838
// current text
3939
zval text;
4040

41+
zend_string *compiled_rules;
42+
4143
zend_object zo;
4244
} BreakIterator_object;
4345

‎ext/intl/breakiterator/rulebasedbreakiterator_methods.cpp‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,15 +34,14 @@ static inline RuleBasedBreakIterator *fetch_rbbi(BreakIterator_object *bio) {
3434

3535
static void _php_intlrbbi_constructor_body(INTERNAL_FUNCTION_PARAMETERS, zend_error_handling *error_handling, bool *error_handling_replaced)
3636
{
37-
char *rules;
38-
size_t rules_len;
37+
zend_string *rules;
3938
bool compiled = false;
4039
UErrorCode status = U_ZERO_ERROR;
4140
BREAKITER_METHOD_INIT_VARS;
4241
object = ZEND_THIS;
4342

4443
ZEND_PARSE_PARAMETERS_START(1, 2)
45-
Z_PARAM_STRING(rules, rules_len)
44+
Z_PARAM_STR(rules)
4645
Z_PARAM_OPTIONAL
4746
Z_PARAM_BOOL(compiled)
4847
ZEND_PARSE_PARAMETERS_END();
@@ -62,7 +61,7 @@ static void _php_intlrbbi_constructor_body(INTERNAL_FUNCTION_PARAMETERS, zend_er
6261
if (!compiled) {
6362
UnicodeString rulesStr;
6463
UParseError parseError = UParseError();
65-
if (intl_stringFromChar(rulesStr, rules, rules_len, &status)
64+
if (intl_stringFromChar(rulesStr, ZSTR_VAL(rules), ZSTR_LEN(rules), &status)
6665
== FAILURE) {
6766
zend_throw_exception(IntlException_ce_ptr,
6867
"IntlRuleBasedBreakIterator::__construct(): "
@@ -84,7 +83,7 @@ static void _php_intlrbbi_constructor_body(INTERNAL_FUNCTION_PARAMETERS, zend_er
8483
RETURN_THROWS();
8584
}
8685
} else { // compiled
87-
rbbi = new RuleBasedBreakIterator((uint8_t*)rules, rules_len, status);
86+
rbbi = new RuleBasedBreakIterator(reinterpret_cast<uint8_t *>(ZSTR_VAL(rules)), ZSTR_LEN(rules), status);
8887
if (U_FAILURE(status)) {
8988
zend_throw_exception(IntlException_ce_ptr,
9089
"IntlRuleBasedBreakIterator::__construct(): "
@@ -95,6 +94,9 @@ static void _php_intlrbbi_constructor_body(INTERNAL_FUNCTION_PARAMETERS, zend_er
9594
}
9695

9796
breakiterator_object_create(return_value, rbbi, 0);
97+
if (compiled) {
98+
Z_INTL_BREAKITERATOR_P(return_value)->compiled_rules = zend_string_copy(rules);
99+
}
98100
}
99101

100102
U_CFUNC PHP_METHOD(IntlRuleBasedBreakIterator, __construct)

‎ext/intl/tests/rbbiter_compiled_rules_lifetime.phpt‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
--TEST--
2+
IntlRuleBasedBreakIterator compiled rules outlive the source string
3+
--EXTENSIONS--
4+
intl
5+
--SKIPIF--
6+
<?php if (version_compare(INTL_ICU_VERSION, '68.1') < 0) die('skip for ICU >= 68.1'); ?>
7+
--FILE--
8+
<?php
9+
10+
$rules = <<<RULES
11+
\$LN = [[:letter:] [:number:]];
12+
\$S = [.;,:];
13+
14+
!!forward;
15+
\$LN+ {1};
16+
\$S+ {42};
17+
!!reverse;
18+
\$LN+ {1};
19+
\$S+ {42};
20+
!!safe_forward;
21+
!!safe_reverse;
22+
RULES;
23+
24+
$src = new IntlRuleBasedBreakIterator($rules);
25+
$len = strlen($src->getBinaryRules());
26+
27+
$it = new IntlRuleBasedBreakIterator($src->getBinaryRules(), true);
28+
unset($src);
29+
30+
/* ICU aliases the buffer it was built from, so the freed rules have to be
31+
reclaimed and overwritten for the iterator below to read stale bytes. */
32+
$ballast = [];
33+
for ($i = 0; $i < 16; $i++) {
34+
$ballast[] = str_repeat("\xCC", $len);
35+
}
36+
37+
$it->setText('ab,cd');
38+
echo $it->first(), "\n";
39+
while (true) {
40+
$n = $it->next();
41+
if ($n === IntlBreakIterator::DONE) {
42+
break;
43+
}
44+
echo $n, "\n";
45+
}
46+
47+
$clone = clone $it;
48+
unset($it);
49+
$ballast[] = str_repeat("\xDD", $len);
50+
$clone->setText('xy');
51+
echo $clone->first(), "\n";
52+
echo $clone->next(), "\n";
53+
54+
?>
55+
--EXPECT--
56+
0
57+
2
58+
3
59+
5
60+
0
61+
2

0 commit comments

Comments
 (0)