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

On Sat, Aug 08, 2020 at 11:44:03AM -0600, Tracey Emery wrote:
> On Sat, Aug 08, 2020 at 09:32:40AM +0200, Stefan Sperling wrote:
> > 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:
> > 
> 
> I'll look in to rearranging to meet this pattern next week. No time this
> weekend, unfortunately.

Sure. Of course if time is an issue you could also commit your fix and
follow-up with cosmetic changes later on. It is more important to get
the memleaks fixed one way or another ;)