From 52e9b1c75814e2121d9260a35746928a469fedba Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 27 May 2022 10:26:15 +0100 Subject: [PATCH 1/5] Use bool for silent parameter --- ext/spl/spl_directory.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c index bfeb7f185327b..88959ca833542 100644 --- a/ext/spl/spl_directory.c +++ b/ext/spl/spl_directory.c @@ -1944,7 +1944,7 @@ static zend_result spl_filesystem_file_read_csv(spl_filesystem_object *intern, c } /* }}} */ -static zend_result spl_filesystem_file_read_line_ex(zval * this_ptr, spl_filesystem_object *intern, int silent) /* {{{ */ +static zend_result spl_filesystem_file_read_line_ex(zval * this_ptr, spl_filesystem_object *intern, bool silent) /* {{{ */ { zval retval; @@ -2026,7 +2026,7 @@ static bool spl_filesystem_file_is_empty_line(spl_filesystem_object *intern) /* } /* }}} */ -static zend_result spl_filesystem_file_read_line(zval * this_ptr, spl_filesystem_object *intern, int silent) /* {{{ */ +static zend_result spl_filesystem_file_read_line(zval * this_ptr, spl_filesystem_object *intern, bool silent) /* {{{ */ { zend_result ret = spl_filesystem_file_read_line_ex(this_ptr, intern, silent); @@ -2052,7 +2052,7 @@ static void spl_filesystem_file_rewind(zval * this_ptr, spl_filesystem_object *i intern->u.file.current_line_num = 0; } if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_AHEAD)) { - spl_filesystem_file_read_line(this_ptr, intern, 1); + spl_filesystem_file_read_line(this_ptr, intern, /* silent */ true); } } /* }}} */ @@ -2206,7 +2206,7 @@ PHP_METHOD(SplFileObject, current) CHECK_SPL_FILE_OBJECT_IS_INITIALIZED(intern); if (!intern->u.file.current_line && Z_ISUNDEF(intern->u.file.current_zval)) { - spl_filesystem_file_read_line(ZEND_THIS, intern, 1); + spl_filesystem_file_read_line(ZEND_THIS, intern, /* silent */ true); } if (intern->u.file.current_line && (!SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_CSV) || Z_ISUNDEF(intern->u.file.current_zval))) { RETURN_STRINGL(intern->u.file.current_line, intern->u.file.current_line_len); @@ -2228,7 +2228,7 @@ PHP_METHOD(SplFileObject, key) /* Do not read the next line to support correct counting with fgetc() if (!intern->u.file.current_line) { - spl_filesystem_file_read_line(ZEND_THIS, intern, 1); + spl_filesystem_file_read_line(ZEND_THIS, intern, silent true); } */ RETURN_LONG(intern->u.file.current_line_num); } /* }}} */ @@ -2244,7 +2244,7 @@ PHP_METHOD(SplFileObject, next) spl_filesystem_file_free_line(intern); if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_AHEAD)) { - spl_filesystem_file_read_line(ZEND_THIS, intern, 1); + spl_filesystem_file_read_line(ZEND_THIS, intern, /* silent */ true); } intern->u.file.current_line_num++; } /* }}} */ @@ -2743,7 +2743,7 @@ PHP_METHOD(SplFileObject, seek) spl_filesystem_file_rewind(ZEND_THIS, intern); for (i = 0; i < line_pos; i++) { - if (spl_filesystem_file_read_line(ZEND_THIS, intern, 1) == FAILURE) { + if (spl_filesystem_file_read_line(ZEND_THIS, intern, /* silent */ true) == FAILURE) { return; } } From db7ff5a100ba117fb00427fbce85a2796e9c665c Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 27 May 2022 12:19:57 +0100 Subject: [PATCH 2/5] Fix GH-8561 and GH-8562 These bugs need a header change, and thus are an ABI break --- ext/spl/spl_directory.c | 24 ++++++++++++++++++++---- ext/spl/spl_directory.h | 3 ++- ext/spl/tests/SplFileObject/gh8561.phpt | 22 ++++++++++++++++++++++ ext/spl/tests/SplFileObject/gh8562.phpt | 22 ++++++++++++++++++++++ 4 files changed, 66 insertions(+), 5 deletions(-) create mode 100644 ext/spl/tests/SplFileObject/gh8561.phpt create mode 100644 ext/spl/tests/SplFileObject/gh8562.phpt diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c index 88959ca833542..65c24198da2ff 100644 --- a/ext/spl/spl_directory.c +++ b/ext/spl/spl_directory.c @@ -2047,10 +2047,12 @@ static void spl_filesystem_file_rewind(zval * this_ptr, spl_filesystem_object *i } if (-1 == php_stream_rewind(intern->u.file.stream)) { zend_throw_exception_ex(spl_ce_RuntimeException, 0, "Cannot rewind file %s", ZSTR_VAL(intern->file_name)); - } else { - spl_filesystem_file_free_line(intern); - intern->u.file.current_line_num = 0; + return; } + + spl_filesystem_file_free_line(intern); + intern->u.file.current_line_num = 0; + intern->u.file.lines_to_catch_up = 0; if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_AHEAD)) { spl_filesystem_file_read_line(this_ptr, intern, /* silent */ true); } @@ -2191,6 +2193,8 @@ PHP_METHOD(SplFileObject, fgets) if (spl_filesystem_file_read_ex(intern, /* silent */ false, /* line_add */ 1) == FAILURE) { RETURN_THROWS(); } + /* Inform current() that it needs to catch up a line */ + intern->u.file.lines_to_catch_up++; RETURN_STRINGL(intern->u.file.current_line, intern->u.file.current_line_len); } /* }}} */ @@ -2205,9 +2209,19 @@ PHP_METHOD(SplFileObject, current) CHECK_SPL_FILE_OBJECT_IS_INITIALIZED(intern); + /* No lines have been read yet (rewind or just opened file) */ if (!intern->u.file.current_line && Z_ISUNDEF(intern->u.file.current_zval)) { spl_filesystem_file_read_line(ZEND_THIS, intern, /* silent */ true); } + /* next() of fgets() has been called and we need to read the line which is now pointed at */ + if (intern->u.file.lines_to_catch_up > 0) { + ZEND_ASSERT(intern->u.file.current_line || !Z_ISUNDEF(intern->u.file.current_zval)); + for (; 0 < intern->u.file.lines_to_catch_up; intern->u.file.lines_to_catch_up--) { + /* Free fetched line */ + spl_filesystem_file_free_line(intern); + spl_filesystem_file_read_line(ZEND_THIS, intern, /* silent */ true); + } + } if (intern->u.file.current_line && (!SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_CSV) || Z_ISUNDEF(intern->u.file.current_zval))) { RETURN_STRINGL(intern->u.file.current_line, intern->u.file.current_line_len); } else if (!Z_ISUNDEF(intern->u.file.current_zval)) { @@ -2242,9 +2256,11 @@ PHP_METHOD(SplFileObject, next) RETURN_THROWS(); } - spl_filesystem_file_free_line(intern); if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_AHEAD)) { + spl_filesystem_file_free_line(intern); spl_filesystem_file_read_line(ZEND_THIS, intern, /* silent */ true); + } else { + intern->u.file.lines_to_catch_up++; } intern->u.file.current_line_num++; } /* }}} */ diff --git a/ext/spl/spl_directory.h b/ext/spl/spl_directory.h index 0a99a0778be1e..273654ec367a2 100644 --- a/ext/spl/spl_directory.h +++ b/ext/spl/spl_directory.h @@ -85,7 +85,8 @@ struct _spl_filesystem_object { char *current_line; size_t current_line_len; size_t max_line_len; - zend_long current_line_num; + zend_long current_line_num; + zend_long lines_to_catch_up; zval zresource; zend_function *func_getCurr; char delimiter; diff --git a/ext/spl/tests/SplFileObject/gh8561.phpt b/ext/spl/tests/SplFileObject/gh8561.phpt new file mode 100644 index 0000000000000..1fb8286a23059 --- /dev/null +++ b/ext/spl/tests/SplFileObject/gh8561.phpt @@ -0,0 +1,22 @@ +--TEST-- +Bug GH-8561: SplFileObject: key() and current() unsinchronized after call to fgets() +--FILE-- +fwrite("line {$i}" . PHP_EOL); +} + +$file->rewind(); + +// read first line +$file->fgets(); + +// where am I? +echo $file->key(), ': ', $file->current(); + +?> +--EXPECT-- +1: line 1 diff --git a/ext/spl/tests/SplFileObject/gh8562.phpt b/ext/spl/tests/SplFileObject/gh8562.phpt new file mode 100644 index 0000000000000..64e269dbd9e0d --- /dev/null +++ b/ext/spl/tests/SplFileObject/gh8562.phpt @@ -0,0 +1,22 @@ +--TEST-- +Bug GH-8562: SplFileObject: current() returns wrong result after call to next() +--FILE-- +fwrite("line {$i}" . PHP_EOL); +} + +$file->rewind(); + +$file->next(); +echo $file->key(), ': ', $file->current(); +$file->next(); +echo $file->key(), ': ', $file->current(); + +?> +--EXPECT-- +1: line 1 +2: line 2 From b3eb7e88a3bbd7b184e2641ff802da50bf29954d Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 27 May 2022 13:13:50 +0100 Subject: [PATCH 3/5] Fix GH-8563 With memory streams if we get a NULL buffer we must not instantiate an empty line --- ext/spl/spl_directory.c | 23 +++++---- .../SplFileObject_fgetcsv_basic.phpt | 5 -- .../SplFileObject_key_error001.phpt | 2 +- .../SplFileObject_key_error002.phpt | 2 +- .../tests/SplFileObject/fileobject_001.phpt | 2 +- ext/spl/tests/SplFileObject/gh8563.phpt | 48 +++++++++++++++++++ 6 files changed, 62 insertions(+), 20 deletions(-) create mode 100644 ext/spl/tests/SplFileObject/gh8563.phpt diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c index 65c24198da2ff..e9cca7021bd4e 100644 --- a/ext/spl/spl_directory.c +++ b/ext/spl/spl_directory.c @@ -1892,22 +1892,21 @@ static zend_result spl_filesystem_file_read_ex(spl_filesystem_object *intern, bo } if (!buf) { - intern->u.file.current_line = estrdup(""); - intern->u.file.current_line_len = 0; - } else { - if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_DROP_NEW_LINE)) { - if (line_len > 0 && buf[line_len - 1] == '\n') { + return FAILURE; + } + + if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_DROP_NEW_LINE)) { + if (line_len > 0 && buf[line_len - 1] == '\n') { + line_len--; + if (line_len > 0 && buf[line_len - 1] == '\r') { line_len--; - if (line_len > 0 && buf[line_len - 1] == '\r') { - line_len--; - } - buf[line_len] = '\0'; } + buf[line_len] = '\0'; } - - intern->u.file.current_line = buf; - intern->u.file.current_line_len = line_len; } + + intern->u.file.current_line = buf; + intern->u.file.current_line_len = line_len; intern->u.file.current_line_num += line_add; return SUCCESS; diff --git a/ext/spl/tests/SplFileObject/SplFileObject_fgetcsv_basic.phpt b/ext/spl/tests/SplFileObject/SplFileObject_fgetcsv_basic.phpt index 9b68d9015cc96..ca584251af9f4 100644 --- a/ext/spl/tests/SplFileObject/SplFileObject_fgetcsv_basic.phpt +++ b/ext/spl/tests/SplFileObject/SplFileObject_fgetcsv_basic.phpt @@ -14,7 +14,6 @@ fclose($fp); $fo = new SplFileObject('SplFileObject__fgetcsv1.csv'); var_dump($fo->fgetcsv()); var_dump($fo->fgetcsv()); -var_dump($fo->fgetcsv()); ?> --CLEAN-- string(1) "5" } -array(1) { - [0]=> - NULL -} bool(false) diff --git a/ext/spl/tests/SplFileObject/SplFileObject_key_error001.phpt b/ext/spl/tests/SplFileObject/SplFileObject_key_error001.phpt index 0c21d0b905e95..7d0e3ae8d9698 100644 --- a/ext/spl/tests/SplFileObject/SplFileObject_key_error001.phpt +++ b/ext/spl/tests/SplFileObject/SplFileObject_key_error001.phpt @@ -18,5 +18,5 @@ var_dump($s->key()); var_dump($s->valid()); ?> --EXPECT-- -int(14) +int(12) bool(false) diff --git a/ext/spl/tests/SplFileObject/SplFileObject_key_error002.phpt b/ext/spl/tests/SplFileObject/SplFileObject_key_error002.phpt index 8fc9b7fef0a58..0834dbc0524fc 100644 --- a/ext/spl/tests/SplFileObject/SplFileObject_key_error002.phpt +++ b/ext/spl/tests/SplFileObject/SplFileObject_key_error002.phpt @@ -18,5 +18,5 @@ var_dump($s->key()); var_dump($s->valid()); ?> --EXPECT-- -int(13) +int(12) bool(false) diff --git a/ext/spl/tests/SplFileObject/fileobject_001.phpt b/ext/spl/tests/SplFileObject/fileobject_001.phpt index 5c2240b3c5972..272428e2e9bc7 100644 --- a/ext/spl/tests/SplFileObject/fileobject_001.phpt +++ b/ext/spl/tests/SplFileObject/fileobject_001.phpt @@ -70,7 +70,7 @@ string(1) "4" int(5) string(1) "5" int(6) -string(0) "" +bool(false) ===B=== int(0) string(1) "0" diff --git a/ext/spl/tests/SplFileObject/gh8563.phpt b/ext/spl/tests/SplFileObject/gh8563.phpt new file mode 100644 index 0000000000000..11309ae8101fa --- /dev/null +++ b/ext/spl/tests/SplFileObject/gh8563.phpt @@ -0,0 +1,48 @@ +--TEST-- +Bug GH-8563: Different results for seek() on SplFileObject and SplTempFileObject +--FILE-- +fwrite("line {$i}" . PHP_EOL); + $file_02->fwrite("line {$i}" . PHP_EOL); + $file_03->fwrite("line {$i}" . PHP_EOL); + $file_04->fwrite("line {$i}" . PHP_EOL); +} + +// reset +$file_01->rewind(); +$file_02->rewind(); +$file_03->rewind(); +$file_04->rewind(); + +// seek +$file_01->seek(INDEX); +$file_02->seek(INDEX); +$file_03->seek(INDEX); +$file_04->seek(INDEX); + +// show results +echo 'file_01: ' . $file_01->key(), PHP_EOL; +echo 'file_02: ' . $file_02->key(), PHP_EOL; +echo 'file_03: ' . $file_03->key(), PHP_EOL; +echo 'file_04: ' . $file_04->key(), PHP_EOL; +?> +--CLEAN-- + +--EXPECT-- +file_01: 4 +file_02: 4 +file_03: 4 +file_04: 4 From 45f5b638040c3dac06da5173f8b923f66868828e Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 27 May 2022 16:54:18 +0100 Subject: [PATCH 4/5] "Fix" tests, clearly there is a bug with Phar --- ext/phar/tests/phar_oo_008.phpt | 4 ++-- ext/spl/tests/bug81477.phpt | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/ext/phar/tests/phar_oo_008.phpt b/ext/phar/tests/phar_oo_008.phpt index 96ba8347ec754..8123d69b008e4 100644 --- a/ext/phar/tests/phar_oo_008.phpt +++ b/ext/phar/tests/phar_oo_008.phpt @@ -107,8 +107,8 @@ __halt_compiler(); 2=>3|c|'e' ===5=== 0=>1|2|3 -1=>2|a|b -2=>3|c|'e' +2=>2|a|b +4=>3|c|'e' ===6=== MyCSVFile2::getCurrentLine 1=>1|2|3 diff --git a/ext/spl/tests/bug81477.phpt b/ext/spl/tests/bug81477.phpt index f7730a791aa03..421c74dc4d68e 100644 --- a/ext/spl/tests/bug81477.phpt +++ b/ext/spl/tests/bug81477.phpt @@ -21,7 +21,6 @@ string(8) "baz,bat " string(10) "more,data " -string(0) "" --CLEAN-- Date: Fri, 27 May 2022 18:44:46 +0100 Subject: [PATCH 5/5] Maybe fix Phar bug? At least I figured out something is hapenning when setting a custom InfoClass with a custom current() --- ext/phar/tests/phar_oo_008.phpt | 4 ++-- ext/spl/spl_directory.c | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ext/phar/tests/phar_oo_008.phpt b/ext/phar/tests/phar_oo_008.phpt index 8123d69b008e4..96ba8347ec754 100644 --- a/ext/phar/tests/phar_oo_008.phpt +++ b/ext/phar/tests/phar_oo_008.phpt @@ -107,8 +107,8 @@ __halt_compiler(); 2=>3|c|'e' ===5=== 0=>1|2|3 -2=>2|a|b -4=>3|c|'e' +1=>2|a|b +2=>3|c|'e' ===6=== MyCSVFile2::getCurrentLine 1=>1|2|3 diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c index e9cca7021bd4e..b469e4bdf0087 100644 --- a/ext/spl/spl_directory.c +++ b/ext/spl/spl_directory.c @@ -1979,6 +1979,7 @@ static zend_result spl_filesystem_file_read_line_ex(zval * this_ptr, spl_filesys spl_filesystem_file_free_line(intern); intern->u.file.current_line = estrndup(Z_STRVAL(retval), Z_STRLEN(retval)); intern->u.file.current_line_len = Z_STRLEN(retval); + intern->u.file.lines_to_catch_up = 0; zval_ptr_dtor(&retval); return SUCCESS; } else {