GNOME Bugzilla – Bug 738329
xcf_seek_pos() can cause unexpected OS behavior
Last modified: 2014-10-21 20:55:41 UTC
Created attachment 288257 [details] Patched xcf-seek.c Saving a xcf file in app/xcf/xcf-save.c calls xcf_seek_pos() many times. However, I have noticed that pos in xcf_seek_pos(info, pos, error) is sometimes greater than the size of its target file, which causes a critical kernel problem under a very specific environment. The specific environment I mean is: * OS is FreeBSD (probably any version, I can reproduce with 9.3-R and 10.1-RC1) * The file system uses FUSE. * If the target xcf file does not exist, then it's OK. However, if the target xcf file already exists and must be overwritten, the FUSE file system program freezes and cannot be killed even by shutdown. However, the problem may occur in a different situation. The following patch for app/xcf/xcf-seek.c is working for me. 21a22,23 > #include <sys/types.h> > #include <sys/stat.h> 36a39,63 > int fd = fileno(info->fp); > struct stat sb; > > // Check current file size > if (fstat(fd, &sb) < 0) > { > g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errno), > _("Could not fstat XCF file: %s"), > g_strerror (errno)); > > return FALSE; > } > else if (pos > sb.st_size) > { > // Expand file > if (ftruncate(fd, pos) < 0) > { > g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errno), > _("Could not ftruncate XCF file: %s"), > g_strerror (errno)); > > return FALSE; > } > } > If you consider to deal with it, I would appreciate it. Sincerely,
Is this bug 730211 ?
Thank you for your quick comment. I will check if the latest version works next week. (Sorry, FreeBSD's Gimp port version is still 2.8.10.) The FreeBSD's side problem is that it does not return an error even if pos < file size and freezes. I also posted the problem to FreeBSD bugzilla. I hope they also do something for it.
(In reply to comment #3) > Created an attachment (id=288257) [details] > Patched xcf-seek.c > > Saving a xcf file in app/xcf/xcf-save.c calls xcf_seek_pos() many times. > However, I have noticed that pos in xcf_seek_pos(info, pos, error) is sometimes > greater than the size of its target file, which causes a critical kernel > problem under a very specific environment. from http://pubs.opengroup.org/onlinepubs/009695399/functions/fseek.html ... The fseek() function shall allow the file-position indicator to be set beyond the end of existing data in the file. If data is later written at this point, subsequent reads of data in the gap shall return bytes with the value 0 until data is actually written into the gap. ... it seems it is not forbidden to seek past the end of file.
Thanks for your comment. In POSIX, it seems to be OK. However, FreeBSD's man does not define anything about it, same as Mac OS X. (Actually, Mac OS X's man fseek was identical to FreeBSD's.) Therefore, I guess it's better to check somehow the file size. I'll read the bug 730211 and test the latest version of Gimp tomorrow.
I have a doubt: what does it happen loading a corrupt file that says to have a layer starting at 4GB from the beginning? Would it truncate it at that size before failing to read the content?
(In reply to comment #5) > I have a doubt: what does it happen loading > a corrupt file that says to have a layer starting > at 4GB from the beginning? Would it truncate it > at that size before failing to read the content? Sorry, I forgot about reading. The patch should be only for writing. I have just tested 2.8.14 but the problem still remains. Then, tested the following patches. They worked well. % diff xcf-seek.h.org xcf-seek.h 26a27,29 > gboolean xcf_seek_pos_for_wrting (XcfInfo *info, > guint pos, > GError **error); % diff xcf-seek.c.org xcf-seek.c 21a22,23 > #include <sys/types.h> > #include <sys/stat.h> 105a108,153 > > /** > * xcf_seek_pos_for_wrting: > * @info: #XcfInfo structure of the file under work > * @pos: new position, relative to the beginning of the file > * @error: Return location for errors > * > * Changes the file position in the output stream to the given position. > * This should be called only before writing in order to adjust a file size. > * Non-POSIX fseek() does not define behavior when the postion greater than > * the file size is specified. > * > * Returns: %TRUE in case of success; %FALSE otherwise > */ > gboolean > xcf_seek_pos_for_wrting (XcfInfo *info, > guint pos, > GError **error) > { > int fd = fileno(info->fp); > struct stat sb; > > // Check current file size > if (fstat(fd, &sb) < 0) > { > g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errno), > _("Could not fstat XCF file: %s"), > g_strerror (errno)); > > return FALSE; > } > else if (pos > sb.st_size) > { > // Expand file > if (ftruncate(fd, pos) < 0) > { > g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errno), > _("Could not ftruncate XCF file: %s"), > g_strerror (errno)); > > return FALSE; > } > } > > return xcf_seek_pos(info, pos, error); > } % diff xcf-save.c.org xcf-save.c 26c326 < xcf_check_error (xcf_seek_pos (info, offset, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, offset, error)); 333c333 < xcf_check_error (xcf_seek_pos (info, saved_pos, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, saved_pos, error)); 354c354 < xcf_check_error (xcf_seek_pos (info, offset, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, offset, error)); 361c361 < xcf_check_error (xcf_seek_pos (info, saved_pos, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, saved_pos, error)); 973c973 < xcf_check_error (xcf_seek_pos (info, pos, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, pos, error)); 981c981 < xcf_check_error (xcf_seek_pos (info, base + length, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, base + length, error)); 1020c1020 < xcf_check_error (xcf_seek_pos (info, pos, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, pos, error)); 1028c1028 < xcf_check_error (xcf_seek_pos (info, base + length, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, base + length, error)); 1086c1086 < xcf_check_error (xcf_seek_pos (info, pos, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, pos, error)); 1094c1094 < xcf_check_error (xcf_seek_pos (info, base + length, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, base + length, error)); 1169c1169 < xcf_check_error (xcf_seek_pos (info, info->floating_sel_offset, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, info->floating_sel_offset, error)); 1171c1171 < xcf_check_error (xcf_seek_pos (info, saved_pos, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, saved_pos, error)); 1200c1200 < xcf_check_error (xcf_seek_pos (info, offset, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, offset, error)); 1207c1207 < xcf_check_error (xcf_seek_pos (info, saved_pos, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, saved_pos, error)); 1215c1215 < xcf_check_error (xcf_seek_pos (info, offset, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, offset, error)); 1223c1223 < xcf_check_error (xcf_seek_pos (info, offset, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, offset, error)); 1247c1247 < xcf_check_error (xcf_seek_pos (info, info->floating_sel_offset, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, info->floating_sel_offset, error)); 1249c1249 < xcf_check_error (xcf_seek_pos (info, saved_pos, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, saved_pos, error)); 1330c1330 < xcf_check_error (xcf_seek_pos (info, offset, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, offset, error)); 1349c1349 < xcf_check_error (xcf_seek_pos (info, saved_pos, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, saved_pos, error)); 1356c1356 < xcf_check_error (xcf_seek_pos (info, offset, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, offset, error)); 1396c1396 < xcf_check_error (xcf_seek_pos (info, offset, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, offset, error)); 1417c1417 < xcf_check_error (xcf_seek_pos (info, saved_pos, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, saved_pos, error)); 1427c1427 < xcf_check_error (xcf_seek_pos (info, offset, error)); --- > xcf_check_error (xcf_seek_pos_for_wrting (info, offset, error));
Created attachment 288403 [details] Patched xcf-save.c
Created attachment 288404 [details] Patched xcf-seek.h
Comment on attachment 288403 [details] Patched xcf-save.c Added xcf_seek_pos_for_wrting()
Comment on attachment 288403 [details] Patched xcf-save.c Replaced xcf_seek_pos with xcf_seek_pos_for_wrting
Created attachment 288405 [details] Patched xcf-seek.c Added xcf_seek_pos_for_wrting()
Sorry for the mess. I have finished reading bug 730211. It may be related to this, but I'm not sure.
(In reply to comment #2) > Thank you for your quick comment. > I will check if the latest version works next week. > (Sorry, FreeBSD's Gimp port version is still 2.8.10.) > The FreeBSD's side problem is that it does not return an error even if pos < > file size and freezes. > I also posted the problem to FreeBSD bugzilla. > I hope they also do something for it. I think this is the link to the twin bug report: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=194293 and I think this is a bug in their libc or FUSE: seeking past the EOF could happen anyway and a system should not freeze, better if it resizes the file or simply reports an error.
I agree with Massimo, if seeking past the file's end doesn't work there is something *seriously* broken on the system.
So, is it OK for you to leave it? It may affect other file systems which do not define the behavior for fseek() beyond EOF. What if fseek() beyond EOF just moves to EOF? Therefore, I would implement so that no potential errors occur. Padding with dummy data so that fseek() does not seek beyond EOF will be OK, too. > I agree with Massimo, if seeking past the file's end doesn't > work there is something *seriously* broken on the system. I agree that it's FUSE's bug.
I didn't agree to anything, I'm investigating the situation. We already know that all that seeking is bad for other reasons, one of them being weird filesystems behind transparent mounts. GIO had to be fixed for remote files because the sftp:// backend accidentially disabled seeking past the end of the file by accident. I looked up various fopen() manpages. While Linux and OS X guarantee to truncate a file opened with fopen(filename, "w"), there is no such guarantee on freebsd. It would be interesting to know how GIO behaves on freebsd since it uses open() and the flags are lacking O_TRUNC. I have ported Massimo's writing patch in bug 730211 to master, which is a first step towards getting rid more seeking.
Created attachment 288774 [details] [review] This patch gets rid of seeking past the end of the file We can argue about FUSE/OS/whatever brokennes for all eternity, but as a matter of fact we can't fix these from inside GIMP. Instead, avoid the seeking that breaks.
Changing OS to All because this has also happened with FUSE mounts on Linux.
Pushed to master, please test: commit 5a4d865358818fdbae203716bd6e33f86f5905c6 Author: Michael Natterer <mitch@gimp.org> Date: Fri Oct 17 19:12:05 2014 +0200 Bug 738329 - xcf_seek_pos() can cause unexpected OS behavior Change XCF saving to never seek past the end of the partially written file. The only places where we still did this was when skipping the offset tables for layers, channels, levels and tiles. Now we write an all-zero offset table first, and then only seek around in areas of the file that already exist. This also simplifies the code a bit. Changed comments to make it clear what happens. app/xcf/xcf-save.c | 132 ++++++++++++++++++++++++++++++++++++----------------------------- app/xcf/xcf-write.c | 17 +++++++++ app/xcf/xcf-write.h | 35 +++++++++-------- 3 files changed, 109 insertions(+), 75 deletions(-)
Created attachment 288817 [details] [review] The same patch, picked to gimp-2-8 This is the same patch, but against gimp-2-8, please test.
Hiroshi, can you please try my patch against 2.8? It should completely fix the problem.
The patch has been tested on 2-8 now, pushed: commit 29d1695911590cfa70c92602be6e3957828f64f4 Author: Michael Natterer <mitch@gimp.org> Date: Fri Oct 17 19:12:05 2014 +0200 Bug 738329 - xcf_seek_pos() can cause unexpected OS behavior Change XCF saving to never seek past the end of the partially written file. The only places where we still did this was when skipping the offset tables for layers, channels, levels and tiles. Now we write an all-zero offset table first, and then only seek around in areas of the file that already exist. This also simplifies the code a bit. Changed comments to make it clear what happens. (cherry picked from commit 5a4d865358818fdbae203716bd6e33f86f5905c6) app/xcf/xcf-save.c | 133 +++++++++++++++++++++++++++++++++++++++++------------------------ app/xcf/xcf-write.c | 27 +++++++++++++ app/xcf/xcf-write.h | 35 +++++++++-------- 3 files changed, 130 insertions(+), 65 deletions(-)
(In reply to comment #21) > Hiroshi, can you please try my patch against 2.8? It should completely > fix the problem. I have just git clone'd the latest one. However, it seems to be very hard to build it with FreeBSD. Just replacing xcf-save.c, xcf-write.[ch] of 2.8.14 with the latest ones did not build either. If you don't use seek any longer, that will be OK enough. Thank you.
It does seek, just not past the end of the file. You can simply copy all .c and .h files in app/xcf to your tree and build. And you need the 2-8 git branch of course (git checkout gimp-2-8).
(In reply to comment #24) > It does seek, just not past the end of the file. You can simply copy all .c > and .h files in app/xcf to your tree and build. And you need the 2-8 > git branch of course (git checkout gimp-2-8). I probably forgot to specify the version when I used git clone. Anyway, I could barely build the latest version and test it. Saving to xcf DID WORK without freezing my FUSE program. So, the patches seem to be OK. Thank you for your efforts. Just in case, the following is the procedure for building the latest Gimp on my FreeBSD 10.1-RC2: * Install gegl-0.3 by git clone, autogen.sh, gmake, gmake install, mv /usr/local/lib/pkgconfig/* /usr/local/libdata/pkgconfig/ * Install glib-2.40.2 by modifying the existing /usr/ports/devel/glib20. I did some tricks but don't remember. * Install babl-0.1.11 by git clone, autogen.sh, gmake, gmake install, mv /usr/local/lib/pkgconfig/* /usr/local/libdata/pkgconfig/ * git clone git://git.gnome.org/gimp * cd gimp * setenv CFLAGS "-I/usr/local/include -L/usr/local/lib" setenv CPPFLAGS "-I/usr/local/include -L/usr/local/lib" setenv CXXFLAGS "-I/usr/local/include -L/usr/local/lib" setenv LDFLAGS "-llzma" * Remove all zma part in configure.ac. lzma is now in FreeBSD base. * ./autogen.sh --prefix=/usr/local * gmake * gmake install That was pretty painful but worked anyway.
Good to hear, thanks for taking that effort to check! :)
I also tested with 2-8 and xcf was saved successfully. So, both master and 2-8 are OK now.