Skip to content

Fix memory leaks in ext/tidy basedir restriction code #15046

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix memory leaks in ext/tidy basedir restriction code #15046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ext/tidy/tests/open_basedir/test.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<p>my epic string</p>
48 changes: 48 additions & 0 deletions ext/tidy/tests/open_basedir_failure_config.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
--TEST--
Tidy with basedir restriction failure on configuration file
--EXTENSIONS--
tidy
--INI--
open_basedir={PWD}/open_basedir
--FILE--
<?php
echo "=== repairString ===\n";
$tidy = new tidy;
$tidy->repairString('my epic string', 'my_config_file.ini');

echo "=== tidy_parse_string ===\n";
tidy_parse_string('my epic string', 'my_config_file.ini');

echo "=== tidy_parse_file ===\n";
tidy_parse_file(__DIR__.'/open_basedir/test.html', 'my_config_file.ini');

echo "=== __construct ===\n";
$tidy = new tidy(__DIR__.'/open_basedir/test.html', 'my_config_file.ini');

echo "=== parseFile ===\n";
$tidy = new tidy;
$tidy->parseFile(__DIR__.'/open_basedir/test.html', 'my_config_file.ini');

echo "=== parseString ===\n";
$tidy = new tidy;
$tidy->parseString('my epic string', 'my_config_file.ini');
?>
--EXPECTF--
=== repairString ===

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
=== tidy_parse_string ===

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
=== tidy_parse_file ===

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
=== __construct ===

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
Comment on lines +40 to +42
Copy link
Member

Choose a reason for hiding this comment

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

This definitely needs to be addressed in master, as constructors must always throw an exception when they fail.

=== parseFile ===

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
=== parseString ===

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
58 changes: 29 additions & 29 deletions ext/tidy/tidy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,19 +74,6 @@
} \
obj = Z_TIDY_P(object); \

#define TIDY_APPLY_CONFIG(_doc, _val_str, _val_ht) \
if (_val_ht) { \
_php_tidy_apply_config_array(_doc, _val_ht); \
} else if (_val_str) { \
TIDY_OPEN_BASE_DIR_CHECK(ZSTR_VAL(_val_str)); \
php_tidy_load_config(_doc, ZSTR_VAL(_val_str)); \
}

#define TIDY_OPEN_BASE_DIR_CHECK(filename) \
if (php_check_open_basedir(filename)) { \
RETURN_FALSE; \
} \

#define TIDY_SET_DEFAULT_CONFIG(_doc) \
if (TG(default_config) && TG(default_config)[0]) { \
php_tidy_load_config(_doc, TG(default_config)); \
Expand Down Expand Up @@ -221,6 +208,19 @@ static void php_tidy_load_config(TidyDoc doc, const char *path)
}
}

static zend_result php_tidy_apply_config(TidyDoc doc, zend_string *str_string, HashTable *ht_options)
{
if (ht_options) {
return _php_tidy_apply_config_array(doc, ht_options);
} else if (str_string) {
if (php_check_open_basedir(ZSTR_VAL(str_string))) {
return FAILURE;
}
php_tidy_load_config(doc, ZSTR_VAL(str_string));
}
Comment on lines +213 to +220
Copy link
Member

Choose a reason for hiding this comment

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

Note to myself, see if the HashTable and underlying char pointer can be made const.

return SUCCESS;
}

static int _php_tidy_set_tidy_opt(TidyDoc doc, char *optname, zval *value)
{
TidyOption opt = tidyGetOptionByName(doc, optname);
Expand Down Expand Up @@ -329,9 +329,9 @@ static void php_tidy_quick_repair(INTERNAL_FUNCTION_PARAMETERS, bool is_file)

TIDY_SET_DEFAULT_CONFIG(doc);

TIDY_APPLY_CONFIG(doc, config_str, config_ht);

if(enc_len) {
if (php_tidy_apply_config(doc, config_str, config_ht) != SUCCESS) {
RETVAL_FALSE;
} else if (enc_len) {
if (tidySetCharEncoding(doc, enc) < 0) {
php_error_docref(NULL, E_WARNING, "Could not set encoding \"%s\"", enc);
RETVAL_FALSE;
Expand Down Expand Up @@ -1024,9 +1024,8 @@ PHP_FUNCTION(tidy_parse_string)
tidy_instantiate(tidy_ce_doc, return_value);
obj = Z_TIDY_P(return_value);

TIDY_APPLY_CONFIG(obj->ptdoc->doc, options_str, options_ht);

if (php_tidy_parse_string(obj, ZSTR_VAL(input), (uint32_t)ZSTR_LEN(input), enc) == FAILURE) {
if (php_tidy_apply_config(obj->ptdoc->doc, options_str, options_ht) != SUCCESS
|| php_tidy_parse_string(obj, ZSTR_VAL(input), (uint32_t)ZSTR_LEN(input), enc) == FAILURE) {
Comment on lines +1027 to +1028
Copy link
Member

Choose a reason for hiding this comment

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

Why is one != SUCCESS and the other one is == FAILURE? I think it would be better if both use the same comparison check.

zval_ptr_dtor(return_value);
RETURN_FALSE;
}
Expand Down Expand Up @@ -1093,9 +1092,8 @@ PHP_FUNCTION(tidy_parse_file)
tidy_instantiate(tidy_ce_doc, return_value);
obj = Z_TIDY_P(return_value);

TIDY_APPLY_CONFIG(obj->ptdoc->doc, options_str, options_ht);

if (php_tidy_parse_string(obj, ZSTR_VAL(contents), (uint32_t)ZSTR_LEN(contents), enc) == FAILURE) {
if (php_tidy_apply_config(obj->ptdoc->doc, options_str, options_ht) != SUCCESS
|| php_tidy_parse_string(obj, ZSTR_VAL(contents), (uint32_t)ZSTR_LEN(contents), enc) == FAILURE) {
Comment on lines +1095 to +1096
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

zval_ptr_dtor(return_value);
RETVAL_FALSE;
}
Expand Down Expand Up @@ -1388,7 +1386,11 @@ PHP_METHOD(tidy, __construct)
RETURN_THROWS();
}

TIDY_APPLY_CONFIG(obj->ptdoc->doc, options_str, options_ht);
if (php_tidy_apply_config(obj->ptdoc->doc, options_str, options_ht) != SUCCESS) {
/* TODO: this is the constructor, we should throw probably... */
zend_string_release_ex(contents, 0);
RETURN_FALSE;
Copy link
Member

Choose a reason for hiding this comment

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

not sure about this indeed.

Copy link
Member Author

Choose a reason for hiding this comment

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

This keeps the existing behaviour of calling ->__construct(...) manually, and yes it returns false in that case. Constructors don't necessarily have a void return :(
I believe this should be at least fixed for master by throwing an exception instead. cc @Girgias

Copy link
Member

Choose a reason for hiding this comment

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

Yes this should, especially if the constructor can emit a warning

}

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

Expand Down Expand Up @@ -1427,9 +1429,8 @@ PHP_METHOD(tidy, parseFile)
RETURN_THROWS();
}

TIDY_APPLY_CONFIG(obj->ptdoc->doc, options_str, options_ht);

if (php_tidy_parse_string(obj, ZSTR_VAL(contents), (uint32_t)ZSTR_LEN(contents), enc) == FAILURE) {
if (php_tidy_apply_config(obj->ptdoc->doc, options_str, options_ht) != SUCCESS
|| php_tidy_parse_string(obj, ZSTR_VAL(contents), (uint32_t)ZSTR_LEN(contents), enc) == FAILURE) {
Comment on lines +1432 to +1433
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Member

Choose a reason for hiding this comment

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

Also not for a follow-up PR that this could use RETVAL_BOOL()

RETVAL_FALSE;
} else {
RETVAL_TRUE;
Expand Down Expand Up @@ -1461,9 +1462,8 @@ PHP_METHOD(tidy, parseString)
TIDY_SET_CONTEXT;
obj = Z_TIDY_P(object);

TIDY_APPLY_CONFIG(obj->ptdoc->doc, options_str, options_ht);

if(php_tidy_parse_string(obj, ZSTR_VAL(input), (uint32_t)ZSTR_LEN(input), enc) == SUCCESS) {
if (php_tidy_apply_config(obj->ptdoc->doc, options_str, options_ht) == SUCCESS
&& php_tidy_parse_string(obj, ZSTR_VAL(input), (uint32_t)ZSTR_LEN(input), enc) == SUCCESS) {
RETURN_TRUE;
}
Comment on lines +1465 to 1468
Copy link
Member

Choose a reason for hiding this comment

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

Note for a follow-up PR, this could just use a RETURN_BOOL()


Expand Down
Loading