owncloud / client

🖥️ Desktop Syncing Client for ownCloud
GNU General Public License v2.0
1.4k stars 664 forks source link

[2.2.4] [NTFS on a VeraCrypt Volume] Windows clients play ping-pong forever #5375

Closed wiesl closed 6 years ago

wiesl commented 7 years ago

Everything tested with 2 clients with Version: 2.2.4 for Windows under Windows 10

Expected behaviour

No ping pong should be played (e.g. file is downloaded and uploaded again, same on other computer, and then on the original one again)

Actual behaviour

Uploads/downloads are done forever

Steps to reproduce

  1. Owncloud server
  2. 2x Latest Owncloud clients under Windows 10
  3. Don't touch the files

Server configuration

Operating system: Fedora 25

Web server: Apache

Database: PostgreSQL

PHP version: php-7.0.13-2.fc25.x86_64

ownCloud version: owncloud-9.1.1-1.fc25.noarch

Storage backend (external storage): NTFS on a VeraCrypt Volume (1.19)

Client configuration

Client version: 2.2.4

Operating system: Windows 10 (latest version)

OS language: German

Installation path of client: Standard path

Logs

Analysis has been done already. Timestamp of the file is not set correctly (server time) but set to the local time. See the next comments.

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/39937325-2-2-4-ntfs-on-a-veracrypt-volume-windows-clients-play-ping-pong-forever?utm_campaign=plugin&utm_content=tracker%2F216457&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F216457&utm_medium=issues&utm_source=github).
wiesl commented 7 years ago

I guess the problems comes from the following:

propagatedownload.cpp void PropagateDownloadFile::downloadFinished()

    FileSystem::setModTime(_tmpFile.fileName(), _item->_modtime);
    // We need to fetch the time again because some file systems such as FAT have worse than a second
    // Accuracy, and we really need the time from the file system. (#3103)
    _item->_modtime = FileSystem::getModTime(_tmpFile.fileName());

    if (FileSystem::fileExists(fn)) {
        // Preserve the existing file permissions.
        QFileInfo existingFile(fn);
        if (existingFile.permissions() != _tmpFile.permissions()) {
            _tmpFile.setPermissions(existingFile.permissions());
        }
        preserveGroupOwnership(_tmpFile.fileName(), existingFile);

Another guess might be then from the move, but that is more unlikely.

A screenshot has been made, where the tempfile had already the wrong timestamp of 19:45 which is the local modified timestamp and not the server modified timestamp (saveputty.bat gets it then, too).

Can you please provide a build to test.

dragotin commented 7 years ago

Note: NTFS on a VeraCrypt Volume (1.19) as storage backend.

wiesl commented 7 years ago

NTFS is just a regular volume like on any other backend. There should not be any difference.

ogoffart commented 7 years ago

Do you have the client log? Could it be that the rename of the tmp file does not keep its modification date?

wiesl commented 7 years ago

Can provide the log later. No, the tmp file (which I saw only for a short period of time by intention) had already the wrong timetimestamp. Therefore it can't be the rename and the suggested patch should fix it. Can you provide a build to test with the patch?

guruz commented 7 years ago

@wiesl Sorry I don't see your suggested patch, could you provide it as a real diff (e.g. via https://gist.github.com) or PR? You are using NTFS/VeraCrypt on the SERVER?

wiesl commented 7 years ago

see proposed patch: https://gist.github.com/wiesl/847ce0e0f515f9bff78ac3fae45e87f4

I'm using VeraCrypt/NTFS on the CLIENT, to be precise on 2 clients with the Windows clients (Server is Linux)! Details see above.

Can you please provide a test build? I can test it fast.

guruz commented 7 years ago

@wiesl Could you try http://download.owncloud.com/desktop/daily/ownCloud-2.3.0.6610-nightly20170103-setup.exe? :)

wiesl commented 7 years ago

Doesn't work. Are you sure the correct branch is used (veracrypt_5375) in the build? Test program looks ok: https://gist.github.com/wiesl/e719815c77edc2dc7d4ef32b42469210

Any other idea which code part might modify the timestamp to the actual date on retrival from the server?

guruz commented 7 years ago

@wiesl From what I see in the build machine that branch is used, but can you double check by looking at the git sha in the settings dialog of the client in that build?

wiesl commented 7 years ago

Yes, libowncloudsync.dll has the commit id in it as well as the settings commit id. Anything modifications done after that code is executed?

guruz commented 7 years ago

Your patch looked fine to me, I didn't expect anything after to change the mtime. The file moving etc is done before.

Maybe the log helps too? It could be related to the file watcher thinking something different?

Could it be that VeraCrypt is only updating the timestamps after some delay? So even with your patch we're getting an old timestamp that is changed in the next sync run and leads to upload?

Shall I provide a build for you with more logging?

wiesl commented 7 years ago

Yes, I know patch looks ok.

I don't see any useful information in the logs except that the written timestamp after download is correct, but the file hasn't a correct timestamp afterwards.

My test program works immediately for the timestamp change, so everything looks correct.

So to summarize again: 1.) download file with correct timestamp information according to the log 2.) create tempfile with modified timestamp (but has already wrong timestamp with actual time) 3.) file is moved to original file and permissions are updated (my guess was here that the permission change updated the timestamp) 4.) PATCH: tried to modify the timestamp again after the move (but it is still wrong) 5.) After next rescan file is newer than in local SQLite database and therefore an upload starts with the new timestamp 6.) Server has a new timestamp and on the second client the download starts at step 1 and we are playing ping-pong :-)

So I understand pretty well what happens but I don't understand why and where to fix it.

Yes, please provide a build with more logging, that would be great.

e.g. // Log here _item->_modtime FileSystem::setModTime(fn, _item->_modtime); // We need to fetch the time again because some file systems such as FAT have worse than a second // Accuracy, and we really need the time from the file system. (#3103) _item->_modtime = FileSystem::getModTime(fn); // Log here _item->_modtime

Done twice (in total 4 times) at the top of the file and in the patched section. Maybe some other logs might help, too.

Thnx.

guruz commented 7 years ago

New build available -> http://download.owncloud.com/desktop/daily/ownCloud-2.3.0.6615-nightly20170105-setup.exe

Are you syncing the container file iself or files INSIDE the container?

wiesl commented 7 years ago

of course, syncing files INSIDE the container. Syncing the container files doesn't make sense and would corrupt the container file (I guess it would be locked anyway). Will test the new build.

ogoffart commented 7 years ago

Maybe another process is modifying the file? (antivirus, ...)

wiesl commented 7 years ago

Analysis of logging debug code of http://download.owncloud.com/desktop/daily/ownCloud-2.3.0.6615-nightly20170105-setup.exe showed that all 4 logged timestamps are ALWAYS the same. I looked already into details but still no conclusion (quite busy days).

I was also thinking of antivirus but hadn't the time to disable it yet and analyze the logs in detail.

wiesl commented 7 years ago

I forgot to mention that I wrote a test program to set the timestamp manually for a file. That worked perfectly on the Veracrypt/NTFS volume. So something else must modify it. Hopefully I can dig into on the weekend. Maybe you can provide a build with more logging. Thnx.

guruz commented 7 years ago

Any news on this issue? :/

wiesl commented 7 years ago

Currently busy, hopefully on the weekend :-)

ogoffart commented 7 years ago

Can this issue be reproduced?
I guess this could be also solved if we check the checksum/hash for every files and not only for .eml file. But then we would not update the mtime to the server (because otherwise the server would change the etag again)

guruz commented 7 years ago

@ogoffart Let's keep it open still and hope for @wiesl to reply. In general, we should check the hash in more cases

ogoffart commented 6 years ago

@wiesl any news? Does it still happen with the 2.4 beta? Did you get any log that would exmplain why the mtime is changing.

wiesl commented 6 years ago

Is ok now with ownclooud client version 2.3.4 and most likely also a newer server version. Veracrypt same version. owncloud-9.1.5-1.fc26.noarch