Download directory as zip while an inner file is being overwritten causes the download to be corrupted

[jvillafanez]

Steps to reproduce

1. Create a folder and upload several files on it
2. Overwrite one of those files with a big one
3. While the file is being uploaded / overwritten, download the parent folder as a zip

Expected behaviour

A locked exception should be thrown

Actual behaviour

A corrupted zip file is downloaded.

Server configuration

Operating system: ubuntu 14.04

Web server: apache 2.4.7

Database: mysql

PHP version: 5.5.9

ownCloud version: 8.1 beta 2 (daily) Build:2015-06-12T03:13:02+00:00 cda9685

Updated from an older ownCloud or fresh install: fresh install

[PVince81]

Not sure what to do in this case.

Either the zip process must skip the file because it's locked, or abort completely with a locked exception + 423 status code.

[PVince81]

Abort is probably the best

[PVince81]

Hmmm, this shouldn't happen, the zip related code uses View::fopen here https://github.com/owncloud/core/blob/master/lib/private/files.php#L156 which should try and lock the file.

Maybe the exception got caught and the error page got appended into the zip file ?

[PVince81]

Zip files are created on the fly and streamed back to the client, while adding each file one by one.
While it is possible to abort the stream by closing the connection, the client will still believe that the file is correct. For example Chrome does not signal an error.

Additionally since we cannot predict the response length, we cannot pre-set the "Content-Length" header that might hint clients that pieces of the file is missing.

So the aborting approach seems quite tricky.

Let's try another approach: before downloading the folder, set a share lock on the folder. This should prevent files to be uploaded there while the zip file is created. Note that this could take a while, so the folder might be locked for a long time.
Also, if someone was already uploading into the folder, the "download as zip" action will fail with "Locked" error message.
I'll have a go with this solution.

CC @icewind1991

[PVince81]

A quick discussion with @icewind1991 pointed me at rather trying to set a shared lock on every file inside the folder recursively. Other ways would not properly prevent people to change contents during the download.

I could even make it progressive: first set a shared lock on ALL files inside. And then, as the files are delivered one by one, unlock them one by one to make them available to others.

[jvillafanez]

That's exactly what I was thinking. It looks the best approach 👏

If by chance we can't get the lock of any file, we'll just need to unlock all the locks and throw the locked exception

[PVince81]

Yes. The way it currently works is that if any exception happens in the middle, any lock will automatically be freed a the end of the PHP process (shutdown handler), so no additional code is required for such cases.

[ynott]

This problem has also happened while 'getPath' is returned "" string.
https://github.com/owncloud/core/blob/master/apps/files_sharing/lib/controllers/sharecontroller.php#L258

I will make another issue ticket.

[ynott]

add another issue ticket.
Download user's directory&files as zip while missing fileid is called by URLlink.
#17004

[PVince81]

Steps for the implementation:

• write unit tests for the zip code path (if even possible).
• refactor zip download handling, splitting from the single file would be nicer
• add the locking code

[DeepDiver1975]

This is a known issue from the very first day when zip download was introduced - not regression - nothing we can do short hand -> 8.2

[PVince81]

Well, the locking is something new, a new situation where the zip file can be corrupted and it might happen more often.

But anyway, I have conflicting views regarding this static Files class which we'll want to kill eventually, so it's best to defer this.

[DeepDiver1975]

Let's see if we can add locking once we have #19318 in place

[DeepDiver1975]

Let's see if we can add locking once we have #19318 in place

This is merged now - we can now integrate locking on the file list.