Skip to content

Commit

Permalink
Fix GH-8273: SplFileObject: key() returns wrong value
Browse files Browse the repository at this point in the history
  • Loading branch information
Girgias committed Apr 23, 2022
1 parent bfad99f commit 660ef91
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 6 deletions.
1 change: 1 addition & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ PHP NEWS
- SPL:
. Fixed bug GH-8366 (ArrayIterator may leak when calling __construct()).
(cmb)
. Fixed bug GH-8273 (SplFileObject: key() returns wrong value). (Girgias)

- Streams:
. Fixed php://temp does not preserve file-position when switched to temporary
Expand Down
17 changes: 11 additions & 6 deletions ext/spl/spl_directory.c
Original file line number Diff line number Diff line change
Expand Up @@ -1864,11 +1864,10 @@ static int spl_filesystem_object_cast(zend_object *readobj, zval *writeobj, int
}
/* }}} */

static int spl_filesystem_file_read(spl_filesystem_object *intern, int silent) /* {{{ */
static zend_result spl_filesystem_file_read_ex(spl_filesystem_object *intern, bool silent, zend_long line_add) /* {{{ */
{
char *buf;
size_t line_len = 0;
zend_long line_add = (intern->u.file.current_line || !Z_ISUNDEF(intern->u.file.current_zval)) ? 1 : 0;

spl_filesystem_file_free_line(intern);

Expand Down Expand Up @@ -1913,7 +1912,13 @@ static int spl_filesystem_file_read(spl_filesystem_object *intern, int silent) /
return SUCCESS;
} /* }}} */

static int spl_filesystem_file_read_csv(spl_filesystem_object *intern, char delimiter, char enclosure, int escape, zval *return_value) /* {{{ */
static inline zend_result spl_filesystem_file_read(spl_filesystem_object *intern, bool silent)
{
zend_long line_add = (intern->u.file.current_line) ? 1 : 0;
return spl_filesystem_file_read_ex(intern, silent, line_add);
}

static zend_result spl_filesystem_file_read_csv(spl_filesystem_object *intern, char delimiter, char enclosure, int escape, zval *return_value) /* {{{ */
{
do {
int ret = spl_filesystem_file_read(intern, 1);
Expand Down Expand Up @@ -2176,7 +2181,7 @@ PHP_METHOD(SplFileObject, fgets)

CHECK_SPL_FILE_OBJECT_IS_INITIALIZED(intern);

if (spl_filesystem_file_read(intern, 0) == FAILURE) {
if (spl_filesystem_file_read_ex(intern, /* silent */ false, /* line_add */ 1) == FAILURE) {
RETURN_THROWS();
}
RETURN_STRINGL(intern->u.file.current_line, intern->u.file.current_line_len);
Expand Down Expand Up @@ -2214,8 +2219,8 @@ PHP_METHOD(SplFileObject, key)
RETURN_THROWS();
}

/* Do not read the next line to support correct counting with fgetc()
if (!intern->current_line) {
/* Do not read the next line to support correct counting with fgetc()
if (!intern->u.file.current_line) {
spl_filesystem_file_read_line(ZEND_THIS, intern, 1);
} */
RETURN_LONG(intern->u.file.current_line_num);
Expand Down
39 changes: 39 additions & 0 deletions ext/spl/tests/SplFileObject_key_fgets_and seek.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
--TEST--
SplFileObject verify interactions between seeking, getting the key and fgets
--FILE--
<?php

$file = new SplTempFileObject();

for ($i = 0; $i < 100; $i++) {
$file->fwrite("Foo $i\n");
}

$file->seek(50);

var_dump(
['line' => $file->key(), 'contents' => trim($file->fgets())],
['line' => $file->key(), 'contents' => trim($file->fgets())],
['line' => $file->key(), 'contents' => trim($file->fgets())],
);

?>
--EXPECT--
array(2) {
["line"]=>
int(50)
["contents"]=>
string(6) "Foo 50"
}
array(2) {
["line"]=>
int(51)
["contents"]=>
string(6) "Foo 51"
}
array(2) {
["line"]=>
int(52)
["contents"]=>
string(6) "Foo 52"
}
24 changes: 24 additions & 0 deletions ext/spl/tests/gh8273.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
GH-8273 (SplFileObject: key() returns wrong value)
--FILE--
<?php

$file = new SplTempFileObject();

// write to file
for ($i = 0; $i < 5; $i++) {
$file->fwrite("line {$i}" . PHP_EOL);
}

// read from file
$file->rewind();
while ($file->valid()) {
echo $file->key(), ': ', $file->fgets();
}
?>
--EXPECT--
0: line 0
1: line 1
2: line 2
3: line 3
4: line 4

3 comments on commit 660ef91

@bukka
Copy link
Member

@bukka bukka commented on 660ef91 Apr 23, 2022

Choose a reason for hiding this comment

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

@Girgias I just got a merge conflict when merging from PHP-8.0. Seems like you merged two different versions (separate commits) and forgot to merge the PHP-8.0 before that which is necessary to keep merge up working cleanly. I used PHP-8.1 version here but please double check if it's all good.

@Girgias
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I did the merge correctly >_> by merging 8.0 into 8.1 and then 8.1 into master, but must have made a mistake (or perhaps the NEWS file?), thanks for letting me know however I'll try to make sure the merges are clean

@bukka
Copy link
Member

@bukka bukka commented on 660ef91 Apr 28, 2022

Choose a reason for hiding this comment

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

Sorry for the late reply.

It was because you created another commit somehow: 6186ecd without merging it up. But if it's all looking good, then it's fine.

Please sign in to comment.