GNOME Bugzilla – Bug 350805
gedit destroys ACLs/SELinux context/xattrs when saving files
Last modified: 2006-08-11 15:57:33 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 :).
Created attachment 70675 [details] [review] patch to add full xattr copying to gedit
Note a downstream report is here: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=202099
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.
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.
> 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.
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.
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.