On 2023/12/20 23:24:31 +1100, Mark Jamsek <mark@jamsek.com> wrote:
> Omar Polo <op@omarpolo.com> wrote:
> > 
> > by the way, create_diff() has a similar problem with f not being closed
> > if the second got_opentemp() or fclose() fail.  A similar treatment
> > could be applied to it too.
> > 
> 
> Thanks for letting me know about this one!
> 
> We could fix it a few ways, and I'm partial to either of the below so
> whichever you prefer is ok with me :)
> 
> As alluded to in your previous email, we can just cleanup f on error in
> create_diff() if we haven't already assigned f to s->f as in the topmost
> diff.
> 
> Alternatively, because s->f is always cleaned up in close_diff_view() on
> error or otherwise, we can just reorder things to close s->f before
> opening f and then assign f to s->f before calling got_opentemp() the
> second time, thereby leaving cleanup to close_diff_view() as in the
> second below diff (which also changes a few goto done to return err as
> there's no cleanup needed in such cases).

I prefer the second diff, the first one depends too much on the order of
the operations at the top of the function.

> [...]
> --- 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");

I'm not sure, but should we also set s->f = NULL before returning here?

Realistically speaking this can fail only if we have a bug elsewhere and
end up closing the fd backing this FILE.  It's great that we catch the
error, but then we leave s->f != NULL and attempt to fclose() it again,
which I'm not sure it's allowed.

> +
>  	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");
> +	s->f = f;
> +

just a nit, you can replace f with s->f and remove the f variable
entirely.

ok op@ either way.

Thanks!