From: Stefan Sperling <stsp@stsp.name>
Subject: Re: batch up tree entries in imsg
To: Omar Polo <op@openbsd.org>
Cc: gameoftrees@openbsd.org
Date: Wed, 18 May 2022 17:46:45 +0200

On Wed, May 18, 2022 at 05:14:30PM +0200, Omar Polo wrote:
> > -		nimsg++;
> > -		if (totlen + len >= MAX_IMSGSIZE - (IMSG_HEADER_SIZE * nimsg)) {
> > -			err = flush_imsg(ibuf);
> > +		if (j > 0 &&
> > +		    entries_len + len > MAX_IMSGSIZE - IMSG_HEADER_SIZE) {
> > +			err = send_tree_entries(ibuf, entries,
> > +			    i, j - 1, entries_len);
> >  			if (err)
> >  				return err;
> > -			nimsg = 0;
> > +			i = j;
> > +			entries_len = sizeof(struct got_imsg_tree_entries);
> >  		}
> >  
> > -		wbuf = imsg_create(ibuf, GOT_IMSG_TREE_ENTRY, 0, 0, len);
> > -		if (wbuf == NULL)
> > -			return got_error_from_errno("imsg_create TREE_ENTRY");
> > +		entries_len += len;
> > +	}
> >  
> > -		/* Keep in sync with struct got_imsg_tree_object definition! */
> > -		if (imsg_add(wbuf, pte->id, SHA1_DIGEST_LENGTH) == -1) {
> > -			err = got_error_from_errno("imsg_add TREE_ENTRY");
> > -			ibuf_free(wbuf);
> > +	if (j > 0) {
> 
> maybe i'm not reading this correctly, but here isn't better to check for
> `j != i-1'?  if somehow we exit the previous loop after setting `i = j'
> then we enter here just to send 0 elements.  (assuming that's possible)

j will advance until *nentries. i tells us where the last split-point
was, where we decided to send out a batch of entries because the buffer
was full.

Perhaps re-read this loop after applying the diff, it might be clearer.
> I'm not sure I'm getting the meaning of this check:
> 
> > +				if (sizeof(ite) + ite.namelen < sizeof(ite)) {
> 
> ite.nameles is size_t so how can this evaluate to true?

This checks for unsigned integer overflow into the range [0, sizeof(ite)-1].
Remember that libexec code can send us garbage; the value of ite.namelen
comes from libexec code. Perhaps other checks already cover this, but the
more the better I guess.
 
> > +					err = got_error(GOT_ERR_PRIVSEP_LEN);
> > +					break;
> > +				}