From: Stefan Sperling <stsp@stsp.name>
Subject: Re: fix invalid imsg_free() in got_privsep_recv_printed_commits()
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Mon, 26 Feb 2024 16:54:27 +0100

On Sat, Feb 24, 2024 at 06:46:59PM +0100, Omar Polo wrote:
> This is probably the cause of the crash stsp observed by ctrl-c'ing
> gotadmin pack -a (https://paste.apache.org/jpr92), while also fixing a
> possible memory leak too.
> 
> if got_privsep_recv_imsg() fails before reading the imsg, the imsg
> struct is still un-initialized and can't be passed to imsg_free().
> Luckily, there are only two offenders in the tree which may call
> imsg_free() in the error path of got_privsep_recv_imsg().
> 
> (technically this introduces a possible leak in libexec/got-read-patch
> but it's just once per process so it should be acceptable.)
> 
> I think that got_privsep_recv_imsg() should free the imsg on error after
> it has read something.
> 
> ok?

Yes, ok!

> p.s. as a follow-up now that I noticed i think we should swap the order
> of the checks in got_privsep_recv_imsg(), so to have the GOT_IMSG_ERROR
> case first, then the length.

This doesn't make sense to me since the GOT_IMSG_ERROR path uses
imsg->hdr.len. So it requires a range-check, doesn't it?

> diff /home/op/w/got
> commit - 7a86002db34d49472a7d75c1802ee99c2120ef3c
> path + /home/op/w/got
> blob - 79db43c49ad84782e167cf267f4958c7fa944561
> file + lib/privsep.c
> --- lib/privsep.c
> +++ lib/privsep.c
> @@ -141,12 +141,16 @@ got_privsep_recv_imsg(struct imsg *imsg, struct imsgbu
>  			return got_error_from_errno("imsg_get");
>  	}
>  
> -	if (imsg->hdr.len < IMSG_HEADER_SIZE + min_datalen)
> +	if (imsg->hdr.len < IMSG_HEADER_SIZE + min_datalen) {
> +		imsg_free(imsg);
>  		return got_error(GOT_ERR_PRIVSEP_LEN);
> +	}
>  
>  	if (imsg->hdr.type == GOT_IMSG_ERROR) {
>  		size_t datalen = imsg->hdr.len - IMSG_HEADER_SIZE;
> -		return recv_imsg_error(imsg, datalen);
> +		err = recv_imsg_error(imsg, datalen);
> +		imsg_free(imsg);
> +		return err;
>  	}
>  
>  	return NULL;
> @@ -3510,10 +3514,8 @@ got_privsep_recv_painted_commits(struct got_object_id_
>  
>  	for (;;) {
>  		err = got_privsep_recv_imsg(&imsg, ibuf, 0);
> -		if (err){
> -			imsg_free(&imsg);
> +		if (err)
>  			return err;
> -		}
>  
>  		datalen = imsg.hdr.len - IMSG_HEADER_SIZE;
>  		if (imsg.hdr.type == GOT_IMSG_COMMIT_PAINTING_DONE) {
> blob - 508821d598050b15d1b623785750f1a0c1040d10
> file + libexec/got-read-patch/got-read-patch.c
> --- libexec/got-read-patch/got-read-patch.c
> +++ libexec/got-read-patch/got-read-patch.c
> @@ -682,8 +682,8 @@ main(int argc, char **argv)
>  		goto done;
>  	}
>  	err = got_privsep_flush_imsg(&ibuf);
> -done:
>  	imsg_free(&imsg);
> +done:
>  	if (fd != -1 && close(fd) == -1 && err == NULL)
>  		err = got_error_from_errno("close");
>  	if (fp != NULL && fclose(fp) == EOF && err == NULL)
> 
>