Bug 118594 - When "moving files, potentially overwriting destination", konqueror does not check that the source file exist before unlinking destination -> data loss.
Summary: When "moving files, potentially overwriting destination", konqueror does not ...
Status: RESOLVED FIXED
Alias: None
Product: kio
Classification: Frameworks and Libraries
Component: ftp (show other bugs)
Version: unspecified
Platform: Debian testing Linux
: NOR grave
Target Milestone: ---
Assignee: J
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-12-18 20:58 UTC by ombreleau
Modified: 2012-08-03 07:44 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.7.4


Attachments
proposed fix (1.34 KB, patch)
2011-11-05 19:00 UTC, Dawit Alemayehu
Details

Note You need to log in before you can comment on or make changes to this bug.
Description ombreleau 2005-12-18 20:58:38 UTC
Version:            (using KDE KDE 3.4.3)
Installed from:    Debian testing/unstable Packages
Compiler:          GCC version unknown (the one used to build Debian GNU/Linux testing/unstable packages) Target: i486-linux-gnu
OS:                Linux

Hello.

This error was discovered when moving files from some FTP remote folder to a local disk. Still, the symptoms make me think that it would occur in any circumstances involving moving items between two konqueror panels.

Summary:
One tries to move multiple files between two Konqueror panels in one operation (drag'n'drop, cut'n'paste...). If the operation is interrupted before its end, Konqueror only refreshes the data reflecting the destination status, not the source (bug #118593). If one attempts the same operation again without refreshing the source panel, konqueror may wrongly delete files because it fails do perform some basic sanity checks.

How to reproduce:
1- Prepare a local disk partition P with "only" N megabytes of free space (the smaller N is, the easiest the test will be)
2- Launch two instances of konqueror.
3- Split the first window into two panels (Ctrl-Shift-T)
4- Use the bottom panel to browse a folder containing more than N megabytes of files (having several files makes things easier to understand) *BEWARE* These MUST be test files, as some will be lost in the process. *BEWARE*
5- Use the top panel to browse to an empty folder of the partition P.
6- Select all files in the bottom panel, drag'n'drop them to the top panel, in a *move* operation.
7- When the partition P is full, Konqueror will complain as expected. Cancel the operation.
=8=> At that point, some files will already have been moved, and hence deleted from the source folder. This shows only in the *destination panel* the first konqueror.
9- Use the second instance of konqueror (or a terminal...) to free enough space on P.
10- In the first instance of konqueror, drag'n'drop the files again.
11- Konqueror complains that some destination files already exist. Choose the "overwrite all" option. (I did so because I did not know how it would handle the file which transfer was interrupted by the "no space left condition").
12- Konqueror complains that some source files do not exist anymore (those which have been moved in the first attempt). Choose "ignore" or "ignore all".
=13=> Even though the user asked to ignore the operation, Konqueror will still delete the destination files, because of the "overwrite all" behaviour!

Expected behaviour:
1 to 7: ok
=8=> Both panels must be refreshed to reflect the new situation. Cf. bug #118593.
9 to 12: ok
=13=> When moving a file is impossible because the source is not available any more, a warning is issued, and no file is deleted.

I expect a file manager to check that a dangerous operation (anything involving overwriting or deleting a file) can be lead to completion *beforehand*. When moving a file, if the source file does not exist when the time comes to read data, *nothing* should be done. Especially not deleting something that may be the only copy left of said file.

This is similar to bug #104606, where the source file of a *move* operation is wrongly deleted, but it is different in that here it is the destination file which is deleted. At first glance, I would say that "move" operations in Konqueror are screwed something bad...
Comment 1 Matthew Dawson 2008-11-29 03:13:10 UTC
Upon trying to reproduce this feature in KDE 4.2 (SVN), konqueror still does not update the display correctly.  However, during local testing, I was unable to reproduce the data loss mentioned in this bug.  Instead, konqueror immediately popped up a message saying a file was unable to be found and canceled the second move.  This bug might occur with remote systems only, as I am only able to test local filesystems.  Unless this bug does occur with remote filesystems still, it should be closed.
Comment 2 Michael Pyne 2008-12-31 03:27:13 UTC
I believe I have been able to reproduce this bug even in latest KDE 4.2 SVN.  It uses the same sequence as that given by the reporter, with the following additions:

1) I used a remote kioslave (sftp to my local computer specifically) for the source files.
2) The destination was the local small partition.

Basically once everything was setup I tried moving via drag-and-drop 10 files from the sftp:/ location to the local partition.  After the partition filled up I had all 10 files split between the two directories (although the source side (i.e., remote ioslave) hadn't updated itself and showed the original 10).  In addition the destination partition had "optimistically" updated with a few files that actually hadn't been copied.

I deleted all but one file from the destination partition, maintained the 10 files selected in the source folder (although there were not actually 10 files in the directory), and tried again (using "Skip" to advance past dialogs saying that the source file didn't exist, until the prompt to "Overwrite All").  I clicked on Overwrite All and once the partition filled up I got another disk full message.  I immediately hit Cancel but now there were only 7 files total, so 3 files were lost.

I just tried this again starting with 13 files and now have 9 files.  So there's definitely still something fishy in the water... -> confirming the bug, and I concur with a severity of "grave", if not critical due to reproducable data loss.
Comment 3 David Faure 2009-01-26 21:54:42 UTC
Wow testing this bug requires quite some setup...
In order not to mess up my existing partitions I created a dummy partition in a file.
I also needed to automate those instructions to make testing easier...

1) dd if=/dev/zero of=image count=200 ; mkfs.ext2 image ; mkdir mntpt ; sudo mount image mntpt -o loop -t ext2 ; cd mntpt ; sudo chown -R $UID . ; konqueror .

Then from another konsole tab in the same starting directory:
2) mkdir source ; cd source ; for i in test{1,2,3,4}; do dd if=/dev/zero of=$i count=55 ; done ; konqueror fish://localhost/$PWD

3) Move the files from source to mntpt, it will say "disk full" after test3.

Indeed, the source dir should update at this point (and this is why the bug only happens with remote files as source; if it was local then KDirWatch would tell konqueror about the changes automatically). Bug 118593. This one is easy to fix (KDirNotify call in CopyJob should be done even in case of errors). But let's move on for now....

4) (your step 9) make room in the full partition. I'll skip that step, not necessary AFAICS (and impossible with my setup ;). Well Michael Pyne deleted some of the recently moved files, that's where the data loss comes from of course. But I see that this isn't what this bug is about.

5) Let's select the four files again and move them to mntpt. "test1 already exists". Overwrite All. I see what the bug is about, it shouldn't delete before writing the new file since there is no existing source file. But the bug doesn't happen here (KDE 4.2.0), test1 remains untouched in the destination folder. Ah, damn, I see why. kio_ftp has special code for "copy from ftp to local file", which has the faulty code (remove dest before starting download), while kio_fish has no special code for copy-to-local, so the usual "get job + put job" is used, and kio_file's put does it right (waits for first chunk of data before truncating dest). Phew! So this is a kio_ftp bug after all.

BTW, Michael, I cannot reproduce "the destination partition had optimistically updated with a few files that actually hadn't been copied." -- that is IMHO impossible; kio_file cannot possibly list files that do not exist. There's just a test3.part file for the partially-transferred file, but no ghosts in the destination folder (unlike the source folder ;).
Comment 4 David Faure 2009-01-26 22:04:38 UTC
OK, both kio_ftp and kio_sftp have the same code (makes it easier to test this, using sftp://localhost), and the same "bug". However the bug looks difficult to fix, because [s]ftpGet needs a file descriptor before starting the download, changing the order of things would involve a major refactor (and kill the code factorization AFAICS).

Can we agree that fixing 118593 is enough? If konqueror had updated correctly to show the correct directory contents this bug wouldn't have happened. The only case where it could still happen would be when some remote source files were deleted independently (e.g. ssh + rm -f), which is quite a stretch...

Well, I'm cc'ing the author of the ftpGet code, maybe he can tell more about the possibility to refactor ftpGet/ftpCopyGet so that the file handle is determined by ftpGet on demand rather than being created upfront... some sort of hook would be needed (a signal maybe?) which would tell ftpCopyGet to go ahead and open the destination file, when the first chunk of data arrives. The other uses of ftpGet wouldn't connect to the signal since they don't have a destination fd. Juergen, are you interested in fixing this? I'll take care of 118593 (but if you look at this once 118593 is fixed, make sure you don't have that fix - e.g. revert it locally).
Comment 5 Michael Pyne 2009-01-26 23:31:54 UTC
I certainly agree that fixing bug 118593 would take care of most of the issue.  I'm unclear on KIO internals though: would ensuring a source file is present prevent data corruption with the disk full condition, or did the disk full condition just trigger a logic bug or something?
Comment 6 David Faure 2009-01-27 00:57:30 UTC
Disk full is mostly unrelated to this bug. You'd get the same bug by cancelling the operation during the move, AFAICS.
The point is that overwriting a file with a non-existing one would, in SOME cases (ftp/sftp <-> file, but not most other cases) delete the destination file before starting the data transfer, so in case of a failure to transfer the data you'd lose the data. Which normally (e.g. connection lost) isn't that much of a problem you'd just do the transfer again, but if even the source doesn't exist anymore (bug 118593, or anyone else deleting the source on your ftp server) then  you lost all copies of the data. It's the combination of "ghost source files" and "delete dest before transfer" that is most disastrous.
Comment 7 Dawit Alemayehu 2011-11-05 19:00:58 UTC
Created attachment 65267 [details]
proposed fix

It would be a lot more saner not to delete the destination file before there is any need to do so. The attached patch should resolve this bug for all users that do not uncheck the "Mark partially uploaded files" FTP option in the network preferences dialog. 

Unfortunately, the issue for those that do not want to use partial files, there is no completely safe solution since the destination file has to been opened at some point with the O_TRUNC option. If an error is encountered after that, then there is a potential for the file to be lost. The code in both sftp/ftp ioslaves can be improved to reduce which sorts of errors will cause this, but it won't completely eliminate the potential problem. 

In light of this fact, I do not know why we make marking partial files a configurable option.
Comment 8 David Faure 2011-11-08 09:55:23 UTC
For downloads I guess you're right, we could always use a .part file.

But for uploads, some FTP servers refuse renaming, so uploading to foo.part and then renaming to foo, is not possible. I think this is why we offer to turn off the use of .part files.

Anyway -- if the user disables the use of .part files, he can expect that an interrupted download will indeed remove the destination file before downloading, that's pretty normal. I don't see an issue there, this bug isn't about what happens when disabling that option.

About the patch: doesn't it wrongly go into the error case when bDestExists is false?
Shouldn't that be if (bDestExists && (can't overwrite it)) ?
Comment 9 Dawit Alemayehu 2011-11-09 06:32:57 UTC
Git commit b98876752f1f14fa508a646a0dca2cdd6a30c27f by Dawit Alemayehu.
Committed on 08/11/2011 at 06:44.
Pushed by adawit into branch 'KDE/4.7'.

Do not delete the destination file before there is a need to do so.

CCBUG: 118594
FIXED-IN: 4.7.4

M  +6    -6    kioslave/ftp/ftp.cpp

http://commits.kde.org/kdelibs/b98876752f1f14fa508a646a0dca2cdd6a30c27f