Save button doesn't respect permissions

[Peter Clifton]

File->Save _sometimes_ fails to respect file-permissions.

For example, when modifying a file with permissions:
-r--r--r-- 1 pcjc2 pcjc2 510 2008-12-27 10:23 /home/pcjc2/geda/share/gEDA/sym/analog/resistor-2.sym

The log window often gets the following text:
o_save: Could not open [/home/pcjc2/geda/share/gEDA/sym/analog/resistor-2.sym]
Could NOT save page [/home/pcjc2/geda/share/gEDA/sym/analog/resistor-2.sym]
(Arguably we just need one descriptive message with a reason).

However; on some occasions, the file gets overritten, creating a new file with permissions:
-rw-r--r-- 1 pcjc2 pcjc2 490 2008-12-27 10:29 /home/pcjc2/geda/share/gEDA/sym/analog/resistor-2.sym

Obviously this is not good!

[Peter TB Brett (peter-b)]

Confirmed that this bug is still a problem.

[Ivan Stankovic (istankovic)]

I took a little time to investigate this. Running strace -e gschem when saving a file gives this output
(generated by the f_save function):

lstat("/tmp/foo.sch", {st_mode=S_IFREG|0644, st_size=45, ...}) = 0
   => produced by follow_symlinks

access("/tmp/foo.sch", F_OK) = 0
stat("/tmp/foo.sch", {st_mode=S_IFREG|0644, st_size=45, ...}) = 0
  => produced by g_file_test

access("/tmp/foo.sch~", F_OK) = 0
stat("/tmp/foo.sch~", {st_mode=S_IFREG|0444, st_size=211, ...}) = 0
  => same here, but for the backup file

chmod("/tmp/foo.sch~", 0600) = 0
  => make the previous backup file readable and writable by the user (why?)

rename("/tmp/foo.sch", "/tmp/foo.sch~") = 0
  => this is the interesting part: the existing schematic is renamed to the backup file;
        note that the backup file will have the same permissions as the original file...

chmod("/tmp/foo.sch~", 0444) = 0
  => ... until this chmod executes; so far so good.

Then the weird part:

stat("/tmp/foo.sch", 0x7fffbfe1f2e0) = -1 ENOENT (No such file or directory)
  => we're giving stat the real_filename, which has just been renamed to backup file,
        so it's no surprise that stat returns ENOENT; The comment just above that stat
        says

         /* If there is not an existing file with that name, compute the
          * permissions and uid/gid that we will use for the newly-created file.
          */

        Now I'd say that this is a bug and that stat() should have been called before renaming,
        Or is there a good reason for this behaviour?

But that isn't what really causes the bug we're talking about. The real bug is
that there is no checking for permissions on the existing file. f_save calls o_save which
uses g_file_set_contents which will happily rename() the temporary file to the existing file,
 ignoring permissions and the resulting file will have the same permissions it had before
(by default read+write).

So the fix would be to have a function that checks the permissions and call that function
before calling f_save. Overall, I think f_save is a complex block of code and tries to do a bit
too much (do we really have to mess with chmod, umask, user ids, group ids etc.?).
And I can't really understand why the backup functionality is there in f_save, or libgeda
for that matter...

[Eivind Kvedalen (eivind-z)]

Can't this be solved by adding an access(2) check at the top of f_save? o_save calls g_file_set_contents, which will replace the whole file, but o_save is only called by f_save. However, if o_save is part of the public interface (it is in the prototype.h, so I assume it is), then o_save also needs to be fixed independently of f_save.

[Eivind Kvedalen (eivind-z)]

Implementation as described in my previous comment (both f_save and o_save).

[Peter TB Brett (peter-b)]

On Thursday 30 June 2011 20:41:10 Eivind Kvedalen wrote:
> Implementation as described in my previous comment (both f_save and
> o_save).

Hi,

This patch seems to have a race condition between the permission check
and modifying the file. Is there any obvious way to arrange things so
that it all happens nice and atomically?

Cheers,

                       Peter

--
Peter Brett <email address hidden>
Remote Sensing Research Group
Surrey Space Centre

[Eivind Kvedalen (eivind-z)]

I don't think there is, unless we replace the g_file_set_contents call and write directly to the file, but I think that would introduce the possibility of writing partial files instead (crash, out of disk space, etc). So choosing between the two, the possible race condition is probably the best? (I was aware of it btw, the security hole is mentioned on the access(2) man page).

[Peter TB Brett (peter-b)]

Hmm. For this kind of problem, I usually ask myself: "What would Emacs do?" I will investigate tomorrow.

In the meantime:

- We should be able to successfully save to a file that we have write perms for but not read perms.
- We need to decide whether or not failing to create the backup means that we also don't write the file.
- How do we cope with the case that we don't have write perms for the directory containing the file?
- How do we deal with the possibility that either file or backup or both exist and are not regular files?

I appreciate that these are the sorts of questions that lead to "just scrap it and try again", and I understand if you don't want to deal with that. But I'd happily commit a version of your patch with an added /*! \bug ... */ comment pointing out the race condition.

[Peter TB Brett (peter-b)]

http://stackoverflow.com/questions/1812115/how-to-safely-write-to-a-file

So I think we need to:

1) Create foo.sch# and write new data to it with g_file_set_contents(). If that fails, not a problem.
2) Rename foo.sch to foo.sch~. If that fails, remove foo.sch# and flag an error.
3) Rename foo.sch# to foo.sch. If that fails, attempt to rename foo.sch~ back to foo.sch and remove foo.sch#. Either way, flag an error.
4) Success!

Does that sound plausible?

[Eivind Kvedalen (eivind-z)]

Added \bug ... to o_save and f_save

[Eivind Kvedalen (eivind-z)]

Your suggestion is essentially what happens today, I think. But your solution will also not handle the read-only case; step two/three will effectively "overwrite" foo.sch if it is read-only. So we need to check access somehow; rename will not fail when replacing read-only files. The only atomic way I know of is to actually try to open the file and replace the contents, but then of course you are not guaranteed that the operation will finish successfully.

[Peter TB Brett (peter-b)]

> But your solution will also not handle the read-only case;
> step two/three will effectively "overwrite" foo.sch if it is
> read-only.

Ah, yes, you're quite right. :-|

[Eivind Kvedalen (eivind-z)]

So, should we settle for my patches for the time being?

I'm not sure if backup (and auto-save) should be part of libgeda at all (f_save is complex as it is already), but doing something with that is much more work. Let me know what you think; I can try to refactor the code if you think it is worth it.

[gpleda.org commit robot (gpleda-launchpad-robot)]

Bug was fixed by a commit
git master commit 22985618d999e1b54009b7a3e4f07b27b449f91d
http://git.gpleda.org/?p=gaf.git;a=commit;h=22985618d999e1b54009b7a3e4f07b27b449f91d

commit 22985618d999e1b54009b7a3e4f07b27b449f91d
Author: Peter TB Brett <email address hidden>
Commit: Peter TB Brett <email address hidden>

    libgeda: Make sure File->Save respects file permissions.

    Added g_access calls to f_save and o_save to abort saving if the
    destination file is read-only.

    Closes-bug: lp-698565
    Reviewed-by: Peter TB Brett <email address hidden>

[Peter TB Brett (peter-b)]

On Saturday 02 July 2011 15:39:41 Eivind Kvedalen wrote:
> I'm not sure if backup (and auto-save) should be part of libgeda at
> all

I'm pretty certain they shouldn't.

> I can try to refactor the code if you think it is worth it.

I think that this part of the libgeda API needs a serious rethink, and
possibly a redesign. I don't think it's worth putting a lot of effort
into refactoring the current API.

Thanks for your work on this patch -- I'm sorry for taking so long to
get your patches reviewed & merged.

                            Peter

--
Peter Brett <email address hidden>
Remote Sensing Research Group
Surrey Space Centre