From 6a6c3efebac8c312055b116b0aefb09ef3b52899 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Thu, 17 Oct 2024 12:06:07 +0100 Subject: [PATCH 1/3] Fix GH-16477 (Segmentation fault when calling __debugInfo() after failed SplFileObject::__constructor) --- ext/spl/spl_directory.c | 14 +++++++------- ext/spl/tests/gh16477-2.phpt | 19 +++++++++++++++++++ ext/spl/tests/gh16477.phpt | 19 +++++++++++++++++++ 3 files changed, 45 insertions(+), 7 deletions(-) create mode 100644 ext/spl/tests/gh16477-2.phpt create mode 100644 ext/spl/tests/gh16477.phpt diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c index 0440ff59c8ba9..bb6984350973b 100644 --- a/ext/spl/spl_directory.c +++ b/ext/spl/spl_directory.c @@ -2047,13 +2047,7 @@ PHP_METHOD(SplFileObject, __construct) size_t path_len; zend_error_handling error_handling; - intern->u.file.open_mode = ZSTR_CHAR('r'); - - if (zend_parse_parameters(ZEND_NUM_ARGS(), "P|Sbr!", - &intern->file_name, &open_mode, - &use_include_path, &intern->u.file.zcontext) == FAILURE) { - intern->u.file.open_mode = NULL; - intern->file_name = NULL; + if (zend_parse_parameters(ZEND_NUM_ARGS(), "P|Sbr!", &intern->file_name, &open_mode, &use_include_path, &intern->u.file.zcontext) == FAILURE) { RETURN_THROWS(); } @@ -2096,6 +2090,12 @@ PHP_METHOD(SplTempFileObject, __construct) RETURN_THROWS(); } + /* Prevent reinitialization of Object */ + if (intern->u.file.stream) { + zend_throw_error(NULL, "cannot call constructor twice"); + RETURN_THROWS(); + } + if (max_memory < 0) { file_name = zend_string_init("php://memory", sizeof("php://memory")-1, 0); } else if (ZEND_NUM_ARGS()) { diff --git a/ext/spl/tests/gh16477-2.phpt b/ext/spl/tests/gh16477-2.phpt new file mode 100644 index 0000000000000..04e21602ab96a --- /dev/null +++ b/ext/spl/tests/gh16477-2.phpt @@ -0,0 +1,19 @@ +--TEST-- +GH-16477-2: Memory leak when calling SplTempFileObject::__constructor() twice +--FILE-- +__construct(); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), PHP_EOL; +} +$obj->__debugInfo(); + +?> +DONE +--EXPECT-- +Error: cannot call constructor twice +DONE diff --git a/ext/spl/tests/gh16477.phpt b/ext/spl/tests/gh16477.phpt new file mode 100644 index 0000000000000..f35c9538e8556 --- /dev/null +++ b/ext/spl/tests/gh16477.phpt @@ -0,0 +1,19 @@ +--TEST-- +GH-16477: Segmentation fault when calling __debugInfo() after failed SplFileObject::__constructor +--FILE-- +__construct(); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), PHP_EOL; +} +$obj->__debugInfo(); + +?> +DONE +--EXPECT-- +ArgumentCountError: SplFileObject::__construct() expects at least 1 argument, 0 given +DONE From e4b710821d82e774f9944e6decea1c6a1bb0b39b Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Thu, 17 Oct 2024 17:21:09 +0100 Subject: [PATCH 2/3] Prevent dangling pointers --- ext/spl/spl_directory.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c index bb6984350973b..95b86085b6ff2 100644 --- a/ext/spl/spl_directory.c +++ b/ext/spl/spl_directory.c @@ -2042,16 +2042,22 @@ static void spl_filesystem_file_rewind(zval * this_ptr, spl_filesystem_object *i PHP_METHOD(SplFileObject, __construct) { spl_filesystem_object *intern = spl_filesystem_from_obj(Z_OBJ_P(ZEND_THIS)); + zend_string *file_name = NULL; zend_string *open_mode = ZSTR_CHAR('r'); + zval *stream_context = NULL; bool use_include_path = 0; size_t path_len; zend_error_handling error_handling; - if (zend_parse_parameters(ZEND_NUM_ARGS(), "P|Sbr!", &intern->file_name, &open_mode, &use_include_path, &intern->u.file.zcontext) == FAILURE) { + if (zend_parse_parameters(ZEND_NUM_ARGS(), "P|Sbr!", &file_name, &open_mode, &use_include_path, &stream_context) == FAILURE) { RETURN_THROWS(); } + // TODO This should probably take a copy: + // intern->file_name = zend_string_copy(file_name); + intern->file_name = file_name; intern->u.file.open_mode = zend_string_copy(open_mode); + intern->u.file.zcontext = stream_context; /* spl_filesystem_file_open() can generate E_WARNINGs which we want to promote to exceptions */ zend_replace_error_handling(EH_THROW, spl_ce_RuntimeException, &error_handling); From f6391250295042fbf8e9acf9c129cdc09adf599e Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Fri, 25 Oct 2024 18:01:22 +0100 Subject: [PATCH 3/3] Fix review comments --- ext/spl/spl_directory.c | 7 +++---- ext/spl/tests/gh16477-2.phpt | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c index 95b86085b6ff2..83653f65e9f5f 100644 --- a/ext/spl/spl_directory.c +++ b/ext/spl/spl_directory.c @@ -2053,10 +2053,9 @@ PHP_METHOD(SplFileObject, __construct) RETURN_THROWS(); } - // TODO This should probably take a copy: - // intern->file_name = zend_string_copy(file_name); - intern->file_name = file_name; intern->u.file.open_mode = zend_string_copy(open_mode); + /* file_name and zcontext are copied by spl_filesystem_file_open() */ + intern->file_name = file_name; intern->u.file.zcontext = stream_context; /* spl_filesystem_file_open() can generate E_WARNINGs which we want to promote to exceptions */ @@ -2098,7 +2097,7 @@ PHP_METHOD(SplTempFileObject, __construct) /* Prevent reinitialization of Object */ if (intern->u.file.stream) { - zend_throw_error(NULL, "cannot call constructor twice"); + zend_throw_error(NULL, "Cannot call constructor twice"); RETURN_THROWS(); } diff --git a/ext/spl/tests/gh16477-2.phpt b/ext/spl/tests/gh16477-2.phpt index 04e21602ab96a..a51b9408c2d44 100644 --- a/ext/spl/tests/gh16477-2.phpt +++ b/ext/spl/tests/gh16477-2.phpt @@ -15,5 +15,5 @@ $obj->__debugInfo(); ?> DONE --EXPECT-- -Error: cannot call constructor twice +Error: Cannot call constructor twice DONE