Skip to content

Conversation

@Flashwade1990
Copy link

No description provided.

@cmb69 cmb69 added the Bug label Nov 29, 2021
@cmb69
Copy link
Member

cmb69 commented Nov 30, 2021

Thank you for the PR! However, it seems this is rather fixing the symptoms than curing the disease, namely that normal reading frees the line before checking for EOF

spl_filesystem_file_free_line(intern);
if (php_stream_eof(intern->u.file.stream)) {

but CSV reading does not
if (php_stream_eof(intern->u.file.stream)) {

What about the following patch instead:

 ext/spl/spl_directory.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c
index c7dd8b4721..045aad5bc1 100644
--- a/ext/spl/spl_directory.c
+++ b/ext/spl/spl_directory.c
@@ -1939,6 +1939,8 @@ static int spl_filesystem_file_read_line_ex(zval * this_ptr, spl_filesystem_obje
 
 	/* 1) use fgetcsv? 2) overloaded call the function, 3) do it directly */
 	if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_CSV) || intern->u.file.func_getCurr->common.scope != spl_ce_SplFileObject) {
+		spl_filesystem_file_free_line(intern);
+
 		if (php_stream_eof(intern->u.file.stream)) {
 			if (!silent) {
 				zend_throw_exception_ex(spl_ce_RuntimeException, 0, "Cannot read from file %s", intern->file_name);

@Flashwade1990
Copy link
Author

Thank you for the PR! However, it seems this is rather fixing the symptoms than curing the disease, namely that normal reading frees the line before checking for EOF

spl_filesystem_file_free_line(intern);
if (php_stream_eof(intern->u.file.stream)) {

but CSV reading does not

if (php_stream_eof(intern->u.file.stream)) {

What about the following patch instead:

 ext/spl/spl_directory.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c
index c7dd8b4721..045aad5bc1 100644
--- a/ext/spl/spl_directory.c
+++ b/ext/spl/spl_directory.c
@@ -1939,6 +1939,8 @@ static int spl_filesystem_file_read_line_ex(zval * this_ptr, spl_filesystem_obje
 
 	/* 1) use fgetcsv? 2) overloaded call the function, 3) do it directly */
 	if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_CSV) || intern->u.file.func_getCurr->common.scope != spl_ce_SplFileObject) {
+		spl_filesystem_file_free_line(intern);
+
 		if (php_stream_eof(intern->u.file.stream)) {
 			if (!silent) {
 				zend_throw_exception_ex(spl_ce_RuntimeException, 0, "Cannot read from file %s", intern->file_name);

Thank you for the PR! However, it seems this is rather fixing the symptoms than curing the disease, namely that normal reading frees the line before checking for EOF

spl_filesystem_file_free_line(intern);
if (php_stream_eof(intern->u.file.stream)) {

but CSV reading does not

if (php_stream_eof(intern->u.file.stream)) {

What about the following patch instead:

 ext/spl/spl_directory.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c
index c7dd8b4721..045aad5bc1 100644
--- a/ext/spl/spl_directory.c
+++ b/ext/spl/spl_directory.c
@@ -1939,6 +1939,8 @@ static int spl_filesystem_file_read_line_ex(zval * this_ptr, spl_filesystem_obje
 
 	/* 1) use fgetcsv? 2) overloaded call the function, 3) do it directly */
 	if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_CSV) || intern->u.file.func_getCurr->common.scope != spl_ce_SplFileObject) {
+		spl_filesystem_file_free_line(intern);
+
 		if (php_stream_eof(intern->u.file.stream)) {
 			if (!silent) {
 				zend_throw_exception_ex(spl_ce_RuntimeException, 0, "Cannot read from file %s", intern->file_name);

Thank you for checking @cmb69, the suggested approach is much better. I update the PR.

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This looks good to me, but I'd like a third pair of eyes reviewing this (after all, it's SPL …)

@cmb69 cmb69 closed this in daf79e2 Dec 6, 2021
@cmb69
Copy link
Member

cmb69 commented Dec 6, 2021

No further review, so I double-checked myself, and merged! Thanks again for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants