Infinite loop in stream_read() for invalid/corrupt chunks

[mdusher]

Description

We've experienced an edge case where stream_read() will enter an infinite loop when one of the chunks of a web upload is invalid/corrupt as it never reads any more data, so stays in the same position for reading indefinitely.

Related Issue

• Fixes Infinite loop in AssemblyStream.php's stream_read() #34488

Motivation and Context

Infinite loops are bad and it is holding files open.

How Has This Been Tested?

This has been tested in our test environment which is a replica of our production environment (see original issue).

The process I used for testing this was:

• Open a window that has the ownCloud "uploads" directory of the user you are using (eg. /path/to/datadirectory/michael.usher@aarnet.edu.au/uploads)
• Clear out the directory of any files/directories (rm -rf web-file-upload-*) - this step is just to make step 4 work when we need it
• Get your "how to corrupt an upload command" ready (don't hit enter on it): sudo -u www-data sh -c 'cd web-file-upload-* && truncate -s 0 $(ls -1 | tail -n1)'
• Login to ownCloud as your chosen user
• Start uploading a file via the web interface
• Once it reaches 100%, wait for it to change to "Processing files..." then press enter on your "how to corrupt an upload" command (it essentially replaces it with a 0 byte file)
• Watch your logs for the never ending rewinddir() errors (not sure if this is just a symptom of our storage, so alternatively - find the pid of the php process for the upload and run strace -s 1024 -p <pid> and you'll see it constantly trying to open the same file over and over.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:

[mdusher]

This is just an idea on how to resolve this issue, it appears to work as expected (throws an exception which is then reported to the user in a friendly way).

[codecov]

Codecov Report

Merging #34489 into master will decrease coverage by <.01%.
The diff coverage is 44.44%.

@@             Coverage Diff              @@
##             master   #34489      +/-   ##
============================================
- Coverage      65.1%    65.1%   -0.01%     
- Complexity    18338    18342       +4     
============================================
  Files          1199     1199              
  Lines         69629    69638       +9     
  Branches       1283     1283              
============================================
+ Hits          45335    45339       +4     
- Misses        23920    23925       +5     
  Partials        374      374

Flag	Coverage Δ	Complexity Δ
#javascript	53.1% <ø> (ø)	0 <ø> (ø)	⬇️
#phpunit	66.49% <44.44%> (-0.01%)	18342 <0> (+4)

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/lib/Upload/AssemblyStream.php	72.34% <44.44%> (-2.96%)	36 <0> (+4)

---

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 406498a...90594ca. Read the comment docs.

[codecov]

Codecov Report

Merging #34489 into master will decrease coverage by <.01%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master   #34489      +/-   ##
============================================
- Coverage     65.11%    65.1%   -0.01%     
- Complexity    18338    18341       +3     
============================================
  Files          1199     1199              
  Lines         69629    69635       +6     
  Branches       1283     1283              
============================================
+ Hits          45336    45339       +3     
- Misses        23919    23922       +3     
  Partials        374      374

Flag	Coverage Δ	Complexity Δ
#javascript	53.1% <ø> (ø)	0 <ø> (ø)	⬇️
#phpunit	66.49% <66.66%> (-0.01%)	18341 <0> (+3)

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/lib/Upload/AssemblyStream.php	74.72% <66.66%> (-0.57%)	35 <0> (+3)
apps/files_trashbin/lib/Expiration.php	96.55% <0%> (-1.73%)	29% <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 c7bfdea...8f3dfb2. Read the comment docs.

[PVince81]

👍 makes sense, thanks

[PVince81]

@labkode FYI as you've observed something similar recently

[PVince81]

stable10: #34558