[Geany-devel] Changed file saving implementation for systems with GIO

[Colomban Wendling]

Le 06/11/2010 01:58, Lex Trotman a écrit :
> On 6 November 2010 10:43, Colomban Wendling <lists.ban at herbesfolles.org> wrote:
>> Le 04/11/2010 21:42, Dimitar Zhekov a écrit :
>>> On Thu, 4 Nov 2010 17:53:47 +0000
>>> Nick Treleaven <nick.treleaven at btinternet.com> wrote:
>>>
>>>> So it seems that function doesn't handle disk exhaustion safely. (But
>>>> this is no worse than before for Geany).
>>>
>>> Before we had safe or unsafe file saving. Let's keep that choice, shall
>>> we? I can write the patch, it's only a few lines.
>>>
>>> For more than 20 years now, the only safe save is to write the data
>>> into a temporary file in the same directory, and then rename it over
>>> the target file. Even if the rename fails, you still have the temporary
>>> file. That changes the onwership and permissions, which is exactly the
>>> behaviour of the safe g_file_set_contents().
>>>
>>> The documentation of g_file_replace() may state "will try to replace
>>> the file in the safest way possible", but I don't believe in miracles,
>>> much less in ones from the glib developers.
>> While searching to report the bug (see below), I found that this isn't
>> that simple. Actually, g_file_replace() itself seems quite safe, but
>> g_file_replace_contents() isn't.
> 
> Well, it doesn't say much in the doc, but why would it not use
> g_file_replace and so be just as safe??
g_file_replace() just /starts/ a replace operation [1]. It returns a
stream to which you probably want to write data, but it doesn't do it.
Getting this stream is safe, and writing to it is also safe.

g_file_replace_contents(), however, also writes to the stream, then
close it.

The problem is that if writing to the stream failed, there is no
(non-hackish) way to abort the final step of the replacement -- moving
the temp file to the original one -- which is done on stream closing.

I know this is a bit confusing at first glance... let's do a small picture:

1) create a stream that points to a temp file
2) write data to this stream
3) move the temp file over the original one

1 is done by g_file_replace();
2 is done with something like g_output_stream_write();
3 is "scheduled" by g_file_replace() and is done upon stream closing
(g_output_stream_close() or g_object_unref() that calls the former).

...and if 1 succeeded, we can't prevent 3 to happen, even if 2 failed.


A small example is better that thousand words:

ostream = g_file_replace(file, ..., &err);
if (ostream) {
  if (! g_output_stream_write_all (ostream, data, length, ..., &err)) {
    /* here's the problem, we don't have
     * g_output_stream_abort_replace() */
  } else {
    /* the final part of the replacement (3) will happen here, when we
     * close the stream.
     * note that the g_object_unref() call above would do this
     * automatically and we do it manually only to get possible
     * errors. */
    success = g_output_stream_close (ostream, &err);
  }
  g_object_unref (ostream);
}

>> g_file_replace() use a probably correct backup strategy, and returns a
>> stream to write to in a safe way. This stream will actually point to a
>> temporary file (or likely, seems there are 2 different strategies), and
>> this file will be moved automatically to the final place when the stream
>> get closed/destructed.
> 
> And then we get the same permissions problem that glibs
> g_file_set_contents has which was where we came in....
Theoretically, yes. But GIO seems to keep attributes quite decently (at
least the permissions are copied, I haven't checked more).

>> The problem is that there is no API yet to abort the replacement, and so
>> prevent the final move to be done.
>>
>> Actually, this is safe if e.g. the caller crashes during write, or
>> anything that prevents the stream to be closed, and so the file to be
>> moved...
>>
>>>> Perhaps you might like to file a bug against GIO. Perhaps first use gdb
>>>> to break on that function just to be absolutely sure.
>>>
>>> Perhaps. But bugs filed to gnome and kde may take years to resolve...
>> The bug is already reported, see
>> https://bugzilla.gnome.org/show_bug.cgi?id=602412
>> Sadly, this bug was reported on 2009-11 and last activity was 2010-04...
>>
> 
> Par for the course, if no one is worried enough to fix it then why
> should the Gnome guys, its open source after all ....
I'd think that maintainers should care a bit more than others, but
basically you're right.

>> Le 04/11/2010 18:53, Nick Treleaven a écrit :
>>> Also, if the argument for backups is set perhaps disk exhaustion is
>>> handled:
>>> http://library.gnome.org/devel/gio/2.25/GFile.html#g-file-replace-contents
>>
>> More or less. The backup file will be kept, but the "original" might
>> still be lost. The data isn't completely lost so, but the user might not
>> notice the backup is here (and it will not be restored).
>>
> 
> I thought the sequence with backup would be:
> 
> 1. write new contents to temp file, uses space
> 2. if successful rename old file to backup, uses no space
> 3. rename temp file to filename, uses no space
> 
> so if the write fails for exhaustion the old file is still there but
> no backup, what we want.
> 
> If one of the renames fails the state could be confusing, with a temp
> file and no old file, or a backup and no new file but that needs a
> filesystem crash to lose metadata right in the rename, *very* rare
> especially with journaled filesystems.
That's (probably, not really checked) right, but that's not really the
concern since the one that move/rename files doesn't know of any write
error (see above).

Hope it's a little bit more clear -- though I'm not sure I'm really
clear at 3 and half AM :D


Regards,
Colomban