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 350805 - gedit destroys ACLs/SELinux context/xattrs when saving files
gedit destroys ACLs/SELinux context/xattrs when saving files
Status: RESOLVED FIXED
Product: gedit
Classification: Applications
Component: general
2.15.x
Other Linux
: Normal normal
: ---
Assigned To: Gedit maintainers
Gedit maintainers
Depends on:
Blocks:
 
 
Reported: 2006-08-10 20:28 UTC by James Antill
Modified: 2006-08-11 15:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch to add full xattr copying to gedit (3.18 KB, patch)
2006-08-10 20:29 UTC, James Antill
reviewed Details | Review
xattr patch, with recommended changes (3.54 KB, patch)
2006-08-11 15:18 UTC, James Antill
committed Details | Review

Description James Antill 2006-08-10 20:28:39 UTC
gedit saves files via. the traditional method of:

x = open_tmp_file()
write_data(x)
rename_orig_backup();
rename_tmp_orig();

...but it doesn't copy the xattrs from the orig. to the tempfile, so ACLs,
xattrs and SELinux context info. is all lost.



 I've created a patch for gedit which fixes this.
 NOTE: You need to call:

autoconf
autoheader
automake

...or it'll silently not do anything :).
Comment 1 James Antill 2006-08-10 20:29:26 UTC
Created attachment 70675 [details] [review]
patch to add full xattr copying to gedit
Comment 2 Ray Strode [halfline] 2006-08-10 20:37:24 UTC
Note a downstream report is here:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=202099
Comment 3 Paolo Borelli 2006-08-10 21:31:43 UTC
thanks a lot for the patch... sorry if the following questions are naive, but I know little about xattr etc..

If I read it correctly, things will continue to work even when saving to a filesystem which doesn't have xattr support, right?


+#ifdef HAVE_LIBATTR
+static int all_xattrs(const char *xattr, struct error_context *err)
+{ /* Save everything: user/root xattrs, SELinux, ACLs. */
+  (void)xattr;
+  (void)err;
+  return 1;
+}
+#endif

what are the void casts needed for? Also, this bit of code doesn't follow coding style.


+			gedit_debug_message (DEBUG_SAVER, "could not set xattrs");
+
+			goto out; /* is it better to save or not save? */
+                }


To save or not to save? Well, I am not sure either... Which are the reasons which could cause this to happen? What is it stored in the xattrs usually? if we created a backup and it has not the same xattrs do we risk to bad things like people being able to read the backup despite not being allowed to read the original file? 

What I am sure however, is that if you give up saving like you do in the patch (goto out), you should also set the error with

			g_set_error (&saver->priv->error,
				     GEDIT_DOCUMENT_ERROR,
				     GEDIT_DOCUMENT_ERROR_CANT_CREATE_BACKUP,
				     "No backup created");

otherwise the error will not bubble up in the UI.

Comment 4 Ray Strode [halfline] 2006-08-10 22:11:49 UTC
presumably, the (void)xattr; (void)err; was an attempt to stop compiler warnings.

It might be better to use G_GNUC_UNUSED on the parameters wor just pass 

(int (*) (const char *, struct error_context *)) gtk_true

for the callback and drop the function entirely.
Comment 5 James Antill 2006-08-11 14:59:18 UTC
> If I read it correctly, things will continue to work even when saving to a
> filesystem which doesn't have xattr support, right?

 Yes, they should do. Although I have seen interesting things like lsetxattr() on a symlink always returns EPERM, but EOPNOTSUPP does seem to work when they aren't supported on ext3 (I'm assuming that's what reiserfs/JFS/XFS/etc. return too).

> To save or not to save? Well, I am not sure either... Which are the reasons
> which could cause this to happen? What is it stored in the xattrs usually? if
> we created a backup and it has not the same xattrs do we risk to bad things
> like people being able to read the backup despite not being allowed to read
> the original file? 

 The three canonical things in xattrs now are: ACLs, SELinux context and general user/root xattrs (from beagle say ... but also user.mime_type).
 In theory we could probably continue if user.* or root.* fails, but if the ACLs/SELinux context fail then that's altered the permissions for the file in a big way. And the API doesn't really allow us to distinguish between the former and the later, so I just left it as always fail.

> What I am sure however, is that if you give up saving like you do in the patch
> (goto out), you should also set the error with

 Ok, I'll upload a fixed patch for that (I assume I should be doing unlink/close like the fchmod fail path too).

> presumably, the (void)xattr; (void)err; was an attempt to stop compiler
> warnings.
> 
> It might be better to use G_GNUC_UNUSED on the parameters wor just pass 

 Yeh, for the callback, G_GNUC_UNUSED would be better ... my bad.
Comment 6 James Antill 2006-08-11 15:18:02 UTC
Created attachment 70722 [details] [review]
xattr patch, with recommended changes

 This fixes the error path and uses G_GNUC_UNUSED instead of cast to void.
Comment 7 Paolo Borelli 2006-08-11 15:57:33 UTC
Ok, I went ahead and committed it (after fixing up the indentation ;).

I have no way to test it though, so please upstream any error your users report



Thanks a lot for the patch.