From: Stefan Sperling <stsp@stsp.name>
Subject: Re: got_blame memory leak
To: Kyle Ackerman <kackerman0102@gmail.com>
Cc: gameoftrees@openbsd.org
Date: Mon, 25 Mar 2024 09:13:46 +0100

On Sun, Mar 24, 2024 at 06:48:05PM -0500, Kyle Ackerman wrote:
> Here is alternative diff. Rather than checking after every message
> received, we can just allocate the got_object_id once we receive a
> GOT_IMSG_COMMIT_TRAVERSAL_DONE. This is here in case checking to see if
> changed_commit_id is not null after each imsg is ugly.`
> 
> diff /home/kyle/src/got
> commit - 39910b637a9a53cc48b0c63766da691dec0af593
> path + /home/kyle/src/got
> blob - 857c09bbb3a5d9425176ed93727a912b2242aac6
> file + lib/privsep.c
> --- lib/privsep.c
> +++ lib/privsep.c
> @@ -2730,8 +2730,7 @@ got_privsep_recv_traversed_commits(struct got_commit_o
>  
>  				/* The last commit may contain a change. */
>  				if (i == icommits->ncommits - 1) {
> -					*changed_commit_id =
> -					    got_object_id_dup(&qid->id);
> +					*changed_commit_id = &qid->id;
>  					if (*changed_commit_id == NULL) {

The pointer can no longer be NULL here.

>  						err = got_error_from_errno(
>  						    "got_object_id_dup");
> @@ -2749,6 +2748,7 @@ got_privsep_recv_traversed_commits(struct got_commit_o
>  			    datalen, ibuf);
>  			break;
>  		case GOT_IMSG_COMMIT_TRAVERSAL_DONE:
> +			*changed_commit_id = got_object_id_dup(*changed_commit_id);

And the above allocation failure check would now need to be here.

Howveer, this whole mechanism is a bit silly. The caller receives the
full list of traversed commits, and *changed_commit points at a newly
allocated copy of the last commit on this list.

Couldn't the caller use STAILQ_LAST to find the same commit, so we
can get rid of the unneccesary copy and **changed_commit_id in this
function entirely?

>  			done = 1;
>  			break;
>  		default:
> 
>