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:[EMAIL PROTECTED]
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-hackers" in the body of the message
