Hi Ben, Ben Cornett wrote on Mon, Apr 28, 2014 at 08:49:36PM +0000:
> I agree that one probably never gets a short write there in practice, > but if there is going to be a loop, it may as well be correct. That's what i think, too. So i finally committed your patch, thanks for sending it. Yours, Ingo > On Sat, Apr 26, 2014 at 10:52:50PM +0200, Ingo Schwarze wrote: >> On Tue, Apr 22, 2014 at 01:29:03AM +0000, Ben Cornett wrote: >> >>> The following corrects the termination condition on the write >>> loop in copyfile. >> >> From inspection, i say the patch is clearly correct. >> >> I was unable to find any condition where the bug might actually >> hit: The file descriptor `to' always point to "/etc/ptmp", >> that's hardcoded, and /etc is always on the root file system. >> So as far as i understand, that's a blocking write to a local >> file system, which will either fail outright and return -1 (ENOSPC, >> EIO, ...) or fully succeed, but never write less than requested. >> Then again, if people remount /etc from NFS after booting (surely >> a bad idea), who knows what might happen? >> >> Anyway, i'd like to commit this, if only for clarity... >> >> OK? >> Ingo >> >> >> Index: usr.sbin/vipw/vipw.c >> =================================================================== >> RCS file: /cvsroot/OpenBSD/src/usr.sbin/vipw/vipw.c,v >> retrieving revision 1.16 >> diff -u -p -r1.16 vipw.c >> --- usr.sbin/vipw/vipw.c 19 Aug 2011 20:53:36 -0000 1.16 >> +++ usr.sbin/vipw/vipw.c 22 Apr 2014 01:17:20 -0000 >> @@ -100,7 +100,7 @@ copyfile(int from, int to, struct stat * >> if (fstat(from, sb) == -1) >> pw_error(_PATH_MASTERPASSWD, 1, 1); >> while ((nr = read(from, buf, sizeof(buf))) > 0) >> - for (off = 0; off < nr; nr -= nw, off += nw) >> + for (off = 0; nr > 0; nr -= nw, off += nw) >> if ((nw = write(to, buf + off, nr)) < 0) >> pw_error(_PATH_MASTERPASSWD_LOCK, 1, 1); >> if (nr < 0)
