From 186788f149ea71f110a409b47410fd6bbb4cf398 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 21 Jul 2024 16:49:43 +0200 Subject: [PATCH] Fix error handling in tidy constructor --- NEWS | 4 ++++ UPGRADING | 4 ++++ ext/tidy/tests/bug54682.phpt | 10 ++++++++-- ext/tidy/tests/open_basedir_failure_config.phpt | 9 ++++++--- ext/tidy/tests/parsing_inexistent_file.phpt | 9 ++++++--- ext/tidy/tidy.c | 13 +++++++++---- 6 files changed, 37 insertions(+), 12 deletions(-) diff --git a/NEWS b/NEWS index b497a32349f..2beb7bea908 100644 --- a/NEWS +++ b/NEWS @@ -34,6 +34,10 @@ PHP NEWS . Fix references in request_parse_body() options array. (nielsdos) . Add RoundingMode enum. (timwolla, saki) +- Tidy: + . Failures in the constructor now throw exceptions rather than emitting + warnings and having a broken object. (nielsdos) + - XSL: . Fix trampoline leak in xpath callables. (nielsdos) diff --git a/UPGRADING b/UPGRADING index 0bbf43832cd..3940180fe01 100644 --- a/UPGRADING +++ b/UPGRADING @@ -167,6 +167,10 @@ PHP 8.4 UPGRADE NOTES . strcspn() with empty $characters now returns the length of the string instead of incorrectly stopping at the first NUL character. See GH-12592. +- Tidy: + . Failures in the constructor now throw exceptions rather than emitting + warnings and having a broken object. + - XML: . The xml_set_*_handler() functions now declare and check for an effective signature of callable|string|null for the $handler parameters. diff --git a/ext/tidy/tests/bug54682.phpt b/ext/tidy/tests/bug54682.phpt index 983a33017e8..333b89e804c 100644 --- a/ext/tidy/tests/bug54682.phpt +++ b/ext/tidy/tests/bug54682.phpt @@ -5,9 +5,15 @@ tidy --FILE-- diagnose(); +var_dump($nx); ?> --EXPECTF-- -Warning: tidy::__construct(): Cannot load "*" into memory%win %s on line %d +object(tidy)#%d (2) { + ["errorBuffer"]=> + NULL + ["value"]=> + NULL +} diff --git a/ext/tidy/tests/open_basedir_failure_config.phpt b/ext/tidy/tests/open_basedir_failure_config.phpt index b14393b7b80..3b12c9e4ea8 100644 --- a/ext/tidy/tests/open_basedir_failure_config.phpt +++ b/ext/tidy/tests/open_basedir_failure_config.phpt @@ -17,7 +17,11 @@ 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'); +try { + $tidy = new tidy(__DIR__.'/open_basedir/test.html', 'my_config_file.ini'); +} catch (Exception $e) { + echo $e->getMessage(), "\n"; +} echo "=== parseFile ===\n"; $tidy = new tidy; @@ -38,8 +42,7 @@ Warning: tidy_parse_string(): open_basedir restriction in effect. File(my_config 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 +tidy::__construct(): open_basedir restriction in effect. File(my_config_file.ini) is not within the allowed path(s): (%sopen_basedir) === 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 diff --git a/ext/tidy/tests/parsing_inexistent_file.phpt b/ext/tidy/tests/parsing_inexistent_file.phpt index 26dfea79857..e5f73c5388c 100644 --- a/ext/tidy/tests/parsing_inexistent_file.phpt +++ b/ext/tidy/tests/parsing_inexistent_file.phpt @@ -10,7 +10,11 @@ var_dump($tidy->parseFile("does_not_exist.html")); var_dump(tidy_parse_file("does_not_exist.html")); -$tidy = new tidy("does_not_exist.html"); +try { + $tidy = new tidy("does_not_exist.html"); +} catch (Exception $e) { + echo $e->getMessage(), "\n"; +} ?> --EXPECTF-- Warning: tidy::parseFile(): Cannot load "does_not_exist.html" into memory in %s on line %d @@ -18,5 +22,4 @@ bool(false) Warning: tidy_parse_file(): Cannot load "does_not_exist.html" into memory in %s on line %d bool(false) - -Warning: tidy::__construct(): Cannot load "does_not_exist.html" into memory in %s on line %d +Cannot load "does_not_exist.html" into memory diff --git a/ext/tidy/tidy.c b/ext/tidy/tidy.c index 1632462e49d..85f55808da9 100644 --- a/ext/tidy/tidy.c +++ b/ext/tidy/tidy.c @@ -40,6 +40,8 @@ #include "tidy_arginfo.h" +#include "Zend/zend_exceptions.h" + /* compatibility with older versions of libtidy */ #ifndef TIDY_CALL #define TIDY_CALL @@ -1371,8 +1373,8 @@ PHP_METHOD(tidy, __construct) if (inputfile) { if (!(contents = php_tidy_file_to_mem(ZSTR_VAL(inputfile), use_include_path))) { - php_error_docref(NULL, E_WARNING, "Cannot load \"%s\" into memory%s", ZSTR_VAL(inputfile), (use_include_path) ? " (using include path)" : ""); - return; + zend_throw_error(zend_ce_exception, "Cannot load \"%s\" into memory%s", ZSTR_VAL(inputfile), (use_include_path) ? " (using include path)" : ""); + RETURN_THROWS(); } if (ZEND_SIZE_T_UINT_OVFL(ZSTR_LEN(contents))) { @@ -1381,11 +1383,14 @@ PHP_METHOD(tidy, __construct) RETURN_THROWS(); } + zend_error_handling error_handling; + zend_replace_error_handling(EH_THROW, NULL, &error_handling); if (php_tidy_apply_config(obj->ptdoc->doc, options_str, options_ht) != SUCCESS) { - /* TODO: this is the constructor, we should throw probably... */ + zend_restore_error_handling(&error_handling); zend_string_release_ex(contents, 0); - RETURN_FALSE; + RETURN_THROWS(); } + zend_restore_error_handling(&error_handling); php_tidy_parse_string(obj, ZSTR_VAL(contents), (uint32_t)ZSTR_LEN(contents), enc);