Stefan Sperling <stsp@stsp.name> wrote:
> 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.

yes, it's right, I misunderstood the code before.

> Perhaps re-read this loop after applying the diff, it might be clearer.

eheh...  I missed the latest commits, that's why I couldn't apply this
diff

> > 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.

I see, haven't thought about the unsigned integer overflow.  I'm still
not sure that's needed, it's an expression involving only `size_t's and
reached that point we already know that namelen is less than NAME_MAX+1
(sizeof te->name).  But as you said it's probably better to keep these
checks even if slightly redundant.

> > > +					err = got_error(GOT_ERR_PRIVSEP_LEN);
> > > +					break;
> > > +				}

thanks for the explanations!