Skip to content

Commit 08dad09

Browse files
iliaaliliaal
authored
Fix GH-8561, GH-8562, GH-8563, GH-8564: SplFileObject iterator desync (#21679)
* Fix GH-8562: SplFileObject::current() returns wrong value after next() SplFileObject::next() without READ_AHEAD cleared the cached line and incremented current_line_num but didn't advance the stream. When called without a preceding current() (e.g. rewind() then next()), the stream position stayed put, so the subsequent current() read stale data. Read a line to advance the stream when next() is called with no cached line. Closes GH-8562 * Fix GH-8561: SplFileObject key()/current() desync after fgets() fgets() read a line into the cache and incremented the line counter, but left the cached line in place. The subsequent current() returned the stale cached value instead of reading the next line from the stream's actual position. Clear the cached line after fgets() copies it to the return value. This forces current() to re-read from the stream, which has already advanced past the fgets'd line. Closes GH-8561 * Fix GH-8563, GH-8564: SplFileObject EOF handling for seek() and next() spl_filesystem_file_read_ex() treated a NULL buffer from php_stream_get_line as a successful read of an empty line, creating a phantom cached line at EOF. This caused seek() to give inconsistent results between SplFileObject and SplTempFileObject (GH-8563), and next() to increment the line counter indefinitely past EOF (GH-8564). Return FAILURE when php_stream_get_line returns NULL, matching the behavior of the existing php_stream_eof check. In seek(), break out of the loop on EOF instead of returning, so the post-loop cleanup runs consistently. In next(), return early at EOF without incrementing. Make __toString() return empty string at EOF instead of throwing. Closes GH-8563 Closes GH-8564 * Refine fgets() to reuse cached line when present When current() reads a line into the cache without advancing line_num, a subsequent fgets() would re-read the stream and return the next line, skipping the cached one and leaving key() out of sync with current() for the rest of the iteration. Use the cached line if present; otherwise read a fresh line. Either way, advance line_num by one.
1 parent 0d30d82 commit 08dad09

File tree

10 files changed

+144
-27
lines changed

10 files changed

+144
-27
lines changed

‎ext/spl/spl_directory.c‎

Lines changed: 33 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1811,21 +1811,24 @@ static zend_result spl_filesystem_file_read_ex(spl_filesystem_object *intern, bo
18111811
}
18121812

18131813
if (!buf) {
1814-
intern->u.file.current_line = ZSTR_EMPTY_ALLOC();
1815-
} else {
1816-
if (!csv && SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_DROP_NEW_LINE)) {
1817-
if (line_len > 0 && buf[line_len - 1] == '\n') {
1814+
if (!silent) {
1815+
spl_filesystem_file_cannot_read(intern);
1816+
}
1817+
return FAILURE;
1818+
}
1819+
1820+
if (!csv && SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_DROP_NEW_LINE)) {
1821+
if (line_len > 0 && buf[line_len - 1] == '\n') {
1822+
line_len--;
1823+
if (line_len > 0 && buf[line_len - 1] == '\r') {
18181824
line_len--;
1819-
if (line_len > 0 && buf[line_len - 1] == '\r') {
1820-
line_len--;
1821-
}
1822-
buf[line_len] = '\0';
18231825
}
1826+
buf[line_len] = '\0';
18241827
}
1825-
1826-
intern->u.file.current_line = zend_string_init(buf, line_len, /* persistent */ false);
1827-
efree(buf);
18281828
}
1829+
1830+
intern->u.file.current_line = zend_string_init(buf, line_len, /* persistent */ false);
1831+
efree(buf);
18291832
intern->u.file.current_line_num += line_add;
18301833

18311834
return SUCCESS;
@@ -2091,10 +2094,17 @@ PHP_METHOD(SplFileObject, fgets)
20912094

20922095
CHECK_SPL_FILE_OBJECT_IS_INITIALIZED(intern);
20932096

2094-
if (spl_filesystem_file_read_ex(intern, /* silent */ false, /* line_add */ 1, /* csv */ false) == FAILURE) {
2095-
RETURN_THROWS();
2097+
if (intern->u.file.current_line) {
2098+
RETVAL_STR_COPY(intern->u.file.current_line);
2099+
spl_filesystem_file_free_line(intern);
2100+
intern->u.file.current_line_num++;
2101+
} else {
2102+
if (spl_filesystem_file_read_ex(intern, /* silent */ false, /* line_add */ 1, /* csv */ false) == FAILURE) {
2103+
RETURN_THROWS();
2104+
}
2105+
RETVAL_STR_COPY(intern->u.file.current_line);
2106+
spl_filesystem_file_free_line(intern);
20962107
}
2097-
RETURN_STR_COPY(intern->u.file.current_line);
20982108
} /* }}} */
20992109

21002110
/* {{{ Return current line from file */
@@ -2140,6 +2150,12 @@ PHP_METHOD(SplFileObject, next)
21402150

21412151
ZEND_PARSE_PARAMETERS_NONE();
21422152

2153+
if (!intern->u.file.current_line && Z_ISUNDEF(intern->u.file.current_zval)) {
2154+
if (spl_filesystem_file_read_line(ZEND_THIS, intern, true) == FAILURE) {
2155+
return;
2156+
}
2157+
}
2158+
21432159
spl_filesystem_file_free_line(intern);
21442160
if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_AHEAD)) {
21452161
spl_filesystem_file_read_line(ZEND_THIS, intern, true);
@@ -2627,7 +2643,7 @@ PHP_METHOD(SplFileObject, seek)
26272643

26282644
for (i = 0; i < line_pos; i++) {
26292645
if (spl_filesystem_file_read_line(ZEND_THIS, intern, true) == FAILURE) {
2630-
return;
2646+
break;
26312647
}
26322648
}
26332649
if (line_pos > 0 && !SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_AHEAD)) {
@@ -2646,9 +2662,8 @@ PHP_METHOD(SplFileObject, __toString)
26462662

26472663
if (!intern->u.file.current_line) {
26482664
ZEND_ASSERT(Z_ISUNDEF(intern->u.file.current_zval));
2649-
zend_result result = spl_filesystem_file_read_line(ZEND_THIS, intern, false);
2650-
if (UNEXPECTED(result != SUCCESS)) {
2651-
RETURN_THROWS();
2665+
if (spl_filesystem_file_read_line(ZEND_THIS, intern, true) == FAILURE) {
2666+
RETURN_EMPTY_STRING();
26522667
}
26532668
}
26542669

‎ext/spl/tests/SplFileObject/SplFileObject_key_error001.phpt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,5 +18,5 @@ var_dump($s->key());
1818
var_dump($s->valid());
1919
?>
2020
--EXPECT--
21-
int(14)
21+
int(12)
2222
bool(false)

‎ext/spl/tests/SplFileObject/SplFileObject_key_error002.phpt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,5 +18,5 @@ var_dump($s->key());
1818
var_dump($s->valid());
1919
?>
2020
--EXPECT--
21-
int(13)
21+
int(12)
2222
bool(false)

‎ext/spl/tests/SplFileObject/bug81477.phpt‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ string(8) "baz,bat
2121
"
2222
string(10) "more,data
2323
"
24-
string(0) ""
2524
--CLEAN--
2625
<?php
2726
@unlink(__DIR__ . '/bug81477.csv');

‎ext/spl/tests/SplFileObject/fgetcsv_blank_file.phpt‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,5 @@ $file->rewind();
1818
var_dump($file->fgetcsv());
1919
?>
2020
--EXPECT--
21-
array(1) {
22-
[0]=>
23-
NULL
24-
}
21+
bool(false)
2522
bool(false)

‎ext/spl/tests/SplFileObject/gh8561.phpt‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
--TEST--
2+
GH-8561 (SplFileObject: key() and current() unsynchronized after fgets())
3+
--FILE--
4+
<?php
5+
$file = new SplTempFileObject();
6+
for ($i = 0; $i < 5; $i++) {
7+
$file->fwrite("line {$i}" . PHP_EOL);
8+
}
9+
10+
// Case 1: rewind + fgets, then key/current
11+
$file->rewind();
12+
$file->fgets();
13+
echo "After rewind+fgets: key=" . $file->key() . " current=" . trim($file->current()) . "\n";
14+
15+
// Case 2: multiple fgets
16+
$file->rewind();
17+
$file->fgets();
18+
$file->fgets();
19+
echo "After rewind+fgets+fgets: key=" . $file->key() . " current=" . trim($file->current()) . "\n";
20+
21+
// Case 3: current then fgets
22+
$file->rewind();
23+
$file->current();
24+
$file->fgets();
25+
echo "After current+fgets: key=" . $file->key() . " current=" . trim($file->current()) . "\n";
26+
?>
27+
--EXPECT--
28+
After rewind+fgets: key=1 current=line 1
29+
After rewind+fgets+fgets: key=2 current=line 2
30+
After current+fgets: key=1 current=line 1

‎ext/spl/tests/SplFileObject/gh8563.phpt‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
--TEST--
2+
GH-8563 (Different results for seek() on SplFileObject and SplTempFileObject)
3+
--FILE--
4+
<?php
5+
$path = __DIR__ . '/gh8563.txt';
6+
$file_01 = new SplFileObject($path, 'w+');
7+
$file_02 = new SplTempFileObject();
8+
9+
for ($i = 0; $i < 5; $i++) {
10+
$file_01->fwrite("line {$i}" . PHP_EOL);
11+
$file_02->fwrite("line {$i}" . PHP_EOL);
12+
}
13+
14+
$file_01->rewind();
15+
$file_02->rewind();
16+
17+
$file_01->seek(10);
18+
$file_02->seek(10);
19+
20+
echo 'SplFileObject: ' . $file_01->key() . "\n";
21+
echo 'SplTempFileObject: ' . $file_02->key() . "\n";
22+
?>
23+
--CLEAN--
24+
<?php
25+
unlink(__DIR__ . '/gh8563.txt');
26+
?>
27+
--EXPECT--
28+
SplFileObject: 5
29+
SplTempFileObject: 5

‎ext/spl/tests/SplFileObject/gh8564.phpt‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
--TEST--
2+
GH-8564 (SplFileObject: next() moves to nonexistent indexes)
3+
--FILE--
4+
<?php
5+
$file = new SplTempFileObject();
6+
for ($i = 0; $i < 5; $i++) {
7+
$file->fwrite("line {$i}" . PHP_EOL);
8+
}
9+
10+
$file->rewind();
11+
for ($i = 0; $i < 10; $file->next(), $i++);
12+
echo "next() 10x: key=" . $file->key() . " valid=" . var_export($file->valid(), true) . "\n";
13+
14+
$file->rewind();
15+
$file->seek(10);
16+
echo "seek(10): key=" . $file->key() . " valid=" . var_export($file->valid(), true) . "\n";
17+
?>
18+
--EXPECT--
19+
next() 10x: key=5 valid=false
20+
seek(10): key=5 valid=false

‎ext/spl/tests/gh13685.phpt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,9 @@ try {
4444
string(14) ""A", "B", "C"
4545
"
4646
string(13) ""D", "E", "F""
47-
Cannot read from file php://temp
47+
string(0) ""
4848
--- Use csv control ---
4949
string(14) ""A", "B", "C"
5050
"
5151
string(13) ""D", "E", "F""
52-
Cannot read from file php://temp
52+
string(0) ""

‎ext/spl/tests/gh8562.phpt‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
--TEST--
2+
GH-8562 (SplFileObject::current() returns wrong result after call to next())
3+
--FILE--
4+
<?php
5+
$file = new SplTempFileObject();
6+
for ($i = 0; $i < 5; $i++) {
7+
$file->fwrite("line {$i}" . PHP_EOL);
8+
}
9+
10+
$file->rewind();
11+
$file->next();
12+
echo "After rewind+next: key=" . $file->key() . " current=" . trim($file->current()) . "\n";
13+
14+
$file->rewind();
15+
$file->next();
16+
$file->next();
17+
echo "After rewind+next+next: key=" . $file->key() . " current=" . trim($file->current()) . "\n";
18+
19+
$file->rewind();
20+
$file->current();
21+
$file->next();
22+
echo "After current+next: key=" . $file->key() . " current=" . trim($file->current()) . "\n";
23+
?>
24+
--EXPECT--
25+
After rewind+next: key=1 current=line 1
26+
After rewind+next+next: key=2 current=line 2
27+
After current+next: key=1 current=line 1

0 commit comments

Comments
 (0)