Date: Sat, 27 Jul 2002 07:12:24 +0200 From: Cyrille Lefevre <cyrille.lefevre@laposte.net> To: Tony Finch <dot@dotat.at> Cc: freebsd-hackers@FreeBSD.ORG, freebsd-ports@FreeBSD.ORG Subject: Re: sed -i has difficulty with read-only files Message-ID: <20020727051224.GA7996@gits.dyndns.org> In-Reply-To: <20020726165021.C7551@chiark.greenend.org.uk> References: <20020726165021.C7551@chiark.greenend.org.uk>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Jul 26, 2002 at 04:50:21PM +0100, Tony Finch wrote: [snip] > 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) > @@ -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) fp = freopen(... > + err(1, "freopen(%s)", *filename); > + if (chmod(*filename, orig.st_mode) == -1) fchmod(fileno(fp), ... > + 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); IMHO, both the old and the newer code are wrong. they should use getenv("TMPDIR") if any and certainly not the current directory. there is probably more room in ${TMPDIR:-/var/tmp} then in the current directory. imagine that the current fs is nearly full... for the rest of the code, same assertion than above. Cyrille. -- Cyrille Lefevre mailto:cyrille.lefevre@laposte.net To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-hackers" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020727051224.GA7996>