From owner-freebsd-bugs Sun Aug 6 12: 0:11 2000 Delivered-To: freebsd-bugs@freebsd.org Received: from freefall.freebsd.org (freefall.FreeBSD.ORG [204.216.27.21]) by hub.freebsd.org (Postfix) with ESMTP id 1FD8B37B75E for ; Sun, 6 Aug 2000 12:00:05 -0700 (PDT) (envelope-from gnats@FreeBSD.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.9.3/8.9.2) id MAA37421; Sun, 6 Aug 2000 12:00:04 -0700 (PDT) (envelope-from gnats@FreeBSD.org) Received: from sushi.CS.Berkeley.EDU (sushi.CS.Berkeley.EDU [128.32.46.243]) by hub.freebsd.org (Postfix) with ESMTP id DE9AB37B804 for ; Sun, 6 Aug 2000 11:58:07 -0700 (PDT) (envelope-from jkuroda@CS.Berkeley.EDU) Received: (from jkuroda@localhost) by sushi.CS.Berkeley.EDU (8.9.3/8.9.3) id LAA24180; Sun, 6 Aug 2000 11:58:06 -0700 (PDT) (envelope-from jkuroda) Message-Id: <200008061858.LAA24180@sushi.CS.Berkeley.EDU> Date: Sun, 6 Aug 2000 11:58:06 -0700 (PDT) From: jkuroda@CS.Berkeley.EDU Reply-To: jkuroda@CS.Berkeley.EDU To: FreeBSD-gnats-submit@freebsd.org X-Send-Pr-Version: 3.2 Subject: bin/20445: restore(8) uses non-varying filenames for /tmp files when restoring given filesystem repeatedly Sender: owner-freebsd-bugs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org >Number: 20445 >Category: bin >Synopsis: restore(8) uses non-varying filenames for /tmp files when restoring given filesystem repeatedly >Confidential: no >Severity: non-critical >Priority: low >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Sun Aug 06 12:00:04 PDT 2000 >Closed-Date: >Last-Modified: >Originator: Jon Masami Kuroda >Release: FreeBSD 3.3-STABLE i386 >Organization: University of California at Berkeley, EECS >Environment: "problem" discovered while cloning filesystems via restore(8) with the -R or -r flags to multiple disks from single dumpfile created via dump(8). # for newroot in 1 2 3 4 5; do cd /mnt/altroot/${newroot}/ && restore rf /scratch/root.dump & done >Description: restore(8) uses the dumpdate of the filesystem being restored to create the filenames for the files in /tmp used to store the names of directories -- /tmp/rst* -- and of their ownership/mode/time stamps -- /tmp/rstmode*. The first file, dirfile is used to store the list of dirs to be created before files are restored into them, the second, modefile, to restore the ownership,mode, and time stamps on those dirs after file restores are finished. The dumpdate is the number of seconds since the Unix Epoch (tm) till the time the filesystem in question was dumped. example: dirs.c: 1 /* * Extract directory contents, building up a directory structure * on disk for extraction by name. * If genmode is requested, save mode, owner, and times for all 5 * directories on the tape. */ void extractdirs(genmode) int genmode; 10 { register int i; register struct dinode *ip; struct inotab *itp; struct direct nulldir; 15 int fd; vprintf(stdout, "Extract directories from tape\n"); (void) sprintf(dirfile, "%srstdir%d", _PATH_TMP, dumpdate); 20 if (command != 'r' && command != 'R') { (void *) strcat(dirfile, "-XXXXXX"); fd = mkstemp(dirfile); } else fd = open(dirfile, O_RDWR|O_CREAT|O_EXCL, 0666); 25 if (fd == -1 || (df = fdopen(fd, "w")) == NULL) { if (fd != -1) close(fd); warn("%s - cannot create directory temporary\nfopen", dirfile); done(1); 30 } if (genmode != 0) { (void) sprintf(modefile, "%srstmode%d", _PATH_TMP, dumpdate); if (command != 'r' && command != 'R') { (void *) strcat(modefile, "-XXXXXX"); 35 fd = mkstemp(modefile); } else fd = open(modefile, O_RDWR|O_CREAT|O_EXCL, 0666); if (fd == -1 || (mf = fdopen(fd, "w")) == NULL) { if (fd != -1) 40 close(fd); warn("%s - cannot create modefile\nfopen", modefile); done(1); } 44 } The dumpdate made of the filenames prior to their creation in lines 19 and 32. Note that after that, a call to mkstemp(3) is made but *only* if neither the "R" or "r" flags are provided to restore(8). Else, the file is created via open(2) with mode 0666, and umask 077 obtained from main.c:93. This introduces one very annoying problem. The name of restore(8)'s temporary files when restore(8) is given the "R" or "r" flags are very predictable -- in fact, they never change. While the use of the O_CREAT and O_EXCL flags to open(2) should prevent a non-privileged user from exploiting an otherwise possible race condition (right?), the problem I found is that this prevents one from using ``restore rf'' or ``restore Rf'' to clone a filesystem to several blank filesystems in parallel from a single dumpfile created earlier by dump(8) as each restore will attempt to create the same filenames in /tmp and only the first one will succeed. (given the example shown above) The rest will fail out like so: restore: /tmp/rstdir965378984 - cannot create directory temporary fopen: File exists restore: /tmp/rstmode965378984 - cannot create modefile fopen: File exists and so prevent one from restoring from one dumpfile to multiple filesystems (such as in FS cloning) simultaneously. It seems that mkstemp(3) was added in revision 1.6.2.1, but mkstemp(3) was used only in certain places, presumably only those places that would introduce nasty race conditions. >How-To-Repeat: * given a number of newfs'd fs's mounted in /mnt/altroot * and a dump of an example root fs to be cloned in /scratch/... # for newroot in 1 2 3 4 5; do cd /mnt/altroot/${newroot}/ && restore rf /scratch/root.dump & done watch the 2nd restore and onward complain about not being to open temporary files correctly. >Fix: Use mkstemp(3) instead of open(2) by itself in extractdirs(), when creating "dirfile" and "modefile". Do not recreate the value of "modefile" from "dumpdate" in setdirmodes(), and instead rely on mkstemp to modify the value of "modefile", back in extractdirs(), which is in an array of "static char"'s. This can easily be accomplished by removing the special casing for the "R" and "r" flags and letting these cases reap the benefits of mkstemp(3) as in other case such as when the "x" flag is used. I scoured the code and the CVS logs and couldn't find a reason for why mkstemp shouldn't be used here. I think it makes sense to on its own merit and also because it makes the code slightly simpler for not having to special case "R" and "r". But, I could be mistaken in this and my patch below could be terribly naive, so take it all with a grain of salt and the 10 coke's I had prior to writing this up. Thanks to Mike Smith for helping me remember that I had been meaning to submit a PR for this for sometime now. A first pass at patch on ${SRC}/sbin/restore/dirs.c This patch tested on 3.4-STABLE and 4.1-RELEASE. *** dirs.c Sun Aug 6 11:46:31 2000 --- dirs.c.moremkstemp Sun Aug 6 11:46:22 2000 *************** *** 148,158 **** vprintf(stdout, "Extract directories from tape\n"); (void) sprintf(dirfile, "%srstdir%d", _PATH_TMP, dumpdate); ! if (command != 'r' && command != 'R') { (void *) strcat(dirfile, "-XXXXXX"); fd = mkstemp(dirfile); ! } else ! fd = open(dirfile, O_RDWR|O_CREAT|O_EXCL, 0666); if (fd == -1 || (df = fdopen(fd, "w")) == NULL) { if (fd != -1) close(fd); --- 148,158 ---- vprintf(stdout, "Extract directories from tape\n"); (void) sprintf(dirfile, "%srstdir%d", _PATH_TMP, dumpdate); ! /* if (command != 'r' && command != 'R') { */ (void *) strcat(dirfile, "-XXXXXX"); fd = mkstemp(dirfile); ! /*} else ! fd = open(dirfile, O_RDWR|O_CREAT|O_EXCL, 0666); */ if (fd == -1 || (df = fdopen(fd, "w")) == NULL) { if (fd != -1) close(fd); *************** *** 161,171 **** } if (genmode != 0) { (void) sprintf(modefile, "%srstmode%d", _PATH_TMP, dumpdate); ! if (command != 'r' && command != 'R') { (void *) strcat(modefile, "-XXXXXX"); fd = mkstemp(modefile); ! } else ! fd = open(modefile, O_RDWR|O_CREAT|O_EXCL, 0666); if (fd == -1 || (mf = fdopen(fd, "w")) == NULL) { if (fd != -1) close(fd); --- 161,171 ---- } if (genmode != 0) { (void) sprintf(modefile, "%srstmode%d", _PATH_TMP, dumpdate); ! /* if (command != 'r' && command != 'R') { */ (void *) strcat(modefile, "-XXXXXX"); fd = mkstemp(modefile); ! /* } else ! fd = open(modefile, O_RDWR|O_CREAT|O_EXCL, 0666); */ if (fd == -1 || (mf = fdopen(fd, "w")) == NULL) { if (fd != -1) close(fd); *************** *** 594,601 **** char *cp; vprintf(stdout, "Set directory mode, owner, and times.\n"); ! if (command == 'r' || command == 'R') ! (void) sprintf(modefile, "%srstmode%d", _PATH_TMP, dumpdate); if (modefile[0] == '#') { panic("modefile not defined\n"); fprintf(stderr, "directory mode, owner, and times not set\n"); --- 594,601 ---- char *cp; vprintf(stdout, "Set directory mode, owner, and times.\n"); ! /* if (command == 'r' || command == 'R') ! (void) sprintf(modefile, "%srstmode%d", _PATH_TMP, dumpdate); */ if (modefile[0] == '#') { panic("modefile not defined\n"); fprintf(stderr, "directory mode, owner, and times not set\n"); >Release-Note: >Audit-Trail: >Unformatted: To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-bugs" in the body of the message