After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 632296 - fuse: replace+fstat can report incorrect information
fuse: replace+fstat can report incorrect information
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: fuse
1.21.x
Other Linux
: Normal major
: ---
Assigned To: gvfs-maint
gvfs-maint
Depends on:
Blocks:
 
 
Reported: 2010-10-16 14:54 UTC by Franco
Modified: 2014-10-26 16:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fuse: Use query_info_on_{read,write} if a stream is open (6.97 KB, patch)
2014-07-24 06:31 UTC, Ross Lagerwall
committed Details | Review
fuse: Truncate stream rather than path if possible (6.78 KB, patch)
2014-07-24 06:32 UTC, Ross Lagerwall
committed Details | Review
http: Always set the file type to regular (1.25 KB, patch)
2014-07-26 10:49 UTC, Ross Lagerwall
committed Details | Review
fuse: Don't g_file_append_to unless O_APPEND is given (2.96 KB, patch)
2014-07-26 10:49 UTC, Ross Lagerwall
committed Details | Review
fuse: Track the size of open files properly (4.61 KB, patch)
2014-07-26 10:49 UTC, Ross Lagerwall
committed Details | Review
fuse: Close stream on release rather than flush (2.79 KB, patch)
2014-07-26 10:49 UTC, Ross Lagerwall
committed Details | Review
fuse: Pass the correct flags when reopening output stream (1.68 KB, patch)
2014-07-29 16:44 UTC, Ross Lagerwall
committed Details | Review

Description Franco 2010-10-16 14:54:28 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

------------------------------------------------------------------------
Comment 1 Cosimo Cecchi 2012-07-20 16:07:09 UTC
Mass component change due to BZ cleanup, sorry for the noise.
Comment 2 Max Mustermann 2014-07-22 18:55:46 UTC
The same problem was reported for GIMP files, bug #730211. That bug also contains test cases.
Comment 3 Ross Lagerwall 2014-07-22 21:37:03 UTC
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.
Comment 4 Ross Lagerwall 2014-07-24 06:31:49 UTC
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.
Comment 5 Ross Lagerwall 2014-07-24 06:32:31 UTC
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.
Comment 6 Ross Lagerwall 2014-07-24 06:37:30 UTC
The attached patches fix the GIMP data loss problem in bug 730211 (for me).
Comment 7 Ross Lagerwall 2014-07-26 10:49:06 UTC
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.
Comment 8 Ross Lagerwall 2014-07-26 10:49:12 UTC
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.
Comment 9 Ross Lagerwall 2014-07-26 10:49:18 UTC
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.
Comment 10 Ross Lagerwall 2014-07-26 10:49:23 UTC
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.
Comment 11 Ross Lagerwall 2014-07-26 10:51:44 UTC
The next set of patches makes getting the size more reliable, fixes related issues, and prevents other ways of corrupting data.
Comment 12 Ross Lagerwall 2014-07-29 16:44:49 UTC
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.
Comment 13 Ondrej Holy 2014-09-17 10:15:20 UTC
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"
Comment 14 Ondrej Holy 2014-09-17 11:22:36 UTC
Review of attachment 281549 [details] [review]:

Looks good, thanks!
Comment 15 Ondrej Holy 2014-09-17 11:24:22 UTC
Review of attachment 281759 [details] [review]:

It makes sense...
Comment 16 Ondrej Holy 2014-09-23 14:59:40 UTC
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).
Comment 17 Ross Lagerwall 2014-09-27 08:45:11 UTC
(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...
Comment 18 Ross Lagerwall 2014-10-26 16:00:11 UTC
Pushed to master as 58db1bd073a8f5f0eaadd9cc56f57c952e43f49e. Thanks for the review!

Now awaiting the problems... :-)