From: Tracey Emery <tracey@traceyemery.net>
Subject: Re: gotd: remove "sendfd" pledge promise from repo_write
To: gameoftrees@openbsd.org
Date: Fri, 28 Oct 2022 07:41:40 -0600

On Fri, Oct 28, 2022 at 12:08:19PM +0200, Stefan Sperling wrote:
> Have the parent process send one end of the pipe directly to gotsh(1),
> such that repo_write can run without "sendfd".
> Combining "sendfd" and "recvfd" in the same process is frowned upon.
> 
> ok?

ok. one extra tab below

>  
> diff 417258479785447bb8b2d6106b1220cd833e4591 e228960be4ba7555768fabb33fb29a89beee42cf
> commit - 417258479785447bb8b2d6106b1220cd833e4591
> commit + e228960be4ba7555768fabb33fb29a89beee42cf
> blob - 4e94e74827b4a318903802c2324b5b653b29e5bd
> blob + 0d1b70abc84b7ffd073e85521770b144812ebcfe
> --- gotd/gotd.c
> +++ gotd/gotd.c
> @@ -720,6 +720,7 @@ recv_packfile(struct gotd_client *client)
>  	memset(&ipipe, 0, sizeof(ipipe));
>  	ipipe.client_id = client->id;
>  
> +	/* Send pack pipe end 0 to repo_write. */
>  	if (gotd_imsg_compose_event(&client->repo_write->iev,
>  	    GOTD_IMSG_PACKFILE_PIPE, PROC_GOTD, pipe[0],
>  	        &ipipe, sizeof(ipipe)) == -1) {
> @@ -729,9 +730,9 @@ recv_packfile(struct gotd_client *client)
>  	}
>  	pipe[0] = -1;
>  
> -	if (gotd_imsg_compose_event(&client->repo_write->iev,
> -	    GOTD_IMSG_PACKFILE_PIPE, PROC_GOTD, pipe[1],
> -	        &ipipe, sizeof(ipipe)) == -1)
> +	/* Send pack pipe end 1 to gotsh(1) (expects just an fd, no data). */
> +	if (gotd_imsg_compose_event(&client->iev,
> +	    GOTD_IMSG_PACKFILE_PIPE, PROC_GOTD, pipe[1], NULL, 0) == -1)
>  		err = got_error_from_errno("imsg compose PACKFILE_PIPE");
>  	pipe[1] = -1;
>  
> @@ -2041,7 +2042,7 @@ main(int argc, char **argv)
>  		exit(0);
>  	case PROC_REPO_WRITE:
>  #ifndef PROFILE
> -		if (pledge("stdio rpath sendfd recvfd", NULL) == -1)
> +		if (pledge("stdio rpath recvfd", NULL) == -1)
>  			err(1, "pledge");
>  #endif
>  		repo_write_main(title, pack_fds, temp_fds);
> blob - fa09c1ca57d4bdb4c0dec036592b4d2967d28018
> blob + 72ae9a0a2f15bd70bd8d6d112727e39f7c311690
> --- gotd/repo_write.c
> +++ gotd/repo_write.c
> @@ -82,7 +82,7 @@ struct repo_write_client {
>  	STAILQ_ENTRY(repo_write_client)	 entry;
>  	uint32_t			 id;
>  	int				 fd;
> -	int				 pack_pipe[2];
> +	int				 pack_pipe;
>  	struct got_pack			 pack;
>  	uint8_t				 pack_sha1[SHA1_DIGEST_LENGTH];
>  	int				 packidx_fd;
> @@ -107,8 +107,7 @@ add_client(struct repo_write_client *client, uint32_t 
>  
>  	client->id = client_id;
>  	client->fd = fd;
> -	client->pack_pipe[0] = -1;
> -	client->pack_pipe[1] = -1;
> +	client->pack_pipe = -1;
>  	client->packidx_fd = -1;
>  	STAILQ_INIT(&client->ref_updates);
>  	client->nref_updates = 0;
> @@ -929,8 +928,7 @@ recv_packfile(struct repo_write_client **client, struc
>  	if (*client == NULL || STAILQ_EMPTY(&(*client)->ref_updates))
>  		return got_error(GOT_ERR_CLIENT_ID);
>  
> -	if ((*client)->pack_pipe[0] == -1 ||
> -	    (*client)->pack_pipe[1] == -1 ||
> +	if ((*client)->pack_pipe == -1 ||
>  	    (*client)->packidx_fd == -1)
>  		return got_error(GOT_ERR_PRIVSEP_NO_FD);
>  
> @@ -969,22 +967,13 @@ recv_packfile(struct repo_write_client **client, struc
>  		tempfiles[i] = f;
>  	}
>  
> -	/* Send pack file pipe to gotsh(1). */
> -	if (imsg_compose(&ibuf, GOTD_IMSG_RECV_PACKFILE, PROC_REPO_WRITE,
> -	    repo_write.pid, (*client)->pack_pipe[1], NULL, 0) == -1) {
> -		(*client)->pack_pipe[1] = -1;
> -		err = got_error_from_errno("imsg_compose ACK");
> -		if (err)	
here
> -			goto done;
> -	}
> -	(*client)->pack_pipe[1] = -1;
>  	err = gotd_imsg_flush(&ibuf);
>  	if (err)
>  		goto done;
>  
>  	log_debug("receiving pack data");
>  	unpack_err = recv_packdata(&pack_filesize, (*client)->pack_sha1,
> -	    (*client)->pack_pipe[0], pack->fd);
> +	    (*client)->pack_pipe, pack->fd);
>  	if (ireq.report_status) {
>  		err = report_pack_status(*client, unpack_err);
>  		if (err) {
> @@ -1023,9 +1012,9 @@ done:
>  	if (lseek((*client)->packidx_fd, 0L, SEEK_SET) == -1)
>  		err = got_error_from_errno("lseek");
>  done:
> -	if (close((*client)->pack_pipe[0]) == -1 && err == NULL)
> +	if (close((*client)->pack_pipe) == -1 && err == NULL)
>  		err = got_error_from_errno("close");
> -	(*client)->pack_pipe[0] = -1;
> +	(*client)->pack_pipe = -1;
>  	for (i = 0; i < nitems(repo_tempfiles); i++) {
>  		struct repo_tempfile *t = &repo_tempfiles[i];
>  		if (t->idx != -1)
> @@ -1197,7 +1186,7 @@ recv_disconnect(struct imsg *imsg)
>  	const struct got_error *err = NULL;
>  	struct gotd_imsg_disconnect idisconnect;
>  	size_t datalen;
> -	int client_fd = -1, pipe0 = -1, pipe1 = - 1, idxfd = -1;
> +	int client_fd = -1, pack_pipe = -1, idxfd = -1;
>  	struct repo_write_client *client = NULL;
>  	uint64_t slot;
>  
> @@ -1224,16 +1213,13 @@ recv_disconnect(struct imsg *imsg)
>  	}
>  	err = got_pack_close(&client->pack);
>  	client_fd = client->fd;
> -	pipe0 = client->pack_pipe[0];
> -	pipe1 = client->pack_pipe[1];
> +	pack_pipe = client->pack_pipe;
>  	idxfd = client->packidx_fd;
>  	free(client);
>  	if (client_fd != -1 && close(client_fd) == -1)
>  		err = got_error_from_errno("close");
> -	if (pipe0 != -1 && close(pipe0) == -1 && err == NULL)
> +	if (pack_pipe != -1 && close(pack_pipe) == -1 && err == NULL)
>  		err = got_error_from_errno("close");
> -	if (pipe1 != -1 && close(pipe1) == -1 && err == NULL)
> -		err = got_error_from_errno("close");
>  	if (idxfd != -1 && close(idxfd) == -1 && err == NULL)
>  		err = got_error_from_errno("close");
>  	return err;
> @@ -1259,14 +1245,10 @@ receive_pack_pipe(struct repo_write_client **client, s
>  	*client = find_client(ireq.client_id);
>  	if (*client == NULL)
>  		return got_error(GOT_ERR_CLIENT_ID);
> -	if ((*client)->pack_pipe[1] != -1)
> +	if ((*client)->pack_pipe != -1)
>  		return got_error(GOT_ERR_PRIVSEP_MSG);
>  
> -	if ((*client)->pack_pipe[0] == -1)
> -		(*client)->pack_pipe[0] = imsg->fd;
> -	else
> -		(*client)->pack_pipe[1] = imsg->fd;
> -
> +	(*client)->pack_pipe = imsg->fd;
>  	return NULL;
>  }
>  
> blob - 87e9f9ab41e1449492b4a1ca2485a8fba48bfbee
> blob + 07ef39056b1ae8681d10c81839b68eb97c9a1146
> --- lib/serve.c
> +++ lib/serve.c
> @@ -1381,7 +1381,7 @@ serve_write(int infd, int outfd, int gotd_sock, const 
>  		case GOTD_IMSG_ERROR:
>  			err = gotd_imsg_recv_error(NULL, &imsg);
>  			goto done;
> -		case GOTD_IMSG_RECV_PACKFILE:
> +		case GOTD_IMSG_PACKFILE_PIPE:
>  			err = recv_packfile(&imsg, infd);
>  			if (err) {
>  				if (err->code != GOT_ERR_EOF)
> 

-- 

Tracey Emery