From: Stefan Sperling <stsp@stsp.name>
Subject: Re: fix GOT_IMSG_COMMIT_TRAVERSAL_REQUEST
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Sun, 26 Feb 2023 12:27:36 +0100

On Thu, Feb 23, 2023 at 08:14:58PM +0100, Omar Polo wrote:
> The serialization/deserialization were sending data differently to
> what got_lib_privsep.h described.

Ah, you mean the path_len wasn't actually used? Is that it, or am
I missing something else?

> This also fixes an object id sent
> as SHA1_DIGEST_LENGTH instead of as whole struct got_object_id.
> 
> I decided to keep the path_len field (before was unused) and the
> struct got_imsg_commit_traversal_request: it's easier to document the
> data for GOT_IMSG_COMMIT_TRAVERSAL_REQUEST this way, instead of just a
> got_imsg_packed_object plus an optional path.
> 
> There's the weird len > 1 case to handle though, since we're sending a
> NUL-terminated string.

Can't we change this to avoid sending a trailing '\0' since we already
know the length? Would that make the code look less surprising?
 
> It also adds the usual comment "Keep in sync with struct ..." that
> it's been helpful in other places to me in the past.
>  
> diff aaff679f20511b583d1cb1711d90549400df530c c829fe838002b73c360e33c6ddd13b047d54286b
> commit - aaff679f20511b583d1cb1711d90549400df530c
> commit + c829fe838002b73c360e33c6ddd13b047d54286b
> blob - 7599c74782932b66b186f9bb91980bd32cd5c663
> blob + d517f36f2ccbda4c48348016a20e6d6e87fbd1a4
> --- lib/got_lib_privsep.h
> +++ lib/got_lib_privsep.h
> @@ -576,8 +576,7 @@ struct got_imsg_commit_traversal_request {
>  
>  /* Structure for GOT_IMSG_COMMIT_TRAVERSAL_REQUEST  */
>  struct got_imsg_commit_traversal_request {
> -	uint8_t id[SHA1_DIGEST_LENGTH];
> -	int idx;
> +	struct got_imsg_packed_object iobj;
>  	size_t path_len;
>  	/* Followed by path_len bytes of path data */
>  } __attribute__((__packed__));
> blob - b8ea370fd2e88931bfc3f441dded6c848d55b3bd
> blob + 428c52ef5d63343f90856dcfbdd533a85e01e94d
> --- lib/privsep.c
> +++ lib/privsep.c
> @@ -2657,12 +2657,19 @@ got_privsep_send_commit_traversal_request(struct imsgb
>  	if (wbuf == NULL)
>  		return got_error_from_errno(
>  		    "imsg_create COMMIT_TRAVERSAL_REQUEST");
> -	if (imsg_add(wbuf, id->sha1, SHA1_DIGEST_LENGTH) == -1)
> +	/*
> +	 * Keep in sync with struct got_imsg_commit_traversal_request
> +	 * and struct got_imsg_packed_object.
> +	 */
> +	if (imsg_add(wbuf, id, sizeof(*id)) == -1)
>  		return got_error_from_errno("imsg_add "
>  		    "COMMIT_TRAVERSAL_REQUEST");
>  	if (imsg_add(wbuf, &idx, sizeof(idx)) == -1)
>  		return got_error_from_errno("imsg_add "
>  		    "COMMIT_TRAVERSAL_REQUEST");
> +	if (imsg_add(wbuf, &path_len, sizeof(path_len)) == -1)
> +		return got_error_from_errno("imsg_add "
> +		    "COMMIT_TRAVERSAL_REQUEST");
>  	if (imsg_add(wbuf, path, path_len) == -1)
>  		return got_error_from_errno("imsg_add "
>  		    "COMMIT_TRAVERSAL_REQUEST");
> blob - ed4f9039ad8c4841bba30c73ac1ec2a413e881e7
> blob + 8a1d148c63d9c40486bab560c3e7d77131742c67
> --- libexec/got-read-pack/got-read-pack.c
> +++ libexec/got-read-pack/got-read-pack.c
> @@ -602,31 +602,30 @@ commit_traversal_request(struct imsg *imsg, struct ims
>      struct got_object_cache *objcache)
>  {
>  	const struct got_error *err = NULL;
> -	struct got_imsg_packed_object iobj;
> +	struct got_imsg_commit_traversal_request ctreq;
>  	struct got_object_qid *pid;
>  	struct got_commit_object *commit = NULL, *pcommit = NULL;
>  	struct got_parsed_tree_entry *entries = NULL, *pentries = NULL;
>  	size_t nentries = 0, nentries_alloc = 0;
>  	size_t pnentries = 0, pnentries_alloc = 0;
>  	struct got_object_id id;
> -	size_t datalen, path_len;
> +	size_t datalen;
>  	char *path = NULL;
>  	const int min_alloc = 64;
>  	int changed = 0, ncommits = 0, nallocated = 0;
>  	struct got_object_id *commit_ids = NULL;
>  
>  	datalen = imsg->hdr.len - IMSG_HEADER_SIZE;
> -	if (datalen < sizeof(iobj))
> +	if (datalen < sizeof(ctreq))
>  		return got_error(GOT_ERR_PRIVSEP_LEN);
> -	memcpy(&iobj, imsg->data, sizeof(iobj));
> -	memcpy(&id, &iobj.id, sizeof(id));
> +	memcpy(&ctreq, imsg->data, sizeof(ctreq));
> +	memcpy(&id, &ctreq.iobj.id, sizeof(id));
>  
> -	path_len = datalen - sizeof(iobj) - 1;
> -	if (path_len < 0)
> +	if (datalen != sizeof(ctreq) + ctreq.path_len)
>  		return got_error(GOT_ERR_PRIVSEP_LEN);
> -	if (path_len > 0) {
> -		path = imsg->data + sizeof(iobj);
> -		if (path[path_len] != '\0')
> +	if (ctreq.path_len > 1) { /* one is for the NUL byte */
> +		path = imsg->data + sizeof(ctreq);
> +		if (path[ctreq.path_len - 1] != '\0')
>  			return got_error(GOT_ERR_PRIVSEP_LEN);
>  	}
>  
> 
> 
>