From: Stefan Sperling <stsp@stsp.name>
Subject: Re: got_object_id-ify painted commits
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Wed, 10 Jul 2024 08:13:41 +0200

On Tue, Jul 09, 2024 at 11:23:14PM +0200, Omar Polo wrote:
> in the same vein as in previous diffs this changes the
> got_imsg_painted_commit to use a got_object_id instead of a uint8_t
> buffer.
> 
> Just to recap, my plan is to change very soon(tm) the struct
> got_object_id to this:
> 
> 	#define GOT_OBJECT_ID_MAXLEN SHA256_DIGEST_LENGTH
> 	struct got_object_id {
> 		u_int_8 hash[GOT_OBJECT_ID_MAXLEN];
> 		enum got_hash_algorithm algo;
> 	};

Perhaps we should define enum got_hash_algorithm in a way that
makes zero an invalid value? This way we could catch cases where
we initialized with memset but did not set the algo explicitly.
Currently, enum value zero means SHA1.

> where the `hash' field could only be partially used in the case of a
> sha1 digest.  Moving whole structs is easier than having to set up the
> algo field and ensure the hash is of the correct size everywhere.  On
> the other hand, a compromised libexec helper could send us garbage.
> nothing but lib/hash.c should touch it however.  I felt I had to point
> out this.

ok for the diff

> diff /home/op/w/got
> commit - 1b1a386df9068b7cb3ceb8a67d88ccd24f5b2deb
> path + /home/op/w/got
> blob - 96ea6e4692bb24d2f128702524fea15db694ddb5
> file + lib/got_lib_privsep.h
> --- lib/got_lib_privsep.h
> +++ lib/got_lib_privsep.h
> @@ -354,7 +354,7 @@ struct got_imsg_commit_painting_request {
>  
>  /* Structure for GOT_IMSG_PAINTED_COMMITS. */
>  struct got_imsg_painted_commit {
> -	uint8_t id[SHA1_DIGEST_LENGTH];
> +	struct got_object_id id;
>  	intptr_t color;
>  } __attribute__((__packed__));
>  
> blob - 0ca99e2d0f60973cd9f9304f89eb72f756cc520b
> file + lib/privsep.c
> --- lib/privsep.c
> +++ lib/privsep.c
> @@ -3427,7 +3427,7 @@ send_painted_commits(struct got_object_id_queue *ids, 
>  		color = (intptr_t)qid->data;
>  
>  		/* Keep in sync with struct got_imsg_painted_commit! */
> -		if (imsg_add(wbuf, qid->id.sha1, SHA1_DIGEST_LENGTH) == -1)
> +		if (imsg_add(wbuf, &qid->id, sizeof(qid->id)) == -1)
>  			return got_error_from_errno("imsg_add PAINTED_COMMITS");
>  		if (imsg_add(wbuf, &color, sizeof(color)) == -1)
>  			return got_error_from_errno("imsg_add PAINTED_COMMITS");
> @@ -3521,7 +3521,7 @@ got_privsep_recv_painted_commits(struct got_object_id_
>  
>  			if (icommits.present_in_pack) {
>  				struct got_object_id id;
> -				memcpy(id.sha1, icommit.id, SHA1_DIGEST_LENGTH);
> +				memcpy(&id, &icommit.id, sizeof(id));
>  				err = cb(cb_arg, &id, icommit.color);
>  				if (err)
>  					break;
> @@ -3530,8 +3530,8 @@ got_privsep_recv_painted_commits(struct got_object_id_
>  				err = got_object_qid_alloc_partial(&qid);
>  				if (err)
>  					break;
> -				memcpy(qid->id.sha1, icommit.id,
> -				    SHA1_DIGEST_LENGTH);
> +				memcpy(&qid->id, &icommit.id,
> +				    sizeof(qid->id));
>  				qid->data = (void *)icommit.color;
>  				STAILQ_INSERT_TAIL(new_ids, qid, entry);
>  			}
> 
>