prevent 0 byte downloads when storage returns false #19081

Merged
merged 2 commits into from Sep 21, 2015

Projects

None yet

7 participants

@butonic
Member
butonic commented Sep 16, 2015

ref #17220
prevents data loss

@PVince81
Collaborator

Makes sense. A unit test would be nice ๐Ÿ˜„

@icewind1991
Member

Added unit tests, code looks good ๐Ÿ‘

@PVince81
Collaborator

Thanks ๐Ÿ‘

@PVince81 PVince81 added this to the 8.2-current milestone Sep 16, 2015
@butonic
Member
butonic commented Sep 17, 2015

thx!

@DeepDiver1975 DeepDiver1975 commented on the diff Sep 17, 2015
lib/private/connector/sabre/file.php
//throw exception if encryption is disabled but files are still encrypted
try {
- return $this->fileView->fopen(ltrim($this->path, '/'), 'rb');
+ $res = $this->fileView->fopen(ltrim($this->path, '/'), 'rb');
+ if ($res === false) {
+ throw new ServiceUnavailable("Could not open file");
@DeepDiver1975
DeepDiver1975 Sep 17, 2015 Member

ServiceUnavailable? in case the file is not there we should return NotFound

@butonic
butonic Sep 17, 2015 Member

This is a fallback for storages that do not throw an exception when there is a infrastructure problem, eg wnd returns false when DNS is not available. There might be other implementations that swallow this kind of problem, leading to 0 byte downloads.

It depends on what we want the client to do. He did a directory listing and now wants to download the file. ServiceUnavailable will make him try the sync again. I don't know what happens when the file has gone. I fear it will locally delete the file, which is what we should avoid to prevent data loss.

@DeepDiver1975
DeepDiver1975 Sep 17, 2015 Member

bรครครครคh .... we have to catch this on the storage implementation .... such guessing on what the problem might be is ๐Ÿ’ฉ

@icewind1991
icewind1991 Sep 17, 2015 Member

As for the NotFound, sabre already throws that if the file is not found

@butonic
butonic Sep 17, 2015 Member

The problem only surfaces because the code path in our sabre connector never checks the return value of fopen after it checked the stat for it. the file view does ... so ... i think this is the right place.

butonic and others added some commits Sep 16, 2015
@butonic @DeepDiver1975 butonic prevent 0 byte downloads when storage returns false 773941d
@icewind1991 @DeepDiver1975 icewind1991 add unit test
2839ef3
@nickvergessen
Contributor

๐Ÿ‘

@icewind1991 icewind1991 merged commit b520a1e into master Sep 21, 2015

8 of 10 checks passed

core-ci-linux/database=pgsql,label=SLAVE Build #1483 failed in 4 min 24 sec
Details
Scrutinizer Created
Details
core-ci-linux-jsunit/database=sqlite,label=SLAVE Build #1218 succeeded in 28 sec
Details
core-ci-linux/database=mysql,label=SLAVE Build #1483 succeeded in 9 min 59 sec
Details
core-ci-linux/database=oci,label=SLAVE Build #1483 succeeded in 40 min
Details
core-ci-linux/database=sqlite,label=SLAVE Build #1483 succeeded in 4 min 17 sec
Details
server-master-linux-externals-ci/database=sqlite,external=smb-silvershell,label=vm-slave-02 Build #1139 succeeded in 1 min 16 sec
Details
server-master-linux-externals-ci/database=sqlite,external=webdav-ownCloud,label=vm-slave-02 Build #1139 succeeded in 3 min 17 sec
Details
server-master-linux-externals-smb-windows-ext-ci/database=sqlite,external=smb-windows,label=master Build #1210 succeeded in 1 min 16 sec
Details
server-master-linux-php7-ci/database=sqlite,label=SLAVE Build #1275 succeeded in 51 sec
Details
@icewind1991 icewind1991 deleted the prevent0bytedownloads branch Sep 21, 2015
@PVince81
Collaborator

@karlitschek backport to 8.0 and 8.1 ?

@PVince81
Collaborator

stable8.1: #21296
stable8: #21297

@karlitschek
Member

please backport ๐Ÿ‘

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment