Date: Fri, 24 Dec 1999 08:55:06 -0500 (EST) From: Robert Watson <robert@cyrus.watson.org> To: Garance A Drosihn <drosih@rpi.edu> Cc: Alfred Perlstein <bright@wintelcom.net>, Andre Albsmeier <andre.albsmeier@mchp.siemens.de>, Warner Losh <imp@village.org>, current@FreeBSD.ORG Subject: Re: NO! Re: [PATCHES] Two fixes for lpd/lpc for review and test Message-ID: <Pine.BSF.3.96.991224084725.28605A-100000@fledge.watson.org> In-Reply-To: <v0421010cb488621c20e1@[128.113.24.47]>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.3.96.991224084725.28605A-100000>