From: Stefan Sperling <stsp@stsp.name>
Subject: Re: got: plug got_diffreg_result leak
To: Mark Jamsek <mark@jamsek.com>
Cc: Game of Trees <gameoftrees@openbsd.org>
Date: Sat, 7 Jan 2023 13:09:42 +0100

On Sat, Jan 07, 2023 at 10:52:09PM +1100, Mark Jamsek wrote:
> We leak the diff result when we fail to call got_diffreg_result_free()
> in two of the three callers of got_diffreg_output() in lib/diff.c.
> 
> If got_diffreg_output() fails, we goto done, which jumps straight over
> got_diffreg_result_free().
> 
> I opted for the fix of least churn, which simply raises the 'done:'
> label, making it the same as the third got_diffreg_output() caller.
> 
> We could instead remove all goto in these routines because disposing of
> got_diffreg_result, which is allocated at the end, is the only cleanup
> we do. So all prior goto's could just return instead. OTOH, the chosen
> approach is more future-proof incase we end up adding further resources
> that need deallocating, and, as mentioned, a lot less churn.

Good catch. I agree with moving the 'done' labels up.

> +done:
>  	if (resultp && err == NULL)
>  		*resultp = result;
> -	else {
> +	else if (result) {

If we do this we should also initialize result to NULL upon entry
to the function. Otherwise the above check could go wrong.

>  		free_err = got_diffreg_result_free(result);
>  		if (free_err && err == NULL)
>  			err = free_err;
>  	}
> -done:
> +
>  	return err;
>  }
>  
> @@ -276,6 +277,7 @@ diff_blob_file(struct got_diffreg_result **resultp,
>  			goto done;
>  	}
>  
> +done:
>  	if (resultp && err == NULL)
>  		*resultp = result;
>  	else if (result) {

In this case it is already initialized.