Bug 16124
| Summary: | [MFC] [PATCH] Enhancement for 'lpr -r' | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Garance A Drosehn <gad> |
| Component: | bin | Assignee: | Garance A Drosehn <gad> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | CC: | gad |
| Priority: | Normal | ||
| Version: | 3.2-RELEASE | ||
| Hardware: | Any | ||
| OS: | Any | ||
On Fri, 14-Jan-2000 at 14:15:24 -0500, Garance A Drosehn wrote:
>
> >Number: 16124
> >Category: bin
> >Synopsis: [PATCH] Enhancement for 'lpr -r'
> >Confidential: no
Thanks a lot for your work. Hopefully, someone commits it soon...
-Andre
Responsible Changed From-To: freebsd-bugs->alfred This addresses the race condition that led you to suspend PR 11997. Perhaps you'd be interested in looking at this when we're out of feature freeze? :-) . Responsible Changed From-To: alfred->freebsd-bugs Submitter says alfred didn't do anything about it. I wanted to improve some of the comments in this update, and thought I could justify that by also getting it bit closer to 'man style' conventions. Here is the spruced-up version. I am not sure the patch will come thru the email correctly, so I'll also mention that it's available at: ftp://freefour.acs.rpi.edu/pub/bsdlpr/lpr-mv.diff [start] = = = = = = = = = = = = = = = = = [start] --- lpr.c.orig Wed Jan 19 09:25:08 2000 +++ lpr.c Tue May 9 22:34:33 2000 @@ -384,6 +384,81 @@ } if (sflag) printf("%s: %s: not linked, copying instead\n", name, arg); + + if (f) { + /* + * The user wants the file removed after it is copied + * to the spool area, so see if the file can be moved + * instead of copy/unlink'ed. This is much faster and + * uses less spool space than copying the file. This + * can be very significant when running services like + * samba, pcnfs, CAP, et al. + */ + int ret, didlink; + struct stat statb1, statb2; + seteuid(euid); + didlink = 0; + /* + * There are several things to check to avoid any + * security issues. Some of these are redundant + * under BSD's, but are necessary when lpr is built + * under some other OS's (which I do do...) + */ + if (lstat(arg, &statb1) < 0) + goto nohardlink; + if (S_ISLNK(statb1.st_mode)) + goto nohardlink; + 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 (statb1.st_dev != statb2.st_dev) + goto nohardlink; + if (statb1.st_ino != statb2.st_ino) + goto nohardlink; + /* skip if the file already had multiple hard links, + * because changing the owner and access-bits would + * change ALL versions of the file */ + if (statb2.st_nlink > 2) + goto nohardlink; + /* + * if we can access and remove the original file + * without special setuid-ness then this method is + * safe. Otherwise, abandon the move and fall back + * to the (usual) copy method. + */ + seteuid(uid); + ret = access(dfname, R_OK); + if (ret == 0) + ret = unlink(arg); + seteuid(euid); + if (ret != 0) + goto nohardlink; + /* + * unlink of user file was successful. Change the + * owner and permissions, add entries to the control + * file, and skip the file copying step. + */ + chown(dfname, pp->daemon_user, 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 { [end] = = = = = = = = = = = = = = = = = = = = = [end] Responsible Changed From-To: freebsd-bugs->imp I suppose I'll do it. Back on May 15th, Warner gave this patch/pr a ringing
endorsement by changing the status to:
I suppose I'll do it.
I thought I'd follow up at this fourth-month mark, and
see if life is any less busy now then it was back then...
:-)
---
Garance Alistair Drosehn = gad@eclipse.acs.rpi.edu
Senior Systems Programmer or drosih@rpi.edu
Rensselaer Polytechnic Institute
In message <v04210103b5e4b34118ea@[128.113.24.47]> Garance A Drosihn writes: : Back on May 15th, Warner gave this patch/pr a ringing : endorsement by changing the status to: : : I suppose I'll do it. : : I thought I'd follow up at this fourth-month mark, and : see if life is any less busy now then it was back then... : :-) Hey, I have 3 days before the 4 month anniversary :-) Warner Responsible Changed From-To: imp->gad Now that I have commit-ability, it seemed I should take over this PR. I already checked with Warner, and he gave the OK for it. I will try committing my most-recent patch to do this soon, probably later this week. (first I want to see if my edit-pr worked... :-) State Changed From-To: open->suspended Patch applied (with minor style changes). Awaiting MFC, after it has sat in current for awhile. State Changed From-To: suspended->closed New feature has been added to both -current (5.x) and -stable (post 4.2) |
This is a follow-up to PR bin/11997. The goal of this enhancement is to save significant time and disk space processing large print jobs coming via mechanisms like samba. These are often configured to copy the (large) print file into a spool directory, and they then run 'lpr -r' to actually print the job. This change causes 'lpr -r' to simply move the spool file (if possible) instead of copying the entire file (thus giving you two copies in your spool directory for a moment) only to remove the original. The update given in bin/11997 had security issues. This version implements the enhancement without opening any security holes. Note this includes a few checks which are redundant under FreeBSD, but important if compiling this version of lpr under other OS's. Fix: Here is the new, improved patch. This is based on the version of lpr.c found in freebsd-current as of Dec 23rd 1999. ===================================================================--JbM2WLeTM1rrivC984eIWDu5f5AePzC7aQs0qqoCZpZs53E4 Content-Type: text/plain; name="file.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="file.diff" Index: lpr/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/27 21:58:39 @@ -382,10 +382,69 @@ 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; + /* skip if the file already had multiple hard links, + * because changing the owner and access-bits would + * change ALL versions of the file */ + if (statb2.st_nlink > 2) 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, pp->daemon_user, 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);