Skip to content

Commit 8de7ccb

Browse files
ndosschendossche
committed
Fix memory leaks in ext/tidy basedir restriction code
TIDY_APPLY_CONFIG can early return because it's a macro, but then the cleanup paths are not executed. Transform this to a real function and handle the cleanups correctly at the callsites. Closes phpGH-15046.
1 parent 5996227 commit 8de7ccb

File tree

4 files changed

+81
-29
lines changed

4 files changed

+81
-29
lines changed

‎NEWS‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@ PHP NEWS
1717
- Streams:
1818
. Fixed bug GH-15028 (Memory leak in ext/phar/stream.c). (nielsdos)
1919

20+
- Tidy:
21+
. Fix memory leaks in ext/tidy basedir restriction code. (nielsdos)
22+
2023
01 Aug 2024, PHP 8.2.22
2124

2225
- Core:

‎ext/tidy/tests/open_basedir/test.html‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
<p>my epic string</p>

‎ext/tidy/tests/open_basedir_failure_config.phpt‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
--TEST--
2+
Tidy with basedir restriction failure on configuration file
3+
--EXTENSIONS--
4+
tidy
5+
--INI--
6+
open_basedir={PWD}/open_basedir
7+
--FILE--
8+
<?php
9+
echo "=== repairString ===\n";
10+
$tidy = new tidy;
11+
$tidy->repairString('my epic string', 'my_config_file.ini');
12+
13+
echo "=== tidy_parse_string ===\n";
14+
tidy_parse_string('my epic string', 'my_config_file.ini');
15+
16+
echo "=== tidy_parse_file ===\n";
17+
tidy_parse_file(__DIR__.'/open_basedir/test.html', 'my_config_file.ini');
18+
19+
echo "=== __construct ===\n";
20+
$tidy = new tidy(__DIR__.'/open_basedir/test.html', 'my_config_file.ini');
21+
22+
echo "=== parseFile ===\n";
23+
$tidy = new tidy;
24+
$tidy->parseFile(__DIR__.'/open_basedir/test.html', 'my_config_file.ini');
25+
26+
echo "=== parseString ===\n";
27+
$tidy = new tidy;
28+
$tidy->parseString('my epic string', 'my_config_file.ini');
29+
?>
30+
--EXPECTF--
31+
=== repairString ===
32+
33+
Warning: tidy::repairString(): open_basedir restriction in effect. File(my_config_file.ini) is not within the allowed path(s): (%sopen_basedir) in %s on line %d
34+
=== tidy_parse_string ===
35+
36+
Warning: tidy_parse_string(): open_basedir restriction in effect. File(my_config_file.ini) is not within the allowed path(s): (%sopen_basedir) in %s on line %d
37+
=== tidy_parse_file ===
38+
39+
Warning: tidy_parse_file(): open_basedir restriction in effect. File(my_config_file.ini) is not within the allowed path(s): (%sopen_basedir) in %s on line %d
40+
=== __construct ===
41+
42+
Warning: tidy::__construct(): open_basedir restriction in effect. File(my_config_file.ini) is not within the allowed path(s): (%sopen_basedir) in %s on line %d
43+
=== parseFile ===
44+
45+
Warning: tidy::parseFile(): open_basedir restriction in effect. File(my_config_file.ini) is not within the allowed path(s): (%sopen_basedir) in %s on line %d
46+
=== parseString ===
47+
48+
Warning: tidy::parseString(): open_basedir restriction in effect. File(my_config_file.ini) is not within the allowed path(s): (%sopen_basedir) in %s on line %d

‎ext/tidy/tidy.c‎

Lines changed: 29 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -74,19 +74,6 @@
7474
} \
7575
obj = Z_TIDY_P(object); \
7676

77-
#define TIDY_APPLY_CONFIG(_doc, _val_str, _val_ht) \
78-
if (_val_ht) { \
79-
_php_tidy_apply_config_array(_doc, _val_ht); \
80-
} else if (_val_str) { \
81-
TIDY_OPEN_BASE_DIR_CHECK(ZSTR_VAL(_val_str)); \
82-
php_tidy_load_config(_doc, ZSTR_VAL(_val_str)); \
83-
}
84-
85-
#define TIDY_OPEN_BASE_DIR_CHECK(filename) \
86-
if (php_check_open_basedir(filename)) { \
87-
RETURN_FALSE; \
88-
} \
89-
9077
#define TIDY_SET_DEFAULT_CONFIG(_doc) \
9178
if (TG(default_config) && TG(default_config)[0]) { \
9279
php_tidy_load_config(_doc, TG(default_config)); \
@@ -221,6 +208,19 @@ static void php_tidy_load_config(TidyDoc doc, const char *path)
221208
}
222209
}
223210

211+
static zend_result php_tidy_apply_config(TidyDoc doc, zend_string *str_string, HashTable *ht_options)
212+
{
213+
if (ht_options) {
214+
return _php_tidy_apply_config_array(doc, ht_options);
215+
} else if (str_string) {
216+
if (php_check_open_basedir(ZSTR_VAL(str_string))) {
217+
return FAILURE;
218+
}
219+
php_tidy_load_config(doc, ZSTR_VAL(str_string));
220+
}
221+
return SUCCESS;
222+
}
223+
224224
static int _php_tidy_set_tidy_opt(TidyDoc doc, char *optname, zval *value)
225225
{
226226
TidyOption opt = tidyGetOptionByName(doc, optname);
@@ -329,9 +329,9 @@ static void php_tidy_quick_repair(INTERNAL_FUNCTION_PARAMETERS, bool is_file)
329329

330330
TIDY_SET_DEFAULT_CONFIG(doc);
331331

332-
TIDY_APPLY_CONFIG(doc, config_str, config_ht);
333-
334-
if(enc_len) {
332+
if (php_tidy_apply_config(doc, config_str, config_ht) != SUCCESS) {
333+
RETVAL_FALSE;
334+
} else if (enc_len) {
335335
if (tidySetCharEncoding(doc, enc) < 0) {
336336
php_error_docref(NULL, E_WARNING, "Could not set encoding \"%s\"", enc);
337337
RETVAL_FALSE;
@@ -1024,9 +1024,8 @@ PHP_FUNCTION(tidy_parse_string)
10241024
tidy_instantiate(tidy_ce_doc, return_value);
10251025
obj = Z_TIDY_P(return_value);
10261026

1027-
TIDY_APPLY_CONFIG(obj->ptdoc->doc, options_str, options_ht);
1028-
1029-
if (php_tidy_parse_string(obj, ZSTR_VAL(input), (uint32_t)ZSTR_LEN(input), enc) == FAILURE) {
1027+
if (php_tidy_apply_config(obj->ptdoc->doc, options_str, options_ht) != SUCCESS
1028+
|| php_tidy_parse_string(obj, ZSTR_VAL(input), (uint32_t)ZSTR_LEN(input), enc) == FAILURE) {
10301029
zval_ptr_dtor(return_value);
10311030
RETURN_FALSE;
10321031
}
@@ -1093,9 +1092,8 @@ PHP_FUNCTION(tidy_parse_file)
10931092
tidy_instantiate(tidy_ce_doc, return_value);
10941093
obj = Z_TIDY_P(return_value);
10951094

1096-
TIDY_APPLY_CONFIG(obj->ptdoc->doc, options_str, options_ht);
1097-
1098-
if (php_tidy_parse_string(obj, ZSTR_VAL(contents), (uint32_t)ZSTR_LEN(contents), enc) == FAILURE) {
1095+
if (php_tidy_apply_config(obj->ptdoc->doc, options_str, options_ht) != SUCCESS
1096+
|| php_tidy_parse_string(obj, ZSTR_VAL(contents), (uint32_t)ZSTR_LEN(contents), enc) == FAILURE) {
10991097
zval_ptr_dtor(return_value);
11001098
RETVAL_FALSE;
11011099
}
@@ -1388,7 +1386,11 @@ PHP_METHOD(tidy, __construct)
13881386
RETURN_THROWS();
13891387
}
13901388

1391-
TIDY_APPLY_CONFIG(obj->ptdoc->doc, options_str, options_ht);
1389+
if (php_tidy_apply_config(obj->ptdoc->doc, options_str, options_ht) != SUCCESS) {
1390+
/* TODO: this is the constructor, we should throw probably... */
1391+
zend_string_release_ex(contents, 0);
1392+
RETURN_FALSE;
1393+
}
13921394

13931395
php_tidy_parse_string(obj, ZSTR_VAL(contents), (uint32_t)ZSTR_LEN(contents), enc);
13941396

@@ -1427,9 +1429,8 @@ PHP_METHOD(tidy, parseFile)
14271429
RETURN_THROWS();
14281430
}
14291431

1430-
TIDY_APPLY_CONFIG(obj->ptdoc->doc, options_str, options_ht);
1431-
1432-
if (php_tidy_parse_string(obj, ZSTR_VAL(contents), (uint32_t)ZSTR_LEN(contents), enc) == FAILURE) {
1432+
if (php_tidy_apply_config(obj->ptdoc->doc, options_str, options_ht) != SUCCESS
1433+
|| php_tidy_parse_string(obj, ZSTR_VAL(contents), (uint32_t)ZSTR_LEN(contents), enc) == FAILURE) {
14331434
RETVAL_FALSE;
14341435
} else {
14351436
RETVAL_TRUE;
@@ -1461,9 +1462,8 @@ PHP_METHOD(tidy, parseString)
14611462
TIDY_SET_CONTEXT;
14621463
obj = Z_TIDY_P(object);
14631464

1464-
TIDY_APPLY_CONFIG(obj->ptdoc->doc, options_str, options_ht);
1465-
1466-
if(php_tidy_parse_string(obj, ZSTR_VAL(input), (uint32_t)ZSTR_LEN(input), enc) == SUCCESS) {
1465+
if (php_tidy_apply_config(obj->ptdoc->doc, options_str, options_ht) == SUCCESS
1466+
&& php_tidy_parse_string(obj, ZSTR_VAL(input), (uint32_t)ZSTR_LEN(input), enc) == SUCCESS) {
14671467
RETURN_TRUE;
14681468
}
14691469

0 commit comments

Comments
 (0)