On 2023/02/26 12:27:36 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> > 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?

Yes, it's more "popular" in privsep.c to do like so and use strndup(3)
on the receiving side.

While here however I noticed that path can't be empty in practice.
(I'm a bit embarassed for not noticing it before.)

got_privsep_send_commit_traversal_request would crash on strlen(3) if
path is NULL, and got-read-pack itself assumes `path' is always filled
(being "/" eventually.)

So, while here, rejects a path of zero too.  It's not strictly needed
(`strndup(0)' should work in practice) but it's more explicit about
what we currently require.

regress with and without GOT_TEST_PACK is still fine :)


Thanks,

diff /home/op/w/got
commit - 7c87767bd660c4520db62cd54975d68d9d82a518
path + /home/op/w/got
blob - 7599c74782932b66b186f9bb91980bd32cd5c663
file + lib/got_lib_privsep.h
--- 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
file + lib/privsep.c
--- lib/privsep.c
+++ lib/privsep.c
@@ -2650,19 +2650,26 @@ got_privsep_send_commit_traversal_request(struct imsgb
     struct got_object_id *id, int idx, const char *path)
 {
 	struct ibuf *wbuf;
-	size_t path_len = strlen(path) + 1;
+	size_t path_len = strlen(path);
 
 	wbuf = imsg_create(ibuf, GOT_IMSG_COMMIT_TRAVERSAL_REQUEST, 0, 0,
 	    sizeof(struct got_imsg_commit_traversal_request) + path_len);
 	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
file + libexec/got-read-pack/got-read-pack.c
--- libexec/got-read-pack/got-read-pack.c
+++ libexec/got-read-pack/got-read-pack.c
@@ -602,34 +602,34 @@ 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')
-			return got_error(GOT_ERR_PRIVSEP_LEN);
-	}
+	if (ctreq.path_len == 0)
+		return got_error(GOT_ERR_PRIVSEP_LEN);
 
+	path = strndup(imsg->data + sizeof(ctreq), ctreq.path_len);
+	if (path == NULL)
+		return got_error_from_errno("strndup");
+
 	nallocated = min_alloc;
 	commit_ids = reallocarray(NULL, nallocated, sizeof(*commit_ids));
 	if (commit_ids == NULL)
@@ -767,6 +767,7 @@ done:
 	}
 	err = send_commit_traversal_done(ibuf);
 done:
+	free(path);
 	free(commit_ids);
 	if (commit)
 		got_object_commit_close(commit);