Slow transfer can bypass etag precondition check, no conflict file

[PVince81]

Steps to reproduce

1. Create a file "test.txt" with the contents "initial"
2. Setup as sync client
3. Add sleep(30) here: https://github.com/owncloud/core/blob/master/lib/private/connector/sabre/file.php#L190 (before the final rename)
4. Change "test.txt" locally, set its contents to "local" and let the sync client upload the file
5. While it's uploading, change "test.txt" in the web UI and set the contents to "web" and save it
6. Wait for the sync to finish
7. Check the file's contents

Expected result

Conflict must occur.

Actual result

File edited from web UI was overwritten

This happens because the precondition check happens before the transfer is finished.
If chunk assembly or writing the part file takes longer (for example with ext storage), we're way past the precondition check and the write operation will simply overwrite what's there.

I didn't check what happens with the etag and other clients, it might also create a situation where one client believes it has the latest version even though the contents is different (to be confirmed).

I hope we can fix this somehow with high-level file locking #11804. We need to make sure the precondition check + final rename are atomic.

Versions

Observed on master (f70c309)

@dragotin @guruz FYI, maybe this was already known.

@DeepDiver1975 @karlitschek @icewind1991

[PVince81]

CC @cmonteroluque

Setting to sev high for now.

[PVince81]

It also looks like no version is created for the "web" version of the file, so that edit is completely lost.
Could also happen if the change was done by another sync client.

The race condition is that both edits must happen at the same time after both did the precondition check.

[PVince81]

Currently the precondition check is done by Sabre before reaching our code.
We either need to find a way to move the chunking work before that happens (it needs to be reworked anyway), or do an additional precondition check just before doing the final rename (might be the easiest solution short term)

[PVince81]

Note: some other conditions could happen at the time of the final rename:

• the target folder has been moved (it currently returns 500 "could not rename part file to final file)
• the file to be overwritten has been moved/deleted, which would probably also fail a precondition check if it happened earlier

[PVince81]

I'll set this to 8.2 for now to keep it in mind.
With a bit of luck we might be able to cover this with the locking. But as this would overspill over the Sabre precondition check, it is not guaranteed to work and might need a custom solution.

[MorrisJobke]

Fix is in #17811