From: Mark Jamsek <mark@jamsek.com>
Subject: Re: Diff memory leak
To: Stefan Sperling <stsp@stsp.name>
Cc: Kyle Ackerman <kackerman0102@gmail.com>, gameoftrees@openbsd.org
Date: Fri, 31 May 2024 20:14:58 +1000

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?


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");


-- 
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68