PUT overwriting chunk uses wrong code path

[PVince81]

Blocker for testing #32282

Steps

1. check out 10.0.9-resuming-chunks branch (which is based on [stable10] [revived] Retry chunks in web UI on stalled uploads #32170 and adds logging)
2. replace code in Sabre/Directory.php with the following:

	public function createFileDirectly($name, $data) {
		//$this->fileView->file_put_contents($this->getPath() . '/' .  $name, $data);
		$tgt = $this->fileView->fopen($this->getPath() . '/' .  $name, 'wb');
		sleep(120);
		die('');

		$count = 0;
		while(!feof($data)) {
				sleep(120);
		}
		fclose($data);
		fclose($tgt);

	}

3. occ config:app:set files upload_stall_timeout --value 5
4. then upload a 120 mb file in the web UI
5. observe the data/admin/uploads/*/ folders and see that chunk data is written depite the code not being run

So far my observation is that whenever a chunk is aborted on the JS side, there is suddenly a second request to the same URL, and here somehow it bypasses the code path leading to createFileDirectly but goes to Sabre\File::put instead.

{"reqId":"ee9W6RalKAyug9foZDvf","level":3,"time":"2018-08-09T16:33:55+00:00","remoteAddr":"127.0.0.1","user":"admin","app":"DEBUG","method":"PUT","url":"\/owncloud\/remote.php\/dav\/uploads\/admin\/web-file-upload-646ae38e82a0dbdc5f71465402a96df6-1533832395140\/52428800","message":"createFileDirectly: 52428800"}
{"reqId":"H2vTTPDb1QuZpl6Q0ZMj","level":3,"time":"2018-08-09T16:34:04+00:00","remoteAddr":"127.0.0.1","user":"admin","app":"DEBUG","method":"PUT","url":"\/owncloud\/remote.php\/dav\/uploads\/admin\/web-file-upload-646ae38e82a0dbdc5f71465402a96df6-1533832395140\/52428800","message":"Sabre\\File::put \/web-file-upload-646ae38e82a0dbdc5f71465402a96df6-1533832395140\/52428800"}
{"reqId":"H2vTTPDb1QuZpl6Q0ZMj","level":3,"time":"2018-08-09T16:34:04+00:00","remoteAddr":"127.0.0.1","user":"admin","app":"DEBUG","method":"PUT","url":"\/owncloud\/remote.php\/dav\/uploads\/admin\/web-file-upload-646ae38e82a0dbdc5f71465402a96df6-1533832395140\/52428800","message":"streamCopy "}

@VicDeo @DeepDiver1975

[PVince81]

[PVince81]

Request headers of aborted call (I made anther one not aborted to get real headers, else I'd get only "provisional headers")

PUT /owncloud/remote.php/dav/uploads/admin/web-file-upload-3bb3587bb6734f4ab08cf829352c592a-1533832986652/0 HTTP/1.1
Host: localhost
Connection: keep-alive
Content-Length: 10485760
Cache-Control: max-age=0
requesttoken: XD9jRnInLhlRHSkMOCwOXlkDMV0qEDYVB1doJCgaBTQ=:iPQ78UK2ejn9weY+oMzifewya1CtnrnL4qU0lLM1/cw=
X-OC-Mtime: 1417094915.04
Origin: http://localhost
X-Requested-With: XMLHttpRequest
User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.75 Safari/537.36
OCS-APIREQUEST: true
Content-Type: application/octet-stream
Accept: */*
If-None-Match: *
Content-Disposition: attachment; filename="data2.dat"
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.9,de;q=0.8
Cookie: oc_sessionPassphrase=ZwAoovI012u6kVPC6M3Pu2b2Sm3YY7g%2Bw5tMd0092%2F7jtMuDUH34wclhFLBYcvkWhQlaOyw%2BrHGbDYIGkmhNaPFQaNejSUpQI6SnzfXCesq1Sea6Jr459RuNVNYZBBVt; ockbfur1449x=uottb7ir08bchurpv5j0f8h932

Request headers of subsequent call:

PUT /owncloud/remote.php/dav/uploads/admin/web-file-upload-646ae38e82a0dbdc5f71465402a96df6-1533832395140/0 HTTP/1.1
Host: localhost
Connection: keep-alive
Content-Length: 10485760
requesttoken: XD9jRnInLhlRHSkMOCwOXlkDMV0qEDYVB1doJCgaBTQ=:iPQ78UK2ejn9weY+oMzifewya1CtnrnL4qU0lLM1/cw=
X-OC-Mtime: 1417094906.53
Origin: http://localhost
User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.75 Safari/537.36
OCS-APIREQUEST: true
Content-Type: application/octet-stream
Accept: */*
X-Requested-With: XMLHttpRequest
Content-Disposition: attachment; filename="data1.dat"
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.9,de;q=0.8
Cookie: oc_sessionPassphrase=ZwAoovI012u6kVPC6M3Pu2b2Sm3YY7g%2Bw5tMd0092%2F7jtMuDUH34wclhFLBYcvkWhQlaOyw%2BrHGbDYIGkmhNaPFQaNejSUpQI6SnzfXCesq1Sea6Jr459RuNVNYZBBVt; ockbfur1449x=uottb7ir08bchurpv5j0f8h932

[PVince81]

Ok, so the first call uses If-None-Match: * but the second doesn't.
It looks like for the first call it would use "createFile" as the chunk doesn't exist yet on disk, and then it quickly resumes and overwrites the existing one, so the code path that overwrites chunks goes to "updateFile" and uses "Sabre\File::put".

I'm not sure if this is good... We're supposed to go straight to storage using createFileDirectly.

So the bug here is that overwriting an existing chunk doesn't go to "createFileDirectly" but the other route.

This also means that when trying to test "retry the same chunk several times" one would need to put a sleep() in both routes.

[PVince81]

The overwrite of chunks is triggered here on retry: https://github.com/owncloud/core/blob/10.0.9-resuming-chunks/apps/files/js/file-upload.js#L1130

[ownclouders]

GitMate.io thinks possibly related issues are #26790 (Catch aborted connections in more places), #26121 (Connection Closed), #16763 (Write unit tests for file chunking), #23460 (Connection Report), and #13833 (LDAP connections).

[PVince81]

Seems we'll need a ChunkedFile implementation of Sabre's IFile to represent a chunk file to be able to catch File::put() and rewire to doing a direct PUT instead of using the standard OC "File" implementation which has a lot more.