Kamil Dudka <kdu...@redhat.com> wrote: > On Thursday 12 February 2009 14:27:09 Jim Meyering wrote: >> While rewriting that, >> >> install accepts a new option, --compare (-C): compare each pair of source >> and destination files, and if the destination has identical content and >> any specified owner, group, permissions, and possibly SELinux context, >> then do not modify the destination at all. > Thanks for review! > >> I realized that install must also handle the case in which >> no explicit owner or group option is specified, yet the destination >> owner and/or group do not match the effective ones. >> >> i.e., some file is installed with owner:group of WRONG_USER:WRONG_GROUP, >> yet with proper permissions and matching content, and root runs >> install F /ABS/NAME/OF/F >> >> In that case we *do* want it to unlink the original and perform the >> copy. Currently it would not. This is especially important with >> set-gid and set-uid programs. >> >> > + if (!S_ISREG(src_sb.st_mode) || !S_ISREG(dest_sb.st_mode)) >> > + return true; >> > + >> > + if (src_sb.st_size != dest_sb.st_size >> > + || (dest_sb.st_mode & CHMOD_MODE_BITS) != mode >> > + || (owner_id != (uid_t) -1 && dest_sb.st_uid != owner_id) >> > + || (group_id != (gid_t) -1 && dest_sb.st_gid != group_id)) >> > + return true; >> >> so replacing the owner/group tests with these should fix it: >> || dest_sb.st_uid != (owner_id == (uid_t) -1 ? geteuid () : owner_id) >> || dest_sb.st_gid != (group_id == (gid_t) -1 ? getegid () : group_id) > Fixed and added new test case. > >> But that doesn't take account of the perhaps-unusual case >> in which the destination directory is set-gid (on a file system >> where that matters). > Any idea how to solve this? Should we stat destination directory? Do we really > need this?
I suspect that it's not worth solving at all. stat'ing the destination directory would be a start, but if you do that, you also have to determine file-system- and mount-specific semantics like whether directory-set-gid bits make a difference(nosuid), or whether all files on that partition will always have a particular UID or GID (uid=, gid=). >> Now that I think of security, I'd prefer that if any non-permission mode >> bits (S_ISUID, S_ISGID, S_ISVTX) should be set, we simply short-circuit >> the optimization and always unlink and then copy. > Also fixed and added new test case. Thanks! Just a few more changes: ... > diff --git a/NEWS b/NEWS > index 9de4f25..4f80813 100644 > --- a/NEWS > +++ b/NEWS > @@ -16,6 +16,11 @@ GNU coreutils NEWS -*- > outline -*- > dd accepts iflag=cio and oflag=cio to open the file in CIO (concurrent I/O) > mode where this feature is available. > > + install accepts a new option, --compare (-C): compare each pair of source > + and destination files, and if the destination has identical content and > + any specified owner, group, permissions, and possibly SELinux context, then > + do not modify the destination at all. ... > diff --git a/src/install.c b/src/install.c > index 9bf9eee..0a102bd 100644 > --- a/src/install.c > +++ b/src/install.c > @@ -31,6 +31,7 @@ > #include "cp-hash.h" > #include "copy.h" > #include "filenamecat.h" > +#include "full-read.h" > #include "mkancesdirs.h" > #include "mkdir-p.h" > #include "modechange.h" > @@ -111,6 +112,8 @@ static char *group_name; > static gid_t group_id; > > #define DEFAULT_MODE (S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH) > +#define EXTRA_MODE(mode) ((S_ISUID & (mode)) || (S_ISGID & (mode)) \ > + || (S_ISVTX & (mode))) Please write this as: static bool extra_mode (mode_t mode) { return mode & ~S_IRWXUGO; } ... > + if (copy_only_if_needed && EXTRA_MODE (mode)) > + error (0, 0, _("options -C is ignored if any non-permission mode should " > + "be set")); Please use this wording: error (0, 0, _("the --compare (-C) option is ignored when you" " specify a mode with non-permission bits")); > get_ids (); > > if (dir_arg) > @@ -645,6 +766,9 @@ copy_file (const char *from, const char *to, const struct > cp_options *x) > { > bool copy_into_self; > > + if (copy_only_if_needed && !need_copy (from, to, x)) > + return true; > + > /* Allow installing from non-regular files like /dev/null. > Charles Karney reported that some Sun version of install allows that > and that sendmail's installation process relies on the behavior. > @@ -835,6 +959,9 @@ Mandatory arguments to long options are mandatory for > short options too.\n\ > --backup[=CONTROL] make a backup of each existing destination file\n\ > -b like --backup but does not accept an argument\n\ > -c (ignored)\n\ > + -C, --compare install file, unless target already exists and is\n\ > + the same as the new file, in which case the\n\ > + modification time won't be changed\n\ At first I wanted to say something like in NEWS, but how about this instead, which gives the flavor, but defers to texinfo documentation for the full details: -C, --compare compare each pair of source and destination files, and\n\ and in some cases, do not modify the destination at all\n\ Also, I agree about keeping stack usage low, so have a slight preference for making those two buffers static. +#define CMP_BLOCK_SIZE 65536 enum { CMP_BLOCK_SIZE = 65536 }; In general, prefer an enum; it is then usable in the debugger. + char a_buff[CMP_BLOCK_SIZE]; + char b_buff[CMP_BLOCK_SIZE]; + + size_t size; + while (0 < (size = full_read (a_fd, a_buff, CMP_BLOCK_SIZE))) { + if (size != full_read (b_fd, b_buff, CMP_BLOCK_SIZE)) Please use "sizeof a_buff" and "sizeof b_buff" in the while-loop, rather than CMP_BLOCK_SIZE. + return false; + + if (memcmp (a_buff, b_buff, size) != 0) + return false; + } + + return size == 0; +#undef CMP_BLOCK_SIZE Finally, please remove this #undef. Not only will it no longer be needed, but #undefs are rarely worth the trouble in .c files. _______________________________________________ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils