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

[alexandre-daubois]

No description provided.

[Girgias]

Looks reasonable

[ndossche]

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.

[ndossche]

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.

[ndossche]

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));
 } /* }}} */
 

[Girgias]

I remember trying to understand and fix a bunch of issues with SplFileObject back in 2021/2022 and from what I recall the main problem is that many things are interconnected in ways that make understanding the internal and user facing impact difficult. And some behaviour, even if buggy, has just been worked around by end-users, so fixing them also causes issues...

I don't know if SplFileObject (and frankly a majority of SPL) is salvageable, generally I would recommend against using it and just using the lower level I/O functions, but I can't deny the convenience of the DirectoryIterators...

[alexandre-daubois]

I pushed a new version. This should fix both cases highlighted here and in the issue without breaking the rest.

[alexandre-daubois]

Rebased on 8.4