Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Premature stream termination when fread returns nothing #19

Closed
Ziyann opened this issue Dec 15, 2023 · 1 comment · Fixed by #20
Closed

Premature stream termination when fread returns nothing #19

Ziyann opened this issue Dec 15, 2023 · 1 comment · Fixed by #20

Comments

@Ziyann
Copy link
Contributor

Ziyann commented Dec 15, 2023

I'm hitting an issue on Nextcloud when zipping files from S3. I've described the situation here: nextcloud/server#42248.

The problem here is that when the stream is a remote file, fread might return an empty string (even if it is a blocking stream, like in this case). This happened in my case, and the downloaded ZIP files contained truncated files.

while (!feof($stream) && $data = fread($stream, self::STREAM_CHUNK_SIZE)) {
  ...
}

So, if fread returns nothing (an empty string), then even if feof is false, indicating that it's not the end of the stream yet, it will still abort reading the file (S3 stream in this case).

If I change it like so:

while (!feof($stream)) {
  $data = fread($stream, self::STREAM_CHUNK_SIZE);
  ...
}

Then my issue is fixed. The behavior was changed here: 22515e3.

One could argue that the underlying stream should be fixed/changed to block as long as necessary instead, but in my view, the library should either handle cases where fread returns nothing while feof is still false (by retrying), or return with an error to the caller, otherwise, as we see, it can silently produce truncated files. Another possible solution would be also passing the source file size (which is usually known by this time), and checking against that.

What are your thoughts about the issue, and my fix/workaround, and how do you think a proper fix (that could be merged) could be organized?

@Ziyann Ziyann changed the title Premature stream termination when fread returns zero Premature stream termination when fread returns nothing Dec 16, 2023
@Ziyann
Copy link
Contributor Author

Ziyann commented Dec 16, 2023

This minimally invasive pull request fixes the issue: #20.
@DeepDiver1975, what do you think about this approach?
Also, the TarStreamer library has the same behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant