Fix Content is larger than the available bytes in the stream

[dpakach]

Fix for sending response of ranged streams when the content length provided is larger than the total number of bytes remaining in the stream.
In such cases the operation completes successfully but the loop never exits as stream_copy_to_stream() returns negative value.

This PR fixes this by checking the return value of stream_copy_to_stream() and returning when the return value is negative.

[codecov]

Codecov Report

Merging #138 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #138      +/-   ##
============================================
+ Coverage     93.61%   93.64%   +0.03%     
- Complexity      256      257       +1     
============================================
  Files            15       15              
  Lines           830      834       +4     
============================================
+ Hits            777      781       +4     
  Misses           53       53

Impacted Files	Coverage Δ	Complexity Δ
lib/Sapi.php	96.29% <100%> (+0.1%)	40 <0> (+1)	⬆️
lib/functions.php	95.55% <0%> (+0.03%)	0% <0%> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd3958b...943e6c4. Read the comment docs.

[staabm]

Thx for the PR. Did you verify this fixes the reported owncloud problems?

[vfreex]

I would interpret the mismatch between Content-Length and length of content is bug. We should definitely raise an exception (or use assert) if they are not equal rather than silently ignore this.

When user is requesting a larger range than expected, the server need to return HTTP 416 (Range Not Satisfiable) per https://tools.ietf.org/html/rfc7233#section-4.2

[phil-davis]

The previous code never checked the return value of stream_copy_to_stream in any way.
So this "worked" in the previous code. The available/existing bytes in the input stream did get copied to the output stream - they were just less than what Content-Length said. And so "it worked for the user" - they actually received the proper file contents.

The problem is seen when an old version of a file in restored and the old version was shorter in length. When we ask for the file from S3, it tells that the length is the (longer) length of what was the "current" file. But the actual file data is correctly the shorter length of the restored version.

Now the question is what to do when sabre/http gets something like this. IMO for now this PR restores the previous behavior, so we do that for a patch release.

[phil-davis]

This code does fix the CPU loop problem observed in ownCloud tests.

[phil-davis]

IMO for now this PR restores the previous behavior, so we do that for a patch release.

And then someone can think through exactly what exception to throw and whether such a change in behaviour is a patch, minor or major version break.

[staabm]

Merging because it fixes a bc break .. whether this is the correct behavior or not can be discussed and fixed in the next major release

[staabm]

Thank you guys for working on the problem. Very appreciated

[phil-davis]

Thank you guys for working on the problem. Very appreciated

No problem - we use this, so happy to help!

[phil-davis]

sabre/http 5.0.5 has been merged to owncloud core in PR owncloud/core#36490
Looks good, see comment owncloud/files_primary_s3#274 (comment)