Re: [PATCH] sha1_file.c (write_sha1_from_fd): Detect close failure.

From: Junio C Hamano <junkio@cox.net>
Date: Mar 26, 2007, 20:33 UTC
Message-ID: <7v8xdjwxr4.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <874po8umyk.fsf@rho.meyering.net>

Jim Meyering <jim@meyering.net> writes:

Show 26 quoted lines

> I stumbled across this in the context of the fchmod 0444 patch.
> At first, I was going to unlink and call error like the two subsequent
> tests do, but a failed write (above) provokes a "die", so I made
> this do the same.  This is testing for a write failure, after all.
>
> Signed-off-by: Jim Meyering <jim@meyering.net>
> ---
>  sha1_file.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 0897b94..42aef33 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2155,7 +2155,8 @@ int write_sha1_from_fd(const unsigned char *sha1, int fd, char *buffer,
>  	inflateEnd(&stream);
>
>  	fchmod(local, 0444);
> -	close(local);
> +	if (close(local) != 0)
> +		die("unable to write sha1 file");
>  	SHA1_Final(real_sha1, &c);
>  	if (ret != Z_STREAM_END) {
>  		unlink(tmpfile);
> --
> 1.5.1.rc1.51.gb08b

Hmph. Not catching error from close() is wrong, so this is an improvement, but it still leaves tmpfile on the filesystem, doesn't it?

Looking at write_sha1_file(), which is in a sense more important than this function, it is worse. We should also detect error from close(), nuke the temporary file and return an error there.