From: Omar Polo Subject: Re: fix overzelous sanity check in got_privsep_get_imsg_obj To: gameoftrees@openbsd.org Date: Sat, 18 Jun 2022 17:07:30 +0200 Omar Polo wrote: > Omar Polo wrote: > > It seems that i was a bit too much optimistic in c98b0f0. With got > > freshly compiled from ports i sometimes get a 'bad offset in pack file' > > failure. This happened in 'got status', 'got diff', ... and even in > > tog, in got.git and src.git. This happens very frequently thought, but > > not always. > > > > Curiously, this didn't happened in my local builds. > > > > Diff below fixes the issue for me. It moves the sanity check in only in > > the case where the object was effectively packed. > > > > This is just fixing the behaviour thought, i don't know why/how > > pack_offset was negative. i tried with > > > > if (obj->pack_offset < 0) > > abort(); > > > > in got_privsep_send_obj but couldn't get it to segfault. > > ...because the libexec is sending good data. * because the libexec is attempting to send good data. > got_privsep_send_obj > copies the data from the given struct got_object into a stack-allocated > struct got_imsg_object. Except that this struct isn't zeroed and so we > read garbage value when the object is not packed. > > so please discard my previous diff, the solution is to fix this at the > sending side: and eventually we may want to zero also the other structs? most of these only have one or two fields, but others like the delta have a number of them. every one of them has all the fields always initialized thought, only got_privsep_send_obj was optionally initializing some. diff 0fe55807cf233278482e51afcee4b60d5c974340 /home/op/w/got blob - 2d5319f731933605f168211ff0d2be2f135b57b2 file + lib/privsep.c --- lib/privsep.c +++ lib/privsep.c @@ -297,6 +297,7 @@ got_privsep_send_raw_obj(struct imsgbuf *ibuf, off_t s size_t len = sizeof(iobj); struct ibuf *wbuf; + memset(&iobj, 0, sizeof(iobj)); iobj.hdrlen = hdrlen; iobj.size = size; @@ -387,6 +388,7 @@ got_privsep_send_commit_req(struct imsgbuf *ibuf, int void *data; size_t len; + memset(&iobj, 0, sizeof(iobj)); if (pack_idx != -1) { /* commit is packed */ iobj.idx = pack_idx; memcpy(iobj.id, id->sha1, sizeof(iobj.id)); @@ -445,6 +447,7 @@ got_privsep_send_tag_req(struct imsgbuf *ibuf, int fd, void *data; size_t len; + memset(&iobj, 0, sizeof(iobj)); if (pack_idx != -1) { /* tag is packed */ iobj.idx = pack_idx; memcpy(iobj.id, id->sha1, sizeof(iobj.id)); @@ -471,6 +474,7 @@ got_privsep_send_blob_req(struct imsgbuf *ibuf, int in void *data; size_t len; + memset(&iobj, 0, sizeof(iobj)); if (pack_idx != -1) { /* blob is packed */ iobj.idx = pack_idx; memcpy(iobj.id, id->sha1, sizeof(iobj.id)); @@ -1423,6 +1427,8 @@ send_tree_entries_batch(struct imsgbuf *ibuf, struct got_imsg_tree_entries ientries; int i; + memset(&ientries, 0, sizeof(ientries)); + wbuf = imsg_create(ibuf, GOT_IMSG_TREE_ENTRIES, 0, 0, len); if (wbuf == NULL) return got_error_from_errno("imsg_create TREE_ENTRY"); @@ -1495,6 +1501,7 @@ got_privsep_send_tree(struct imsgbuf *ibuf, const struct got_error *err = NULL; struct got_imsg_tree_object itree; + memset(&itree, 0, sizeof(itree)); itree.nentries = nentries; if (imsg_compose(ibuf, GOT_IMSG_TREE, 0, 0, -1, &itree, sizeof(itree)) == -1) @@ -1685,6 +1692,7 @@ got_privsep_send_blob(struct imsgbuf *ibuf, size_t siz { struct got_imsg_blob iblob; + memset(&iblob, 0, sizeof(iblob)); iblob.size = size; iblob.hdrlen = hdrlen; @@ -1964,6 +1972,9 @@ got_privsep_init_pack_child(struct imsgbuf *ibuf, stru struct got_imsg_pack ipack; int fd; + memset(&ipackidx, 0, sizeof(ipackidx)); + memset(&ipack, 0, sizeof(ipack)); + ipackidx.len = packidx->len; ipackidx.packfile_size = pack->filesize; fd = dup(packidx->fd); @@ -2002,6 +2013,7 @@ got_privsep_send_packed_obj_req(struct imsgbuf *ibuf, { struct got_imsg_packed_object iobj; + memset(&iobj, 0, sizeof(iobj)); iobj.idx = idx; memcpy(iobj.id, id->sha1, sizeof(iobj.id)); @@ -2019,6 +2031,7 @@ got_privsep_send_packed_raw_obj_req(struct imsgbuf *ib { struct got_imsg_packed_object iobj; + memset(&iobj, 0, sizeof(iobj)); iobj.idx = idx; memcpy(iobj.id, id->sha1, sizeof(iobj.id)); @@ -2983,6 +2996,7 @@ got_privsep_send_raw_delta_req(struct imsgbuf *ibuf, i { struct got_imsg_raw_delta_request dreq; + memset(&dreq, 0, sizeof(dreq)); dreq.idx = idx; memcpy(dreq.id, id->sha1, SHA1_DIGEST_LENGTH); @@ -3007,6 +3021,7 @@ got_privsep_send_raw_delta(struct imsgbuf *ibuf, uint6 struct got_imsg_raw_delta idelta; int ret; + memset(&idelta, 0, sizeof(idelta)); idelta.base_size = base_size; idelta.result_size = result_size; idelta.delta_size = delta_size; @@ -3089,6 +3104,8 @@ send_idlist(struct imsgbuf *ibuf, struct got_object_id struct ibuf *wbuf; size_t i; + memset(&idlist, 0, sizeof(idlist)); + if (nids > GOT_IMSG_OBJ_ID_LIST_MAX_NIDS) return got_error(GOT_ERR_NO_SPACE); @@ -3224,6 +3241,8 @@ got_privsep_send_reused_deltas(struct imsgbuf *ibuf, struct got_imsg_reused_deltas ideltas; size_t i; + memset(&ideltas, 0, sizeof(ideltas)); + if (ndeltas > GOT_IMSG_REUSED_DELTAS_MAX_NDELTAS) return got_error(GOT_ERR_NO_SPACE);