From 5452d65baa2088d44b86202bf7fb800f0b3d50dc Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 5 Apr 2021 17:49:59 +0200 Subject: [PATCH 1/6] Fix #80933: SplFileObject::fgets() stops at NUL byte for DROP_NEW_LINE `buf` may contain NUL bytes, so we must not use `strcspn()` but rather the binary safe `php_strcspn()`. Signed-off-by: Christoph M. Becker --- ext/spl/spl_directory.c | 3 ++- ext/spl/tests/bug80933.phpt | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 ext/spl/tests/bug80933.phpt diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c index daed72d847089..e105155b21eca 100644 --- a/ext/spl/spl_directory.c +++ b/ext/spl/spl_directory.c @@ -2046,7 +2046,8 @@ static int spl_filesystem_file_read(spl_filesystem_object *intern, int silent) / intern->u.file.current_line_len = 0; } else { if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_DROP_NEW_LINE)) { - line_len = strcspn(buf, "\r\n"); + const char breaks[] = "\r\n"; + line_len = php_strcspn(buf, breaks, buf + line_len, breaks + sizeof(breaks)-1); buf[line_len] = '\0'; } diff --git a/ext/spl/tests/bug80933.phpt b/ext/spl/tests/bug80933.phpt new file mode 100644 index 0000000000000..2cfdb99e23f99 --- /dev/null +++ b/ext/spl/tests/bug80933.phpt @@ -0,0 +1,22 @@ +--TEST-- +Bug #80933 (SplFileObject::fgets() stops at NUL byte for DROP_NEW_LINE) +--FILE-- +fwrite($line); + +$temp->rewind(); +$read = $temp->fgets(); +var_dump($line === $read); + +$temp->rewind(); +$temp->setFlags(SplFileObject::DROP_NEW_LINE); +$read = $temp->fgets(); +var_dump($line === $read); +?> +--EXPECT-- +bool(true) +bool(true) From 0fa3859e30b85fe3837359bbb153e0a1dba6f8b8 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 5 Apr 2021 19:16:22 +0200 Subject: [PATCH 2/6] Drop const qualifier `php_strcspn()` expects `char*`s prior to PHP 8.0.0. Signed-off-by: Christoph M. Becker --- ext/spl/spl_directory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c index e105155b21eca..d70d81ddcaa91 100644 --- a/ext/spl/spl_directory.c +++ b/ext/spl/spl_directory.c @@ -2046,7 +2046,7 @@ static int spl_filesystem_file_read(spl_filesystem_object *intern, int silent) / intern->u.file.current_line_len = 0; } else { if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_DROP_NEW_LINE)) { - const char breaks[] = "\r\n"; + char breaks[] = "\r\n"; line_len = php_strcspn(buf, breaks, buf + line_len, breaks + sizeof(breaks)-1); buf[line_len] = '\0'; } From 4621da5ad1b2fd2dd03797514c7f194c4969352b Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Wed, 7 Apr 2021 12:29:48 +0200 Subject: [PATCH 3/6] Properly cater to line endings We must not assume that either CR or LF are valid line endings, but should use `php_stream_locate_eol()` in the first place. --- ext/spl/spl_directory.c | 10 +++++++--- ext/spl/tests/bug80933.phpt | 31 ++++++++++++++++++------------- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c index d70d81ddcaa91..f679c35cacfe0 100644 --- a/ext/spl/spl_directory.c +++ b/ext/spl/spl_directory.c @@ -2046,9 +2046,13 @@ static int spl_filesystem_file_read(spl_filesystem_object *intern, int silent) / intern->u.file.current_line_len = 0; } else { if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_DROP_NEW_LINE)) { - char breaks[] = "\r\n"; - line_len = php_strcspn(buf, breaks, buf + line_len, breaks + sizeof(breaks)-1); - buf[line_len] = '\0'; + zend_string *zbuf = zend_string_init(buf, line_len, 0); + const char *eol = php_stream_locate_eol(intern->u.file.stream, zbuf); + if (eol != NULL) { + line_len = eol - ZSTR_VAL(zbuf); + buf[line_len] = '\0'; + } + zend_string_release(zbuf); } intern->u.file.current_line = buf; diff --git a/ext/spl/tests/bug80933.phpt b/ext/spl/tests/bug80933.phpt index 2cfdb99e23f99..543d4e0365053 100644 --- a/ext/spl/tests/bug80933.phpt +++ b/ext/spl/tests/bug80933.phpt @@ -1,22 +1,27 @@ --TEST-- -Bug #80933 (SplFileObject::fgets() stops at NUL byte for DROP_NEW_LINE) +Bug #80933 (SplFileObject::DROP_NEW_LINE is broken for NUL and CR) --FILE-- fwrite($line); -$temp = new SplTempFileObject(); -$temp->fwrite($line); + $temp->rewind(); + $read = $temp->fgets(); + var_dump($line === $read); -$temp->rewind(); -$read = $temp->fgets(); -var_dump($line === $read); - -$temp->rewind(); -$temp->setFlags(SplFileObject::DROP_NEW_LINE); -$read = $temp->fgets(); -var_dump($line === $read); + $temp->rewind(); + $temp->setFlags(SplFileObject::DROP_NEW_LINE); + $read = $temp->fgets(); + var_dump($line === $read); +} ?> --EXPECT-- bool(true) bool(true) +bool(true) +bool(true) From c5ee4700ca57364826d629470e71f9a6ff9c1462 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Tue, 13 Apr 2021 11:59:04 +0200 Subject: [PATCH 4/6] Only detect CRLF and LF as line endings, but not CR --- ext/spl/spl_directory.c | 7 +++---- ext/spl/tests/bug80933.phpt | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c index f679c35cacfe0..9324389c2807c 100644 --- a/ext/spl/spl_directory.c +++ b/ext/spl/spl_directory.c @@ -2046,13 +2046,12 @@ static int spl_filesystem_file_read(spl_filesystem_object *intern, int silent) / intern->u.file.current_line_len = 0; } else { if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_DROP_NEW_LINE)) { - zend_string *zbuf = zend_string_init(buf, line_len, 0); - const char *eol = php_stream_locate_eol(intern->u.file.stream, zbuf); + char breaks[] = "\r\n"; + const char *eol = memchr(buf, '\n', line_len); if (eol != NULL) { - line_len = eol - ZSTR_VAL(zbuf); + line_len = (eol > buf && *(eol - 1) == '\r') ? eol - buf - 1 : eol - buf; buf[line_len] = '\0'; } - zend_string_release(zbuf); } intern->u.file.current_line = buf; diff --git a/ext/spl/tests/bug80933.phpt b/ext/spl/tests/bug80933.phpt index 543d4e0365053..78e94aba409b7 100644 --- a/ext/spl/tests/bug80933.phpt +++ b/ext/spl/tests/bug80933.phpt @@ -12,7 +12,7 @@ foreach ($lines as $line) { $temp->rewind(); $read = $temp->fgets(); - var_dump($line === $read); + var_dump($line === $read); $temp->rewind(); $temp->setFlags(SplFileObject::DROP_NEW_LINE); From 6d7afd87a17213e2193ae3db5b6c3d9876bd621b Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Tue, 13 Apr 2021 12:11:54 +0200 Subject: [PATCH 5/6] Drop unused variable --- ext/spl/spl_directory.c | 1 - 1 file changed, 1 deletion(-) diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c index 9324389c2807c..d2b581d224d6c 100644 --- a/ext/spl/spl_directory.c +++ b/ext/spl/spl_directory.c @@ -2046,7 +2046,6 @@ static int spl_filesystem_file_read(spl_filesystem_object *intern, int silent) / intern->u.file.current_line_len = 0; } else { if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_DROP_NEW_LINE)) { - char breaks[] = "\r\n"; const char *eol = memchr(buf, '\n', line_len); if (eol != NULL) { line_len = (eol > buf && *(eol - 1) == '\r') ? eol - buf - 1 : eol - buf; From a81f73b975cac4123168f9fbef3d87b57b82c824 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Tue, 13 Apr 2021 12:35:10 +0200 Subject: [PATCH 6/6] We only need to deal with a newline at the end Co-authored-by: Nikita Popov --- ext/spl/spl_directory.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c index d2b581d224d6c..202a815e82c77 100644 --- a/ext/spl/spl_directory.c +++ b/ext/spl/spl_directory.c @@ -2046,9 +2046,11 @@ static int spl_filesystem_file_read(spl_filesystem_object *intern, int silent) / intern->u.file.current_line_len = 0; } else { if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_DROP_NEW_LINE)) { - const char *eol = memchr(buf, '\n', line_len); - if (eol != NULL) { - line_len = (eol > buf && *(eol - 1) == '\r') ? eol - buf - 1 : eol - buf; + if (line_len > 0 && buf[line_len - 1] == '\n') { + line_len--; + if (line_len > 0 && buf[line_len - 1] == '\r') { + line_len--; + } buf[line_len] = '\0'; } }