Fix/stream seek negative position

[marc-mabe]

fixes #23905

This is targeting 8.6 even if this bug exists on 8.4 as well but #21433 conflicts and targets 8.6.
Please tell me if I should target PHP-8.4 instead.

[marc-mabe]

Ping @iliaal @Girgias @ndossche as you where working/reviewing #21433

[ndossche]

Unless I'm missing something, returning -1 is enough without setting *newoffs.
We had a similar issue in zlib before: 2709ebc

[marc-mabe]

@ndossche I created to deeper test script comparing different streams and behavior (see #23905 (comment)).
Id I take POSIX as the source of truth this PR is still missing some cases -> will need to improve it.

[marc-mabe]

@ndossche I reworked the branch and updated it to latest PHP-8.6.

What has been changed since last?

Fix failed seek position on php://memory

• The failure branches no longer write the new offset at all, so both the internal and the reported position stay untouched.
• php://temp is affected as well while its data is still in memory, as it forwards seeks to an inner php://memory stream. NEWS and the test title now mention it.
• The test now covers php://memory, php://temp, php://temp/maxmemory:0 and the same three behind php://filter/string.rot13/..., each with SEEK_SET, SEEK_CUR and SEEK_END before the start and past the end.
• Removed the redundant eof/fatal_error resets from the memory seek handler, as php_stream_seek() already does that on success. php_stream_temp_seek() no longer reports -1 when it has no inner stream.

New commit: Fix failed seek discarding the read buffer
While extending the blob tests I found that php_stream_seek() discarded the read buffer even when the seek failed. A buffered stream has usually read ahead, so after a failed seek the next read skipped the buffered data and a write landed where the stream had read ahead to, while ftell() still reported the old position. This also affects plain files and user stream wrappers:

$fp = fopen($file, 'r+');    // "hello world"
fread($fp, 5);               // "hello"
fseek($fp, -12, SEEK_END);   // -1, ftell() is still 5
fread($fp, 5);               // before: "", now: " worl"

A failed seek now returns early and keeps the position, the read buffer and the filter state. The new test ext/standard/tests/streams/stream_seek_failure_keeps_buffer.phpt covers a plain file and a user stream wrapper.

Fix failed seek position on SQLite3 / PDO SQLite blob streams

• The failure branches no longer write the new offset. Before, after a partial (buffered) read, a failed seek still moved ftell() to the position the blob had read ahead to.
• A negative SEEK_SET offset is now rejected explicitly instead of relying on the size_t cast.
• The tests now cover read-only and read-write blobs, all whence values before the start and past the end, failed seeks after a partial read, and a write after a failed seek past the end. That write now happens at the unchanged position instead of failing with "It is not possible to increase the size of a BLOB".
  • This is because the failed seek before does not change the position anymore. Writing still works until the end.

Not addressed / possible follow-ups
These came up while analysing seek behaviour across stream types. They are out of scope for this PR, so I left them for follow-ups:

• Assertion failure _php_stream_seek at streams/streams.c #21700 looks related, as it hits the same stream->position >= 0 assertion, but it isn't fixed by this PR. The cause is different: php_stream_seek() converts SEEK_CUR into an absolute SEEK_SET target without checking that the target isn't negative (only an explicit SEEK_SET is checked). So a user wrapper receives SEEK_SET -1, accepts it, and stream_tell() then reports -1. A possible follow-up is to reject negative SEEK_CUR targets in php_stream_seek().
• Phar refuses seeks past the end of an entry without a warning, while POSIX allows them, and a write then fills the gap with zeros. A following read or write correctly happens at the unchanged position. Allowing the seek would require phar_stream_read() to stop at the end of the entry. It currently relies on the seek refusal: with a position past the end, uncompressed_filesize - position underflows. It also computes EOF with ==, which sets EOF early after a read that ends exactly at the end of the entry.
• zlib: gzseek() doesn't support SEEK_END. In write mode, backward seeks are impossible, and a forward seek past the end writes the zeros straight away rather than on the next write. These are mostly zlib limitations.
• zip: php_zip_ops_seek() still sets the new offset from zip_ftell() when zip_fseek() fails. Now that a failed seek keeps the read buffer, this would let the position jump ahead while the buffered data is kept, at least with a libzip where entries are seekable. I couldn't reproduce it locally because zip entries weren't seekable there, so the seek is emulated.
• No error reporting: apart from zlib's SEEK_END, a failed seek returns -1 without a warning or reason.

[ndossche]

I will have a read through this tomorrow.

[ndossche]

Looks mostly fine. I wonder what some of the reasoning behind the old code was, really bizarre...

[ndossche]

Why did you change the behaviour of this case?

[marc-mabe]

This is needed because a failed seek discarded the read buffer, although the stream did not move. A buffered stream has usually read ahead, so the next read or write then continued from the read-ahead position while ftell() still reported the old one. This affects every buffered stream, e.g. plain files and user wrappers, not only the blob streams:

file_put_contents($f, 'hello world');
$fp = fopen($f, 'r+');
fread($fp, 5);               // "hello" (the stream has read ahead to the end)
fseek($fp, -12, SEEK_END);   // -1, ftell() is still 5
// on reading
fread($fp, 5);               // before: "" (" world" skipped), this change: " worl"
// or writing:
fwrite($fp, '!');            // before: "hello world!", this change: "hello!world"

With a user wrapper whose stream_seek() refuses the target, feof() returned true after the failed seek, and the next read returned "" although data remained. Stream filters were also told about the seek (php_stream_filters_seek_all()), although it never happened.

After the change, a failed seek returns early and keeps the position, the read buffer and the filter state. Successful seeks are unchanged. The SQLite blob fixes depend on this: their streams are buffered, so a failed seek after a partial read would otherwise still continue from the read-ahead position.

[bukka]

It looks good. Just minor things really.

I think it needs to target master only. There are some semantic changes and that's just given when you need to update UPGRADING / UPGRADING.INTERNALS which cannot be done during RC or stable version.

[bukka]

hmm this should be probably kept in userspace.c for case where seek is successful but tell is missing or return non int (missing is already a warning I think but we should still reset it there).

[marc-mabe]

Good catch, thanks! In that case stream_seek() already moved the user stream, so keeping the buffer would mix the stale buffered data with the new position.

I have changed it so php_userstreamop_seek() now discards the read buffer and clears eof/fatal_error itself when stream_seek() succeeds but stream_tell() is missing or doesn't return an int. The seek still fails, and ftell() keeps reporting the old position, since the new one is unknown.

stream_seek_failure_keeps_buffer.phpt covers it.

[bukka]

I guess this can be deleted too.

[marc-mabe]

Not yet.

php_stream_write_buffer() (before a write after a read) and php_stream_cast() call ops->seek directly to move back to the logical position, without going through php_stream_seek(). In these cases, the handler's own reset is the only thing that clears eof.

The blob read handler sets eof as soon as it reaches the end of the blob, while data is still buffered. Without the reset in the handler, a write or a cast after a partial read leaves feof() returning true in the middle of the blob.

Streams whose handlers never reset eof already lose data this way on master. With a read filter, which reads ahead until eof, fread() returns "" after such a write on a plain file.

I'll fix that in a follow up PR in the stream layer, which than removes these resets from the memory, temp and SQLite seek handler.

[marc-mabe]

@bukka I updated the PR

• Rebased onto master and retargeted the PR to master.
• This now also addresses the userspace.c - see inline comment.