Skip site navigation (1)Skip section navigation (2)
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>