From: Kyle Ackerman <kackerman0102@gmail.com>
Subject: Re: Diff memory leak
To: Mark Jamsek <mark@jamsek.com>
Cc: Stefan Sperling <stsp@stsp.name>, gameoftrees@openbsd.org
Date: Fri, 31 May 2024 08:51:11 -0500

Mark Jamsek <mark@jamsek.com> writes:

> Stefan Sperling <stsp@stsp.name> wrote:
>> On Mon, May 27, 2024 at 11:58:02PM -0500, Kyle Ackerman wrote:
>> > Hello all,
>> > 
>> > There is a memory leak within `got diff` that leaks line metadata.
>> > 
>> > ******** Start dump got *******
>> > M=8 I=1 F=1 U=1 J=2 R=0 X=0 C=0x449af07a cache=0 G=4096
>> > Leak report:
>> >                  f     sum      #    avg
>> >      0xd551a917735     512      1    512 addr2line -e /home/kyle/bin/got 0x4e735
>> >      0xd57f17f70b0      64      1     64 addr2line -e /usr/lib/libc.so.100.1 0xa40b0
>> >      0xd57f182c4d1     112      7     16 addr2line -e /usr/lib/libc.so.100.1 0xd94d1
>> >      0xd57f18158d2    1024      1   1024 addr2line -e /usr/lib/libc.so.100.1 0xc28d2
>> >      0xd57f17aec53   69632      1  69632 addr2line -e /usr/lib/libc.so.100.1 0x5bc53
>> > 
>> > ******** End dump got *******
>> 
>> Could you please show what these addr2line commands are printing?
>> 
>> > Here is the diff to patch the memory leak
>> > 
>> > diff /home/kyle/src/got
>> > commit - c89c70b628c1825024e333214392011409d71184
>> > path + /home/kyle/src/got
>> > blob - 245df76cba6ccd1d6c155ecbb3632f386db7f3e1
>> > file + lib/diff.c
>> > --- lib/diff.c
>> > +++ lib/diff.c
>> > @@ -1282,6 +1282,8 @@ diff_objects_as_trees(struct got_diff_line **lines, si
>> >  	if (want_linemeta) {
>> >  		*lines = arg.lines; /* was likely re-allocated */
>> >  		*nlines = arg.nlines;
>> > +	} else {
>> > +		free(arg.lines);
>> >  	}
>> >  done:
>> >  	if (tree1)
>> 
>> If this fixes the above leak report then I wonder if the real problem isn't
>> elsewhere. As far as I understand, arg.lines is set to NULL when want_linemeta
>> is false. And if arg.lines is NULL, the expectation is that no line offset
>> information will be produced by the diff driver and callbacks, and args.lines
>> will remain as NULL and won't need to be freed.
>> 
>> Is there code somewhere which reallocs args.lines even though it is NULL?
>> This would be where a fix is needed, I believe. I cannot spot such code
>> in lib/diff.c. Can you perhaps pin-point something?
>
> Kyle's right in that we leak this lines array because it gets allocated
> in diffreg.c:got_diffreg_output() but stsp's right in that the problem
> is elsewhere with some incorrect NULL pointer checks.
>
> The below diff plugs the leak for me; can you please confirm it does for
> you, too, Kyle?
This plugs the leak for me! Thanks!
>
>
> diff /home/mark/src/got
> commit - c6458e88f5a9085ec9206a60b93a713138b9b2fa
> path + /home/mark/src/got
> blob - 245df76cba6ccd1d6c155ecbb3632f386db7f3e1
> file + lib/diff.c
> --- lib/diff.c
> +++ lib/diff.c
> @@ -146,12 +146,15 @@ diff_blobs(struct got_diff_line **lines, size_t *nline
>  	off_t outoff = 0;
>  	int n;
>  
> -	if (lines && *lines && *nlines > 0)
> -		outoff = (*lines)[*nlines - 1].offset;
> -	else if (lines) {
> -		err = add_line_metadata(lines, nlines, 0, GOT_DIFF_LINE_NONE);
> -		if (err)
> -			goto done;
> +	if (lines && *lines) {
> +		if (*nlines > 0)
> +			outoff = (*lines)[*nlines - 1].offset;
> +		else {
> +			err = add_line_metadata(lines, nlines,
> +			    0, GOT_DIFF_LINE_NONE);
> +			if (err != NULL)
> +				goto done;
> +		}
>  	}
>  
>  	if (resultp)
> @@ -218,7 +221,7 @@ diff_blobs(struct got_diff_line **lines, size_t *nline
>  		if (n < 0)
>  			goto done;
>  		outoff += n;
> -		if (lines) {
> +		if (lines && *lines) {
>  			err = add_line_metadata(lines, nlines, outoff,
>  			    GOT_DIFF_LINE_BLOB_MIN);
>  			if (err)
> @@ -230,7 +233,7 @@ diff_blobs(struct got_diff_line **lines, size_t *nline
>  		if (n < 0)
>  			goto done;
>  		outoff += n;
> -		if (lines) {
> +		if (lines && *lines) {
>  			err = add_line_metadata(lines, nlines, outoff,
>  			    GOT_DIFF_LINE_BLOB_PLUS);
>  			if (err)
> blob - 5f1c310ce05497519a9838e46b29d28af595d0b4
> file + lib/diffreg.c
> --- lib/diffreg.c
> +++ lib/diffreg.c
> @@ -272,13 +272,13 @@ got_diffreg_output(struct got_diff_line **lines, size_
>  	switch (output_format) {
>  	case GOT_DIFF_OUTPUT_UNIDIFF:
>  		rc = diff_output_unidiff(
> -		    lines ? &output_info : NULL, outfile, &info,
> +		    lines && *lines ? &output_info : NULL, outfile, &info,
>  		    diff_result->result, context_lines);
>  		if (rc != DIFF_RC_OK)
>  			return got_error_set_errno(rc, "diff_output_unidiff");
>  		break;
>  	case GOT_DIFF_OUTPUT_PLAIN:
> -		rc = diff_output_plain(lines ? &output_info : NULL,
> +		rc = diff_output_plain(lines && *lines ? &output_info : NULL,
>  		    outfile, &info, diff_result->result, 1);
>  		if (rc != DIFF_RC_OK)
>  			return got_error_set_errno(rc, "diff_output_edscript");