From: Mark Jamsek <mark@jamsek.com>
Subject: Re: got: plug got_diffreg_result leak
To: Game of Trees <gameoftrees@openbsd.org>
Date: Sat, 7 Jan 2023 23:21:03 +1100

On 23-01-07 01:09PM, Stefan Sperling wrote:
> 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.

Good save! Thanks :)

I've committed this with your fix.

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68