From owner-freebsd-current Fri Dec 24 5:55:24 1999 Delivered-To: freebsd-current@freebsd.org Received: from fledge.watson.org (fledge.watson.org [204.156.12.50]) by hub.freebsd.org (Postfix) with ESMTP id E4B5015230 for ; Fri, 24 Dec 1999 05:55:15 -0800 (PST) (envelope-from robert@cyrus.watson.org) Received: from fledge.watson.org (robert@fledge.pr.watson.org [192.0.2.3]) by fledge.watson.org (8.9.3/8.9.3) with SMTP id IAA28655; Fri, 24 Dec 1999 08:55:06 -0500 (EST) (envelope-from robert@cyrus.watson.org) Date: Fri, 24 Dec 1999 08:55:06 -0500 (EST) From: Robert Watson X-Sender: robert@fledge.watson.org Reply-To: Robert Watson To: Garance A Drosihn Cc: Alfred Perlstein , Andre Albsmeier , Warner Losh , current@FreeBSD.ORG Subject: Re: NO! Re: [PATCHES] Two fixes for lpd/lpc for review and test In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG This is not a comment on your code, which I have not inspected yet, but instead on the idea of the optimization. This is probably not a serious objection, but keep in mind that this optimization does not produce identical behavior in some case. For example, imagine that the user has a number of hard links to the file in question. If the file is copied and then deleted, then the link count is decremented by one, and the data used to print is independent of the original file. However, if a single file system rename is used, the link count remains the same until the end of the print job, and if any of the other references to the same inode are modified, the print job will see inconsistent versions of the file (i.e., it being changed out from under it). I do not know whether the current lpr waits until it finishes copying before returning, but assume so, meaning that once lpr returns, normally, you are guaranteed that unless using the symlink option, you can continue to modify the inode in question with impunity. With this optimization makes that assumption not true. A caveat is already present for the symlink, "-s", option to this effect. I believe this also allows users to present files that start out being within the maximum file size when that is checked, but later add more, allowing users to avoid the file size limit on print jobs. This is presumably already present with "-s", and it's possible that lpd checks for this. That said, I doubt anyone relies on this assumption--printing hard linked files and doing simultaneous modification is probably uncommon. Exceeding the file size limit may already be possible if the check is performed only once (given -s), and if it is not possible in the -s usage, it isn't possible in this situation either. On the other hand, it might be worth a flag to disable this behavior, or at least to document it in the man page, as it isn't quite the same and could cause a nightmare in debugging complex shell scripts if anyone found it the hard way :-). Robert On Thu, 23 Dec 1999, Garance A Drosihn wrote: > At 2:33 AM -0800 12/10/99, Alfred Perlstein wrote: > >Can someone take a look at this? > > > >Basically, it makes the link to the file, if it can unlink the original > >it will then chown the spool file if it can't delete or read the original > >then the user didn't have permission and it backs out. > > Okay, I've finally gotten back to this and come up with an alternate > version, which I think should be pretty safe. It includes some checks > which are probably redundant for freebsd, but which would be necessary > on some other platforms I use this on. > > It's basically Alfred's code, with a few more checks for symlinks > and to make sure race conditions can't trick us. If no problems > are found with this, I'll submit it as a PR follow-on to #bin/11997 > > Pardon the difference in version-numbering, I probably don't have > CVS setup exactly right for version numbers to match freebsd's... > > Index: lpr.c > =================================================================== > RCS file: /Users/cvsdepot/lpr-fbsd/lpr/lpr.c,v > retrieving revision 1.1.1.1 > diff -U5 -r1.1.1.1 lpr.c > --- lpr.c 1999/11/30 16:15:22 1.1.1.1 > +++ lpr.c 1999/12/23 23:31:43 > @@ -382,10 +382,65 @@ > nact++; > continue; > } > if (sflag) > printf("%s: %s: not linked, copying > instead\n", name, arg); > + > + if (f) { > + /* > + * The user wants the file removed after it is copied, > + * so see if it can be mv'ed instead of copy/unlink'ed. > + * This will be much faster and better than copying the > + * file, especially for larger files. Can be very > + * useful for services like samba, pcnfs, CAP, et al. > + */ > + int ret, didlink; > + struct stat statb2; > + seteuid(euid); > + didlink = 0; > + /* don't do this if the user's file is a symlink */ > + if (lstat(arg, &statb) < 0) goto nohardlink; > + if (S_ISLNK(statb.st_mode)) goto nohardlink; > + /* if the attempt to link fails, abandon the move */ > + if (link(arg, dfname) != 0) goto nohardlink; > + didlink = 1; > + /* make sure the user hasn't tried to trick us via > + * any race conditions */ > + if (lstat(dfname, &statb2) < 0) goto nohardlink; > + if (statb.st_dev != statb2.st_dev) goto nohardlink; > + if (statb.st_ino != statb2.st_ino) goto nohardlink; > + > + /* if we can access and remove the given file without > + * special setuid-ness then this method is safe. */ > + seteuid(uid); > + ret = access(dfname, R_OK); > + if (ret == 0) > + ret = unlink(arg); > + seteuid(euid); > + /* the user does not have access to read or remove > + * this file, so abandon the move and fall back to > + * the usual (copy) methods. */ > + if (ret != 0) goto nohardlink; > + > + /* unlink of user file was successful. fixup perms, > + * add entries to control file, and skip copy step */ > + chown(dfname, userid, getegid()); > + chmod(dfname, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP); > + seteuid(uid); > + if (format == 'p') > + card('T', title ? title : arg); > + for (i = 0; i < ncopies; i++) > + card(format, &dfname[inchar-2]); > + card('U', &dfname[inchar-2]); > + card('N', arg); > + nact++; > + continue; > +nohardlink: > + if (didlink) unlink(dfname); > + seteuid(uid); /* restore old uid */ > + } /* end: if (f) */ > + > if ((i = open(arg, O_RDONLY)) < 0) { > printf("%s: cannot open %s\n", name, arg); > } else { > copy(pp, i, arg); > (void) close(i); > =================================================================== > > > --- > Garance Alistair Drosehn = gad@eclipse.acs.rpi.edu > Senior Systems Programmer or drosih@rpi.edu > Rensselaer Polytechnic Institute > > > To Unsubscribe: send mail to majordomo@FreeBSD.org > with "unsubscribe freebsd-current" in the body of the message > Robert N M Watson robert@fledge.watson.org http://www.watson.org/~robert/ PGP key fingerprint: AF B5 5F FF A6 4A 79 37 ED 5F 55 E9 58 04 6A B1 TIS Labs at Network Associates, Safeport Network Services To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message