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.

Thanks.

> 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;
> }

-- 

Tracey Emery