GNOME Bugzilla – Bug 632296
fuse: replace+fstat can report incorrect information
Last modified: 2014-10-26 16:00:53 UTC
Files that reside on a samba network share mounted with nautilus get corrupted. I can reproduce the bug on any samba share, hosted on Windows, Ubuntu, etc... even on shares that reside on the same machine: In Nautilus I right-click on a folder and chose "file sharing options" to make it a writable share. Then, again in Nautilus, I navigate to "network" and mount the share by clicking on it and supplying username and password. I created a new file on the share "sample.txt" with the following content: 01 sample line of text 02 sample line of text 03 sample line of text 04 sample line of text 05 sample line of text 06 sample line of text 07 sample line of text 08 sample line of text 09 sample line of text 10 sample line of text Then I removed the lines 02 up to 05 from the file and hit save. Editor shows: 01 sample line of text 06 sample line of text 07 sample line of text 08 sample line of text 09 sample line of text 10 sample line of text When I reopen the file the new content is: 01 sample line of text 06 sample line of text 07 sample line of text 08 sample line of text 09 sample line of text 10 sample line of text 07 sample line of text 08 sample line of text 09 sample line of text 10 sample line of text I tried several editors, several servers providing SAMBA shares (Ubuntu Hardy, Windows Vista, and my Maverick notebook) The result is always the same. A very, very nasty bug. --------------------------------------------------------------------- Architecture: amd64 DistroRelease: Ubuntu 10.10 InstallationMedia: Ubuntu 9.10 "Karmic Koala" - Release amd64 (20091027) Package: samba 2:3.5.4~dfsg-1ubuntu8 PackageArchitecture: amd64 ProcEnviron: LANGUAGE=de_DE:de:en_GB:en PATH=(custom, user) LANG=de_DE.utf8 SHELL=/bin/bash ProcVersionSignature: Ubuntu 2.6.35-22.34-generic 2.6.35.4 RelatedPackageVersions: nautilus 1:2.32.0-0ubuntu1 gvfs 1.6.4-0ubuntu1 SambaClientRegression: Yes Tags: maverick Uname: Linux 2.6.35-22-generic x86_64 UserGroups: adm admin audio cdrom dialout dip fax floppy fuse lpadmin netdev plugdev sambashare tape vboxusers video ------------------------------------------------------------------------ See also : https://bugs.launchpad.net/ubuntu/+source/samba/+bug/660747 ------------------------------------------------------------------------
Mass component change due to BZ cleanup, sorry for the noise.
The same problem was reported for GIMP files, bug #730211. That bug also contains test cases.
The bug Sven reported only affects programs which use the fuse fallback code. The fuse fallback doesn't use g_file_query_info_on_{read,write} when it should which makes open(O_TRUNC)+fstat return the size of the original file rather than the size of the new file for backends which implement g_file_replace with write-to-temp+rename. This apparently confuses some programs which get the size of a file using fstat when they are replacing a file. I don't think this is the same bug as Franco's since in the linked Ubuntu bug he reported that rolling back the kernel version fixed the problem.
Created attachment 281548 [details] [review] fuse: Use query_info_on_{read,write} if a stream is open If a stream is open and it supports query_info_on_{read,write}, use that in preference to querying the file using the path. This causes the correct information to be retrieved when g_file_replace is implemented by writing to a temporary file, rather than the information of the yet-to-be-replaced file.
Created attachment 281549 [details] [review] fuse: Truncate stream rather than path if possible When truncating, if a writable stream is open, truncate that rather than using g_file_replace on the path since that fails to have the desired effect if replace is implemented by writing to a temporary and then renaming.
The attached patches fix the GIMP data loss problem in bug 730211 (for me).
Created attachment 281759 [details] [review] http: Always set the file type to regular Always set the file type to regular so that querying the type of an input stream gives the expected result.
Created attachment 281760 [details] [review] fuse: Don't g_file_append_to unless O_APPEND is given Because g_file_append_to opens an output stream that can only append regardless of the seek position, only use g_file_append_to when O_APPEND is actually specified. This prevents file corruption when the mode is O_WRONLY or O_RDWR without O_APPEND. Note that this means opening a file with a bare O_WRONLY or O_RDWR returns ENOTSUP since there is no equivalent in gvfs. Furthermore, don't attempt to seek when the file mode is O_APPEND since this is unnecessary.
Created attachment 281761 [details] [review] fuse: Track the size of open files properly Previously, if a getattr() for an open file could not get the size information, the file position was returned as the size. This is incorrect if seek or ftruncate had previously been used on the stream. Instead, track the size of the open file and return this value. In addition, use this size in preference to the size returned by g_file_query_info() if a stream is open (this only happens if the stream doesn't support g_file_query_info_on_write()) since the tracked size is more likely to be correct due to implementing g_file_replace by writing to a temporary file. Tracking the size is only necessary for files which are created or opened with O_TRUNC since files opened with O_APPEND will be appended in-place.
Created attachment 281762 [details] [review] fuse: Close stream on release rather than flush Before, if an application created, dup()ed the fd and then closed it, the stream would be closed. When attempting to write to the original fd, reopening the stream would fail since it is an error to open an existing file without either truncating it or appending to it. Close the stream on release() rather than flush() to fix this since release() is called only once per open/create rather than for each close. Since g_output_stream_flush() is called after every write, flush() and fsync() are unnecessary.
The next set of patches makes getting the size more reliable, fixes related issues, and prevents other ways of corrupting data.
Created attachment 281967 [details] [review] fuse: Pass the correct flags when reopening output stream If a file is open for reading and writing, ensure to pass the O_APPEND flag if necessary when reopening the output stream. This allows a write-read-write sequence to work properly if the file is opened for O_APPEND. The O_TRUNC flag is not passed since this would cause a write-read-write sequence to truncate the file on the second write. As it is, the second write fails with ENOTSUP since this kind of operation is not supported by GVFS.
Review of attachment 281548 [details] [review]: Looks good to me, thanks! Just a small typo... ::: client/gvfsfusedaemon.c @@ +809,3 @@ } +/* Call wth fh locked */ "wth" -> "with"
Review of attachment 281549 [details] [review]: Looks good, thanks!
Review of attachment 281759 [details] [review]: It makes sense...
Also rest of the patches looks good to me, thanks for them! However not sure I am the right person to check those patches as this is my first touch with the fuse daemon, but the patches make sense to me as per fuse and gio docs. So feel free to commit them if you haven't any doubts about... Would be good to make also testing suite for the fuse daemon (together with lot of others).
(In reply to comment #16) > Also rest of the patches looks good to me, thanks for them! However not sure I > am the right person to check those patches as this is my first touch with the > fuse daemon, but the patches make sense to me as per fuse and gio docs. So feel > free to commit them if you haven't any doubts about... OK, I'll give them another round of review and testing and commit after we branch master. Thanks for reviewing them. > > Would be good to make also testing suite for the fuse daemon (together with lot > of others). Yes indeed...
Pushed to master as 58db1bd073a8f5f0eaadd9cc56f57c952e43f49e. Thanks for the review! Now awaiting the problems... :-)