Fix memory leaks in ext/tidy basedir restriction code #15046
Conversation
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.
| 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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yes this should, especially if the constructor can emit a warning
| 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)); | ||
| } |
There was a problem hiding this comment.
Note to myself, see if the HashTable and underlying char pointer can be made const.
| 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) { |
There was a problem hiding this comment.
Why is one != SUCCESS and the other one is == FAILURE? I think it would be better if both use the same comparison check.
| 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) { |
| 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; |
There was a problem hiding this comment.
Yes this should, especially if the constructor can emit a warning
| 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) { |
There was a problem hiding this comment.
Also not for a follow-up PR that this could use RETVAL_BOOL()
| 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; | ||
| } |
There was a problem hiding this comment.
Note for a follow-up PR, this could just use a RETURN_BOOL()
| === __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 |
There was a problem hiding this comment.
This definitely needs to be addressed in master, as constructors must always throw an exception when they fail.
|
Ok I am gonna merge this and make a follow-up for master. Thanks for the input. |
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.
This was detected using an experimental static analyser I'm developing.