On Mon, Jan 20, 2014 at 12:51:47PM -0700, Tobias Stoeckmann wrote:
> CVSROOT: /cvs
> Module name: src
> Changes by: tob...@cvs.openbsd.org 2014/01/20 12:51:47
>
> Modified files:
> usr.sbin/lpr/lpd: printjob.c
>
> Log message:
> Fix race condition during symlink check. If there was no symbolic link
> requested, use O_NOFOLLOW, otherwise make sure afterwards that the
> correct file has been referenced (device/inode supplied by 'S' line).
>
> diff basically from and ok millert@, ok guenther@
This change has broken remote printing for me.
With a printcap like this:
lp|remote line printer:\
:lp=:rm=10.197.84.33:rp=lp:sd=/var/spool/output:lf=/var/log/lpd-errs:
I get the following when trying to send jobs to the remote printer:
# lpr /tmp/ppuf.pdf
# lpq
ted.stsp.name: no space on remote; waiting for queue to drain
Rank Owner Job Files
Total Size
1st root 87 /tmp/ppuf.pdf
1537701 bytes
If I revert the above commit, it works again:
# lpq
ted.stsp.name: sending to 10.197.84.33
Rank Owner Job Files
Total Size
1st stsp 90 /tmp/ppuf.pdf
1537701 bytes
FWIW, this diff reverts the change.
I cannot spot the problem in the diff. Any ideas?
Index: printjob.c
===================================================================
RCS file: /cvs/src/usr.sbin/lpr/lpd/printjob.c,v
retrieving revision 1.51
diff -u -p -r1.51 printjob.c
--- printjob.c 20 Jan 2014 19:52:45 -0000 1.51
+++ printjob.c 7 Feb 2014 18:23:36 -0000
@@ -226,9 +226,6 @@ again:
continue;
errcnt = 0;
restart:
- fdev = (dev_t)-1;
- fino = (ino_t)-1;
-
(void)lseek(lfd, pidoff, SEEK_SET);
if ((i = snprintf(line, sizeof(line), "%s\n", q->q_name)) >=
sizeof(line) || i == -1)
@@ -541,30 +538,20 @@ print(int format, char *file)
int fd, status, serrno;
int n, fi, fo, p[2], stopped = 0, nofile;
- if (fdev != (dev_t)-1 && fino != (ino_t)-1) {
- /* symbolic link */
- PRIV_START;
- fi = safe_open(file, O_RDONLY, 0);
- PRIV_END;
- if (fi != -1) {
- /*
- * The symbolic link should still point to the same file
- * or someone is trying to print something he shouldn't.
- */
- if (fstat(fi, &stb) == -1 ||
- stb.st_dev != fdev || stb.st_ino != fino) {
- close(fi);
- return(ACCESS);
- }
- }
- } else {
- /* regular file */
- PRIV_START;
- fi = safe_open(file, O_RDONLY|O_NOFOLLOW, 0);
+ PRIV_START;
+ if (lstat(file, &stb) < 0 || (fi = safe_open(file, O_RDONLY, 0)) < 0) {
PRIV_END;
- }
- if (fi == -1)
return(ERROR);
+ }
+ PRIV_END;
+ /*
+ * Check to see if data file is a symbolic link. If so, it should
+ * still point to the same file or someone is trying to print
+ * something he shouldn't.
+ */
+ if (S_ISLNK(stb.st_mode) && fstat(fi, &stb) == 0 &&
+ (stb.st_dev != fdev || stb.st_ino != fino))
+ return(ACCESS);
if (!SF && !tof) { /* start on a fresh page */
(void)write(ofd, FF, strlen(FF));
tof = 1;
@@ -888,30 +875,20 @@ sendfile(int type, char *file)
char buf[BUFSIZ];
int sizerr, resp;
- if (fdev != (dev_t)-1 && fino != (ino_t)-1) {
- /* symbolic link */
- PRIV_START;
- f = safe_open(file, O_RDONLY, 0);
+ PRIV_START;
+ if (lstat(file, &stb) < 0 || (f = safe_open(file, O_RDONLY, 0)) < 0) {
PRIV_END;
- if (f != -1) {
- /*
- * The symbolic link should still point to the same file
- * or someone is trying to print something he shouldn't.
- */
- if (fstat(f, &stb) == -1 ||
- stb.st_dev != fdev || stb.st_ino != fino) {
- close(f);
- return(ACCESS);
- }
- }
- } else {
- /* regular file */
- PRIV_START;
- f = safe_open(file, O_RDONLY|O_NOFOLLOW, 0);
- PRIV_END;
- }
- if (f == -1)
return(ERROR);
+ }
+ PRIV_END;
+ /*
+ * Check to see if data file is a symbolic link. If so, it should
+ * still point to the same file or someone is trying to print something
+ * he shouldn't.
+ */
+ if (S_ISLNK(stb.st_mode) && fstat(f, &stb) == 0 &&
+ (stb.st_dev != fdev || stb.st_ino != fino))
+ return(ACCESS);
if ((amt = snprintf(buf, sizeof(buf), "%c%lld %s\n", type,
(long long)stb.st_size, file)) >= sizeof(buf) || amt == -1)
return (ACCESS); /* XXX hack */
