From: Stefan Sperling <stsp@stsp.name>
Subject: Re: plug memory leak in got_pack_dump_delt_chain_to_file()
To: Omar Polo <op@omarpolo.com>
Cc: "Todd C. Miller" <millert@openbsd.org>, gameoftrees@openbsd.org
Date: Tue, 13 Feb 2024 21:05:20 +0100

On Tue, Feb 13, 2024 at 08:57:52PM +0100, Omar Polo wrote:
> On 2024/02/13 12:51:44 -0700, Todd C. Miller <millert@openbsd.org> wrote:
> > Won't this cause the uninitialized contents of accum_buf to be
> > written out?  I think this can happen for other error paths as well.
> 
> ah yeah, I forgot to ask about this.  accum_size is still zero so we
> don't end up reading the uninitialized memory.  However, we may clobber
> err on fwrite, and we could handle it slightly better.
> 
> what about this?

Yes, fine with me. That is slightly better indeed.

> diff -s /home/op/w/got
> commit - 8c8d22ba3f20c7468ada36f3f21837de6483fa8c
> path + /home/op/w/got (staged changes)
> blob - c5c1be69a232bf7e5a17ad4a0e06d81efbc1d8f8
> blob + 2bb13ec800c4d056689ef15d9179ab3d8151a359
> --- lib/pack.c
> +++ lib/pack.c
> @@ -1643,12 +1643,16 @@ got_pack_dump_delta_chain_to_file(size_t *result_size,
>  			}
>  		}
>  	}
>  
>  done:
>  	free(base_buf);
> +	if (err) {
> +		free(accum_buf);
> +		accum_buf = NULL;
> +	}
>  	if (accum_buf) {
>  		size_t len = fwrite(accum_buf, 1, accum_size, outfile);
>  		free(accum_buf);
>  		if (len != accum_size)
>  			err = got_ferror(outfile, GOT_ERR_IO);
>  	}
> 
>