Fix GH-19942: avoid infinite loop when using iterator_count() on an empty SplFileObject

[alexandre-daubois]

No description provided.

[Girgias]

Looks reasonable

[nielsdos]

Will this break for non-seekable file streams?

[alexandre-daubois]

Good catch, I added checks to only perform this on seekable streams + a test to cover it.

[pilif]

a heads-up: This isn't just happening for empty files. iterator_count() on any SplFileObject will loop forever, whereas the patch looks suspiciously to me as it's only handling the empty file case:

<?php

file_put_contents('/tmp/file', "a\n");
var_dump(iterator_count(new SplFileObject("/tmp/file", "r")));

Testing it now.

[pilif]

yes. can confirm the path only fixes the empty file case, not the general case. above test-code still loops endlessly with 4b81bba applied.

So while, yes, the patch is probably ok for that super specific case, I don't think it fixes gh-19942 which doesn't specifically talk about empty files.

[nielsdos]

Right indeed.
The root cause is simply misunderstood. The root cause is that PHP_METHOD(SplFileObject, next) only reads data if SPL_FILE_OBJECT_READ_AHEAD is set:

php-src/ext/spl/spl_directory.c

Lines 2209 to 2212 in 80e4278

	if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_AHEAD)) {
	spl_filesystem_file_read_line(ZEND_THIS, intern, true);
	}
	intern->u.file.current_line_num++;

because this isn't set, the current stream offset never advances and therefore EOF is never reached. This causes the infinite loop.

[nielsdos]

This also means btw that calling SplFileObject::valid() after seeking can give erroneous results. I think we should attempt to make a read (which would solve both issues, although I'm not sure if this gives the right results and what consequences are):

diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c
index a769d627a54..b4c643d9f54 100644
--- a/ext/spl/spl_directory.c
+++ b/ext/spl/spl_directory.c
@@ -2118,6 +2118,7 @@ PHP_METHOD(SplFileObject, eof)
 
 	CHECK_SPL_FILE_OBJECT_IS_INITIALIZED(intern);
 
+	// TODO: should this be consistent with valid() ?
 	RETURN_BOOL(php_stream_eof(intern->u.file.stream));
 } /* }}} */
 
@@ -2136,6 +2137,10 @@ PHP_METHOD(SplFileObject, valid)
 	if (!intern->u.file.stream) {
 		RETURN_FALSE;
 	}
+	/* If there is no current line, we might've seeked, so we need to attempt a read. */
+	if (!intern->u.file.current_line && Z_ISUNDEF(intern->u.file.current_zval)) {
+		spl_filesystem_file_read_line(ZEND_THIS, intern, true);
+	}
 	RETURN_BOOL(!php_stream_eof(intern->u.file.stream));
 } /* }}} */