On Wed, Dec 20, 2023 at 11:24:31PM +1100, Mark Jamsek wrote:
> diff /home/mark/src/got
> commit - 85467924748b0e27f0105bde280878a149df9fc8
> path + /home/mark/src/got
> blob - be366266e526e26c8734719d20d29ed05d463b06
> file + tog/tog.c
> --- tog/tog.c
> +++ tog/tog.c
> @@ -5318,21 +5318,17 @@ create_diff(struct tog_diff_view_state *s)
>  		return got_error_from_errno("malloc");
>  	s->nlines = 0;
>  
> +	if (s->f && fclose(s->f) == EOF)
> +		return got_error_from_errno("fclose");
> +

s->f should be set to NULL here because ...

>  	f = got_opentemp();
> -	if (f == NULL) {
> -		err = got_error_from_errno("got_opentemp");
> -		goto done;
> -	}
> +	if (f == NULL)
> +		return got_error_from_errno("got_opentemp");

... otherwise if we return above, s->f has been closed while
s->f is still non-NULL, and some other code path might try
to close it again.

> +	s->f = f;
> +
>  	tmp_diff_file = got_opentemp();
> -	if (tmp_diff_file == NULL) {
> -		err = got_error_from_errno("got_opentemp");
> -		goto done;
> -	}
> -	if (s->f && fclose(s->f) == EOF) {
> -		err = got_error_from_errno("fclose");
> -		goto done;
> -	}
> -	s->f = f;
> +	if (tmp_diff_file == NULL)
> +		return got_error_from_errno("got_opentemp");
>  
>  	if (s->id1)
>  		err = got_object_get_type(&obj_type, s->repo, s->id1);
>