From: Stefan Sperling <stsp@stsp.name>
Subject: Re: tog free fixes
To: Tracey Emery <tracey@traceyemery.net>
Cc: gameoftrees@openbsd.org
Date: Sat, 8 Aug 2020 09:32:40 +0200

On Fri, Aug 07, 2020 at 02:13:17PM -0600, Tracey Emery wrote:
> A few missed frees in tog were noticed by cppcheck.
> 
> Ok?
> -- 
> Tracey Emery
> 
> diff 6d7260fd5af3b577d020bdbcb038b6c245216d5e /home/tracey/src/got
> blob - be1e9bf1dfdb2efc4b2b139be32285d1519c4a3c
> file + tog/tog.c
> --- tog/tog.c
> +++ tog/tog.c
> @@ -3049,8 +3049,10 @@ get_filestream_info(size_t *filesize, int *nlines, off
>  		return got_error_from_errno("calloc");
>  
>  	fread(buf, 1, len, infile);
> -	if (ferror(infile))
> +	if (ferror(infile)) {
> +		free(buf);
>  		return got_error_from_errno("fread");
> +	}
>  
>  	i = 0;
>  	if (line_offsets && nlines) {
> @@ -3059,8 +3061,11 @@ get_filestream_info(size_t *filesize, int *nlines, off
>  			noffsets = 1;
>  			*nlines = 1;
>  			*line_offsets = calloc(1, sizeof(**line_offsets));
> -			if (*line_offsets == NULL)
> +			if (*line_offsets == NULL) {
> +				free(buf);
> +				free(*line_offsets);

Please also set output parameter *line_offsets to NULL here.
NULL pointer derefs are easier to track down than potential use-after-free.
		
>  				return got_error_from_errno("calloc");
> +			}
>  				/* Skip forward over end of first line. */
>  			while (i < len) {
>  				if (buf[i] == '\n')
> @@ -3094,13 +3099,18 @@ get_filestream_info(size_t *filesize, int *nlines, off
>  		}
>  	}
>  
> -	if (fflush(infile) != 0)
> +	if (fflush(infile) != 0) {
> +		free(buf);
> +		free(*line_offsets);
>  		return got_error_from_errno("fflush");
> +	}
>  	rewind(infile);
>  
>  	if (filesize)
>  		*filesize = len;
>  
> +	free(buf);
> +	free(*line_offsets);
>  	return NULL;
>  }


To make error handling consistent in all the cases we could use this pattern:

func(char **p)
{
	char *buf;

	/* Code that allocates buf and *p, and does other stuff */

	err = f();
	if (err)
		goto done;

	[...]

done:
	free(buf)
	if (err) {
		free(*p);
		*p = NULL;
	}
	return err;
}