Date: Fri, 26 Jul 2002 16:50:21 +0100 From: Tony Finch <dot@dotat.at> 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>
next in thread | raw e-mail | index | archive | help
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 <dot@dotat.at> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020726165021.C7551>