Fix VFX Windows .lnk files freeze/crash issue #3057

allexzander · 2021-04-01T15:52:52Z

This is a fix for #3041

As mentioned here https://doc.qt.io/qt-5/qfileinfo.html#details

"On Windows, shortcuts (.lnk files) are currently treated as symlinks. As on Unix systems, the property getters return the size of the targeted file, not the .lnk file itself. This behavior is deprecated and will likely be removed in a future version of Qt, after which .lnk files will be treated as regular files."

In our case, executing any of QFileInfo or QDir functions that query the ".lnk" file internally, would result in a freeze if the ".lnk" is not pointing to a valid location (a shortcut created on another PC to a folder that only exists there). The freeze is causing the CfAPI to trigger the hydration, even though, it's not triggered by the desktop client's logic (not sure why this happens).
Another problem is - when clicking "Free up local space" the logic breaks when trying to create a placeholder for a ".lnk" file because the ".lnk" file is queried with QFileInfo::isDir() which does not lead to a freeze, but, the result returned is incorrect (it treats this file as a directory, while it should be treated as a normal file. This is caused by the same Qt's bug described above). The result - is a crash because of using an incorrect file handle for placeholder creation.

During the discussion with teammates, we've decided to always ignore ".lnk" files on Windows. With, or without the VFS enabled (for consistency). ".lnk" files point to a real file/folder anyways, so, by ignoring them from sync, we are not losing the data.

src/libsync/discovery.cpp

src/libsync/discoveryphase.cpp

FlexW · 2021-04-01T16:41:15Z

src/libsync/discovery.cpp

+        const bool isServerEntrySymLink = !e.localEntry.isValid() && e.serverEntry.isValid() && !e.serverEntry.isDirectory && FileSystem::isLnkFile(e.serverEntry.name);
+#else
+        const bool isServerEntrySymLink = false;
+#endif


I wonder if it is a good idea to move the preprocessor conditionals into the FileSystem::isLnkFile() method? That would simplify the logic a bit.

@FlexW
In this case, I wouldn't do that, cause, "isLnkFile" can be used on other platforms too.

cscholz · 2021-04-05T19:03:37Z

@allexzander: Let me known when the fix is available in the daily build. I'm gonne test once it again.

er-vin · 2021-04-06T05:39:45Z

* In our case, executing any of **QFileInfo** or **QDir** functions that query the ".lnk" file internally, would result in a freeze if the ".lnk" is not pointing to a valid location (a shortcut created on another PC to a folder that only exists there). The freeze is causing the CfAPI to trigger the hydration, even though, it's not triggered by the desktop client's logic (not sure why this happens).

This is because a QFileInfo or a QDir call will need to read the content of the lnk file to know where it points to in order to give a proper reply. Which will thus trigger the implicit hydration code path in a secondary thread (Windows is triggering this, implicit hydration is generally triggered by outside processes but not in that case). Now remember the download itself has to be done in the main thread, but the main thread is blocked waiting on a QFileInfo or a QDir sync call so it's not able to process the necessary events.

er-vin

As for the general approach, a word of caution: are we sure this is unused by users? I wouldn't be surprised if there were people somewhere having symlinks or lnk files pointing inside or outside the sync folder.

I'm not saying it shouldn't be changed (especially pointing outside is... well... don't :-)), but clearly it should be done with some care to not silently break something for those users.

er-vin · 2021-04-06T05:43:19Z

src/libsync/discoveryphase.cpp

@@ -305,7 +306,13 @@ void DiscoverySingleLocalDirectoryJob::run() {
        i.inode = dirent->inode;
        i.isDirectory = dirent->type == ItemTypeDirectory;
        i.isHidden = dirent->is_hidden;
-        i.isSymLink = dirent->type == ItemTypeSoftLink;
+#ifdef Q_OS_WIN


Why isn't that one done in csync_vio_local_win.cpp instead? With what you're trying to achieve, this would be a more natural fit to me to not set ItemTypeSoftLink there in the first place.

@er-vin In fact, I am fine with ItemTypeSoftLink, but, I've moved this into csync_vio_local_win.cpp indeed. Pushed a new commit with changes.

allexzander · 2021-04-06T06:48:24Z

As for the general approach, a word of caution: are we sure this is unused by users? I wouldn't be surprised if there were people somewhere having symlinks or lnk files pointing inside or outside the sync folder.

I'm not saying it shouldn't be changed (especially pointing outside is... well... don't :-)), but clearly it should be done with some care to not silently break something for those users.

@er-vin We've been discussing that. And, yes, indeed, this can be used by some people. More on that, some of them may be using SMB share and have links pointing to a file/folder on their SMB file server machine. Having said that, right now, the problem with VFS is much more severe than it will become for those users, and, we can think of some improvement for them later if they start to complain. It's the simplest way right now to just ignore those files.

Other solutions:

Create a wrapper around QFileInfo/QDir/QFile, and use it in place of a direct call to these classes' methods, but, treat ".lnk" files in a special way (e.g. use WinaAPI functions like PathIsDirectory that don't freeze on ".lnk" files from what I have found out). This requires lots of changes in multiple places (not just CfAPI code, but, also owncloudpropagator.cpp has some calls that freeze, and, possibly, other places.
Make those files (.lnk ones) always local and don't allow dehydration (this works too from what I've noticed). But, this introduces changes to the VFS logic. This is not something for 3.2.0, I'd say, as, this will need to be tested.

So, once again, ignoring those ".lnk" files, for now, is really a quick fix for 3.2.0.

er-vin · 2021-04-06T12:44:06Z

So, once again, ignoring those ".lnk" files, for now, is really a quick fix for 3.2.0.

Not denying that, I'm not even saying this should move in a different direction it's likely the better approach. I'm just wondering about the impact for the users who already have those lnk files synced typically. Will that make them disappear? Only locally or also remotely?

I'm just wondering if you want a notification on top like the warning @FlexW has been working on. So that they know what to expect.

Also: this could be a fix for 3.2.1... it's really last minute again.

allexzander · 2021-04-06T12:49:40Z

So, once again, ignoring those ".lnk" files, for now, is really a quick fix for 3.2.0.

Not denying that, I'm not even saying this should move in a different direction it's likely the better approach. I'm just wondering about the impact for the users who already have those lnk files synced typically. Will that make them disappear? Only locally or also remotely?

I'm just wondering if you want a notification on top like the warning @FlexW has been working on. So that they know what to expect.

Also: this could be a fix for 3.2.1... it's really last minute again.

@er-vin Remote ".lnk" files will not disappear. They are just ignored locally and remotely altogether. Users can't sync new ".lnk" files from the remote, even if those were being synced before. Also, newly created ".lnk" files won't get uploaded to the remote anymore.

There is a notification message already (files from ignore list as well as symbolic links are not synced, or something like that).

This MUST be in 3.2.0 as this is a severe issue and it will look like 3.2.0 is completely broken for new users. So, we've decided it should be in 3.2.0, otherwise, we need RC4, which we can not afford. We will be testing daily build with this fix and release 3.2.0. Without this, VFS is pretty-much useless :(

er-vin · 2021-04-06T12:55:13Z

There is a notification message already (files from ignore list as well as symbolic links are not synced, or something like that).

Ah then it's all good I guess.

This MUST be in 3.2.0 as this is a severe issue and it will look like 3.2.0 is completely broken for new users.

If they have a .lnk file in there but yes. I wonder if that's really that common.

So, we've decided it should be in 3.2.0, otherwise, we need RC4, which we can not afford. We will be testing daily build with this fix and release 3.2.0. Without this, VFS is pretty-much useless :(

Again: if you have a .lnk file in your sync folder.

In any case the "this could be for 3.2.1" was more for the notification but it's already there anyway.

allexzander · 2021-04-06T16:18:19Z

Again: if you have a .lnk file in your sync folder.

@er-vin It's not for 50% of users I guess, but, for many of them. cloud.nextcloud.com, for example, has a few of those files, same with those users that commented on the issue I've mentioned - they both had .lnk files in their sync folder. So, it's kinda, big enough amount of users that will start to complain after they upgrade to 3.2.0 and enable VFS.

Sor, there are risks in both cases, I am not even sure now what's worse.

tomdereub · 2021-04-07T06:54:44Z

For me, ignoring .ink files is a good fix for now. I have .ink files in my folders, so I can't use vfx for the moment.
And generaly .ink files are not usable over synchronization anyway: users often synchronize their files in C:\users\name\nextcloud, file paths are different, so a .ink is usable just by one user.
From my point of view, it's much worst to get many people with a big error than to just not synchronize shortcuts.

er-vin · 2021-04-07T07:28:25Z

Again: if you have a .lnk file in your sync folder.

@er-vin It's not for 50% of users I guess, but, for many of them. cloud.nextcloud.com, for example, has a few of those files, same with those users that commented on the issue I've mentioned - they both had .lnk files in their sync folder. So, it's kinda, big enough amount of users that will start to complain after they upgrade to 3.2.0 and enable VFS.

Yeah that's what I was trying to gauge, are we talking say about 5% or 50%. But again, if it doesn't kill preexisting files on the server and locally and if there's a notification it's probably good. From your assessment it seems to have both so it's probably the right approach and won't require further work.

Sor, there are risks in both cases, I am not even sure now what's worse.

Welcome in the wonderful world of dealing with people data. ;-)

er-vin · 2021-04-07T07:37:43Z

src/libsync/discovery.cpp

@@ -177,9 +178,15 @@ void ProcessDirectoryJob::process()
        // local stat function.
        // Recall file shall not be ignored (#4420)
        bool isHidden = e.localEntry.isHidden || (f.first[0] == '.' && f.first != QLatin1String(".sys.admin#recall#"));
+#ifdef Q_OS_WIN
+        // exclude ".lnk" files as they are not essential, but, causing troubles when enabling the VFS due to QFileInfo::isDir() and other methods are freezing, which causes the ".lnk" files to start hydrating and freezing the app eventually.
+        const bool isServerEntrySymLink = !e.localEntry.isValid() && e.serverEntry.isValid() && !e.serverEntry.isDirectory && FileSystem::isLnkFile(e.serverEntry.name);


Maybe s/SymLink/LnkFile/ here? (and in the else part of course) or s/SymLink/WindowsShortcut/ as you did in the vfs implementation. Just to align the naming everywhere, since it's not a symlink in the unix sense.

@er-vin Changed

nextcloud-desktop-bot · 2021-04-07T07:48:36Z

AppImage file: Nextcloud-PR-3057-e76d9100bc6293ea3a5b4698aee882572edf8271-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

FlexW · 2021-04-07T08:10:16Z

What I don't like is that we now have this isLnkFile() method and a special case just for these files. In my opinion we should have just one, for example isSymLink() method, that handles lnk files as well as symbolic links on other platforms, so that we don't sprinkle our code ifndef's. But I guess that is to much work for now :)

er-vin · 2021-04-07T08:16:45Z

What I don't like is that we now have this `isLnkFile()` method and a special case just for these files. In my opinion we should have just one, for example `isSymLink()` method, that handles lnk files as well as symbolic links on other platforms, so that we don't sprinkle our code ifndef's. But I guess that is to much work for now :)

I'm not too much in love with those ifdefs to be honest. Now the thing is they are hard to get rid of and probably the best option for now because lnk files and symlinks are not the same thing despite looking somewhat similar. It's in part why Qt is moving away from treating lnk files like symlinks. That abstraction kind of fails unfortunately, so a function like you propose would very likely lead to similar issues.

allexzander · 2021-04-07T09:07:09Z

/rebase

Signed-off-by: allexzander <blackslayer4@gmail.com>

… placeholder. Signed-off-by: allexzander <blackslayer4@gmail.com>

er-vin · 2021-04-07T09:45:46Z

Merged #3057 into master.

Oops, forgot to clean up history. Not that I mentioned it.

allexzander · 2021-04-07T10:04:04Z

@er-vin Daammn. Yeah. This is what happens when trying to do some work while being on a call as well :( Do you think I should rebase master? Or let it slide this time?

er-vin · 2021-04-07T10:08:41Z

@er-vin Daammn. Yeah. This is what happens when trying to do some work while being on a call as well :( Do you think I should rebase master? Or let it slide this time?

Too late, more harm than good when trying to change this after it got merged.

vgdh · 2021-07-03T18:56:47Z

I also want sync *.lnc files!

wlnx · 2021-07-05T23:04:46Z

Well... This change breaks things. Windows has symlinks, and lnk-files are not symlinks at all. The files are closer to *.desktop files in some Linux-DEs. The way how Qt works with the files is an issue and should be handled in a proper way, because just ignoring lnk-files is a good way to break user filesets (e. g., Total Commander portable contains multiple WebSite.lnk files) and workflows (e. g., syncdir may contain something like hr-docs.lnk... Ok, may have contained xD).
Could you pls make the point optional with something like "you can have either lnk-files or vfs"? Pleeeaaazzz...

clayauld · 2021-07-22T22:15:19Z

I think this issue and the "fix" posted here is a complete regression for Windows users. I understand the issue of not syncing symlinks, but .lnk files in windows are not that. They are shortcuts. The default behavior (in my opinion) should be to ignore the .lnk files in VFS mode, but put them as part of the ignore list instead of hard-coding it. Then, a user can remove them from the ignore list and disable VFS mode if they want to sync those files. OR the files can be treated like .desktop files in Linux.

Because this is hardcoded, I'm seriously thinking about forking the repo and removing this "fix" so I can use the .lnk files like I used to. Otherwise I'll be switching to a completely different software entirely.....

For now I've downgraded my client to v3.2.0-rc3

allexzander · 2021-07-23T06:39:48Z

I can understand you, but, right now, it's something we had to do to allow VFS to work. At some point (could be even this year), we plan to approach this problem from another perspective and bring back .lnk files synchronization.

clayauld · 2021-07-23T08:39:04Z

Fair enough. I'm not a user of the VFS mode so until that change occurs I'll use my forked version.

I'll keep an eye on the development effort, in any case.

vgdh · 2021-07-23T17:54:03Z

lnk must back! ))

dbmaxpayne · 2021-07-27T21:31:32Z

Hey, I'd also like to have *.lnk-syncing back. I second the suggestion to implement an entry in the global ignore list that only gets set when VFS is used and then cannot be deselected. However, I'm also using the full sync and am missing my *.lnk files.

clayauld · 2021-07-27T22:31:47Z

I've moved back over to Seafile because of these issues. Its a breaking change for me and really limits my ability to use Nextcloud as I would like.

wlnx · 2021-07-27T23:03:47Z

I've moved back over to Seafile because of these issues. Its a breaking change for me and really limits my ability to use Nextcloud as I would like.

Well... Actually, I installed NC Desktop 3.1.3, thus ends. VFS not working, but it seems to me to be a cosmetic feature. I'd better miss some hard drive space rather than some sensitive data.

Taomyn · 2021-08-05T06:59:46Z

Not sync'ing .lnk files is a huge mistake, users with shortcuts on their desktops would completely lose them if they were to recover their local profile for any reason and not everyone can remember exactly what they were to simply "put them back manually".

allexzander · 2021-08-05T07:21:16Z

@Taomyn Desktop client is not meant for using it as backup software.

Taomyn · 2021-08-05T07:34:29Z

@Taomyn Desktop client is not meant for using it as backup software.

Show me exactly where I wrote it was - irrelevant

allexzander · 2021-08-05T07:39:01Z

@Taomyn

users with shortcuts on their desktops would completely lose them if they were to recover their local profile

Recover assumes having a backup that one wants to recover. Did you mean something else? When you connect the Nextcloud Desktop client to a remote directory, a local directory is not set to Desktop by default, unless you choose it manually. Local sync directory is a separate folder not related to Desktop, Downloads, Documents, etc. Hard to think of any use case where .lnk files on a Desktop synced to a remote server would be useful.

In any case, until we move to Qt 6.2+, we won't be working on fixing .lnk files with VFS as we have a lot of work to do in more critical areas. Apologies for temporary inconvenience.

Taomyn · 2021-08-05T07:48:19Z

@Taomyn
Recover assumes having a backup that one wants to recover. Did you mean something else? When you connect the Nextcloud

Well you know what they say about assumption - taking a file out of the recycle bin and back onto the desktop can also be seen as recover, same for emails from Deleted Items in Outlook.

No end of users store documents on their desktop, yes, it's not the best or even correct place, but it still happens and won't stop happening - adding the Desktop folder is a must to allow them to sync those files as well. I think you need to "think" harder.

I'm just glad that the other users I need to support are using OneDrive, as its "Files-on-demand" feature has zero issues with this plus Desktop is an included folder by default. It too isn't a backup solution.

allexzander · 2021-08-05T07:54:40Z

@Taomyn

I think you need to "think" harder.

I don't really agree this is a good place for such conversations. I understand your frustration, but, there is little I can do right now.

If by any chance, you can find some spare time, we would welcome it if you open a PR that brings back .lnk files support that is fully compatible with CFApi and Qt.

vgdh · 2021-08-05T12:17:51Z

😥

wlnx · 2021-08-09T12:33:44Z

@Taomyn Desktop client is not meant for using it as backup software.

@allexzander It definitely isn't. But is it meant for using it as synchronization software when it isn't synchronizing files?

As for the general approach, a word of caution: are we sure this is unused by users? I wouldn't be surprised if there were people somewhere having symlinks or lnk files pointing inside or outside the sync folder.
I'm not saying it shouldn't be changed (especially pointing outside is... well... don't :-)), but clearly it should be done with some care to not silently break something for those users.

@er-vin We've been discussing that. And, yes, indeed, this can be used by some people. More on that, some of them may be using SMB share and have links pointing to a file/folder on their SMB file server machine. Having said that, right now, the problem with VFS is much more severe than it will become for those users, and, we can think of some improvement for them later if they start to complain. It's the simplest way right now to just ignore those files.

Other solutions:

Create a wrapper around QFileInfo/QDir/QFile, and use it in place of a direct call to these classes' methods, but, treat ".lnk" files in a special way (e.g. use WinaAPI functions like PathIsDirectory that don't freeze on ".lnk" files from what I have found out). This requires lots of changes in multiple places (not just CfAPI code, but, also owncloudpropagator.cpp has some calls that freeze, and, possibly, other places.

Make those files (.lnk ones) always local and don't allow dehydration (this works too from what I've noticed). But, this introduces changes to the VFS logic. This is not something for 3.2.0, I'd say, as, this will need to be tested.

So, once again, ignoring those ".lnk" files, for now, is really a quick fix for 3.2.0.

We started to complain. 😊Moreover, you knew that we would. Unfortunately, I ain't a programmer and cannot write the patch for lnk+CFapi+Qt to work together. That's why the only thing I can do is whining: "Please fix the regression".

CombeeMike · 2021-08-27T07:06:42Z

I'm also one of those who really miss syncing of windows *.lnk files and for whom this change drastically degrades the usefulness of my whole Nextcloud installation.

Most of my thoughts have already been said by someone else, so I won't be repeating them here. However, I'd like to explain my use case here since that's maybe something you (the devs) would like to consider for future changes etc.:

Here's my use case (copied form here)

I regularly work on multiple machines (work notebook, desktop pc home, notebook home, ...) which all "share" some basic data (data synced via Nextcloud, data synced via one drive, data "synced" via GitHub, ...).

For all of this data I built an extensive list of "most valuable links" which is basically just a folder inside my Nextcloud holding links to important folders, files, programs, ...

I can access all those links very quickly using a quick launcher like Launchy and/or PowerToys Run.

Most of those links "internally" use a set of defined system vars in their paths instead of absolute paths to support different paths on different machines. E.g.: The Nextcloud.lnk points to %nextcloud% instead of D:\Nextcloud which allows the path to differ on every machine.

SilenyKrecek · 2021-10-18T13:26:39Z

+1 to CombeeMike
My use case is quite similar to his.
.lnk must come back :D

…indows-lnk-files-freeze-issue"

allexzander changed the title ~~Bugfix/vfs windows lnk files freeze issue~~ Fix VFX Windows .lnk files freeze/crash issue Apr 1, 2021

allexzander requested review from mgallien, FlexW, er-vin and camilasan April 1, 2021 15:55

FlexW reviewed Apr 1, 2021

View reviewed changes

allexzander force-pushed the bugfix/vfs-windows-lnk-files-freeze-issue branch from 4ff8ce0 to d37c28f Compare April 5, 2021 11:06

er-vin reviewed Apr 6, 2021

View reviewed changes

allexzander requested review from er-vin and FlexW April 6, 2021 07:51

er-vin reviewed Apr 7, 2021

View reviewed changes

allexzander requested a review from er-vin April 7, 2021 07:42

FlexW approved these changes Apr 7, 2021

View reviewed changes

allexzander added 4 commits April 7, 2021 09:08

Fix VFS freeze when syncing '.lnk' files on Windows.

d5b6d93

Signed-off-by: allexzander <blackslayer4@gmail.com>

Added .lnk file placeholder creation fix.

7235c70

Signed-off-by: allexzander <blackslayer4@gmail.com>

Another way to fix this.

3906ae0

Signed-off-by: allexzander <blackslayer4@gmail.com>

Treat .lnk files as normal files without hydrating them or creating a…

13f4bb5

… placeholder. Signed-off-by: allexzander <blackslayer4@gmail.com>

allexzander merged commit 5eb6834 into master Apr 7, 2021

allexzander deleted the bugfix/vfs-windows-lnk-files-freeze-issue branch April 7, 2021 09:28

allexzander mentioned this pull request Apr 15, 2021

Client 3.2 RC2 - Client freeze during sync (? vfs link creation) #3041

Closed

FlexW mentioned this pull request Apr 30, 2021

[3.2.1 regression] Shortcut .lnk files cannot be synced anymore #3244

Closed

Discostu36 mentioned this pull request Jul 2, 2021

*.lnk files is not syncing #3499

Closed

mgallien added this to the 3.4.0 milestone Oct 19, 2021

chylex added a commit to chylex/Nextcloud-Desktop that referenced this pull request Feb 7, 2022

Revert "Merge pull request nextcloud#3057 from nextcloud/bugfix/vfs-w…

d2e5094

…indows-lnk-files-freeze-issue"

RokeJulianLockhart mentioned this pull request Jun 7, 2022

Syncing symbolic links as reference #250

Open

Fix VFX Windows .lnk files freeze/crash issue #3057

Fix VFX Windows .lnk files freeze/crash issue #3057

Conversation

allexzander commented Apr 1, 2021 • edited Loading

FlexW Apr 1, 2021

Choose a reason for hiding this comment

allexzander Apr 2, 2021

Choose a reason for hiding this comment

cscholz commented Apr 5, 2021

er-vin commented Apr 6, 2021

er-vin left a comment

Choose a reason for hiding this comment

er-vin Apr 6, 2021

Choose a reason for hiding this comment

allexzander Apr 6, 2021

Choose a reason for hiding this comment

allexzander commented Apr 6, 2021

er-vin commented Apr 6, 2021

allexzander commented Apr 6, 2021 • edited Loading

er-vin commented Apr 6, 2021

allexzander commented Apr 6, 2021

tomdereub commented Apr 7, 2021

er-vin commented Apr 7, 2021

er-vin Apr 7, 2021

Choose a reason for hiding this comment

allexzander Apr 7, 2021

Choose a reason for hiding this comment

nextcloud-desktop-bot commented Apr 7, 2021

FlexW commented Apr 7, 2021

er-vin commented Apr 7, 2021 via email

allexzander commented Apr 7, 2021

er-vin commented Apr 7, 2021 via email

allexzander commented Apr 7, 2021

er-vin commented Apr 7, 2021 via email

vgdh commented Jul 3, 2021

wlnx commented Jul 5, 2021

clayauld commented Jul 22, 2021 • edited Loading

allexzander commented Jul 23, 2021

clayauld commented Jul 23, 2021

vgdh commented Jul 23, 2021

dbmaxpayne commented Jul 27, 2021

clayauld commented Jul 27, 2021

wlnx commented Jul 27, 2021

Taomyn commented Aug 5, 2021

allexzander commented Aug 5, 2021

Taomyn commented Aug 5, 2021

allexzander commented Aug 5, 2021

Taomyn commented Aug 5, 2021

allexzander commented Aug 5, 2021

vgdh commented Aug 5, 2021

wlnx commented Aug 9, 2021

CombeeMike commented Aug 27, 2021 • edited Loading

Here's my use case (copied form here)

SilenyKrecek commented Oct 18, 2021

allexzander commented Apr 1, 2021 •

edited

Loading

allexzander commented Apr 6, 2021 •

edited

Loading

clayauld commented Jul 22, 2021 •

edited

Loading

CombeeMike commented Aug 27, 2021 •

edited

Loading