From owner-freebsd-ports Fri Jul 26 8:50:37 2002 Delivered-To: freebsd-ports@freebsd.org Received: from mx1.FreeBSD.org (mx1.FreeBSD.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 95B2637B400; Fri, 26 Jul 2002 08:50:24 -0700 (PDT) Received: from chiark.greenend.org.uk (chiark.greenend.org.uk [212.135.138.206]) by mx1.FreeBSD.org (Postfix) with ESMTP id 92A5743E4A; Fri, 26 Jul 2002 08:50:22 -0700 (PDT) (envelope-from fanf@chiark.greenend.org.uk) Received: from fanf by chiark.greenend.org.uk with local (Exim 3.12 #1) id 17Y7MD-0006gB-00 (Debian); Fri, 26 Jul 2002 16:50:21 +0100 Date: Fri, 26 Jul 2002 16:50:21 +0100 From: Tony Finch To: freebsd-hackers@freebsd.org, freebsd-ports@freebsd.org Cc: dot@dotat.at Subject: sed -i has difficulty with read-only files Message-ID: <20020726165021.C7551@chiark.greenend.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i Sender: owner-freebsd-ports@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org I discovered this in the following way: fanf2@cyan.csi.cam.ac.uk:/FreeBSD/ports/databases/db3 : 0 ; make ===> Extracting for db3-3.2.9_3,1 >> Checksum OK for bdb/db-3.2.9.tar.gz. >> Checksum OK for bdb/patch.3.2.9.1. >> Checksum OK for bdb/patch.3.2.9.2. ===> db3-3.2.9_3,1 depends on executable: libtool - found ===> Patching for db3-3.2.9_3,1 /usr/bin/sed -i.bak -e 's|-lpthread|"-pthread"|g' /work/ports/FreeBSD/ports/databases/db3/work/db-3.2.9/build_unix/../dist/configure sed: stdout: Bad file descriptor *** Error code 1 This, plus analysis of the code, reveals a number of bugs: (1) The return from freopen() isn't checked; (2) Read-only output files (the original input file and/or any pre-existing backup file) cause permissions problems; (2) Files too large to be mmapped can't be handled; There are two ways to fix the immediate problem, the first which just patches up the existing code but which only fixes the first two bugs, and the second which deals with the backup file very differently, which fixes all of the bugs. Comments, opinions? I'd like to commit patch two if it works satisfactorily, otherwise I'll commit patch one. Patch one: --- main.c Fri Jul 26 15:19:06 2002 +++ main.c.one Fri Jul 26 14:44:48 2002 @@ -407,6 +407,8 @@ /* * Modify a pointer to a filename for inplace editing and reopen stdout + * + * We remove the files before opening them in case of permissions problems */ static int inplace_edit(filename) @@ -434,7 +436,9 @@ } else { strlcpy(backup, *filename, MAXPATHLEN); strlcat(backup, inplace, MAXPATHLEN); - output = open(backup, O_WRONLY | O_CREAT | O_TRUNC); + if (unlink(backup) == -1 && errno != ENOENT) + err(1, "unlink(%s)", backup); + output = open(backup, O_WRONLY | O_CREAT | O_TRUNC, orig.st_mode); if (output == -1) err(1, "open(%s)", backup); } @@ -453,8 +457,15 @@ err(1, "munmap(%s)", *filename); close(input); close(output); - freopen(*filename, "w", stdout); + if (unlink(*filename) == -1) + err(1, "unlink(%s)", *filename); + if (freopen(*filename, "w", stdout) == NULL) + err(1, "freopen(%s)", *filename); + if (chmod(*filename, orig.st_mode) == -1) + err(1, "chmod(%s)", *filename); *filename = strdup(backup); + if (*filename == NULL) + err(1, "malloc"); return 0; } Patch two: --- main.c Fri Jul 26 15:42:32 2002 +++ main.c.two Fri Jul 26 15:43:09 2002 @@ -413,9 +413,7 @@ char **filename; { struct stat orig; - int input, output; char backup[MAXPATHLEN]; - char *buffer; if (lstat(*filename, &orig) == -1) err(1, "lstat"); @@ -425,35 +423,33 @@ } if (*inplace == '\0') { - char template[] = "/tmp/sed.XXXXXXXXXX"; - - output = mkstemp(template); - if (output == -1) - err(1, "mkstemp"); - strlcpy(backup, template, MAXPATHLEN); + /* + * This is a bit of a hack: we use mkstemp() to avoid the + * mktemp() link-time warning, although mktemp() would fit in + * this context much better. We're only interested in getting + * a name for use in the rename(); there aren't any security + * issues here that don't already exist in relation to the + * original file and its directory. + */ + int fd; + strlcpy(backup, *filename, MAXPATHLEN); + strlcat(backup, ".XXXXXXXXXX", MAXPATHLEN); + fd = mkstemp(backup); + if (fd == -1) + errx(1, "could not create backup of %s", *filename); + else + close(fd); } else { strlcpy(backup, *filename, MAXPATHLEN); strlcat(backup, inplace, MAXPATHLEN); - output = open(backup, O_WRONLY | O_CREAT | O_TRUNC); - if (output == -1) - err(1, "open(%s)", backup); } - input = open(*filename, O_RDONLY); - if (input == -1) - err(1, "open(%s)", *filename); - if (fchmod(output, orig.st_mode & ~S_IFMT) == -1) - err(1, "chmod"); - buffer = (char *)mmap(0, orig.st_size, PROT_READ, MAP_SHARED, input, 0); - if (buffer == MAP_FAILED) - err(1, "mmap(%s)", *filename); - if (write(output, buffer, orig.st_size) == -1) - err(1, "write(%s)", backup); - if (munmap(buffer, orig.st_size) == -1) - err(1, "munmap(%s)", *filename); - close(input); - close(output); - freopen(*filename, "w", stdout); + if (rename(*filename, backup) == -1) + err(1, "rename(\"%s\", \"%s\")", *filename, backup); + if (freopen(*filename, "w", stdout) == NULL) + err(1, "freopen(\"%s\")", *filename); + if (chmod(*filename, orig.st_mode & ~S_IFMT) == -1) + err(1, "chmod(\"%s\")", *filename); *filename = strdup(backup); return 0; } Tony. -- f.a.n.finch http://dotat.at/ BISCAY: VARIABLE 3. FAIR. MODERATE WITH FOG PATCHES. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-ports" in the body of the message