On Mon, May 16, 2022 at 03:51:35PM +0200, Stefan Sperling wrote:
> On Sun, May 15, 2022 at 03:00:04PM +0200, Stefan Sperling wrote:
> > On Sun, May 15, 2022 at 02:47:42PM +0200, Stefan Sperling wrote:
> > > A couple of fixes for got_privsep_recv_tree().
> > > 
> > > The most important fix is at the bottom: We failed to check whether
> > > the number of tree entries sent by the libexec helper exceeds the
> > > number of tree entries advertised in the first got_imsg_tree_object
> > > message. This makes it trivial for the libexec process to make the
> > > main process write beyond the end of its entries array (heap overflow).
> > > 
> > > Besides this, rework the logic to avoid an ugly 'goto', check whether
> > > the number of entries is >= 0 (this is an int, which gets converted
> > > to size_t when passed to calloc, where it would become a very large
> > > number if negative and likely trigger ENOMEM), and free *tree if we
> > > fail to allocate (*tree)->entries. Freeing *tree via got_object_tree_close()
> > > below depends on (*tree)->nentries being set up properly, which hasn't
> > > happened at this point.
> > > 
> > > I should probably make separate commits for each fix but don't want
> > > to bother the mailing list with 4 tiny patches.
> > > 
> > > ok?
> > 
> > My previous patch broke tests in cherrypick.sh.
> > 
> > I forgot that a tree with zero entries is a valid case.
> > There may be an issue for -portable here because we rely on OpenBSD calloc()
> > behaviour where a unique non-NULL pointer is returned for zero-size
> > allocations, instead of a NULL pointer. But this can be fixed separately,
> > and it is not a new problem.
> > 
> > Fixed diff (all tests are passing this time):
> 
> I found another bug in this patch, where got tree -R failed on a large
> repository because of inversed logic in an if-statement. Fixed here.

I have committed these fixes.
If anyone still has feedback, it is always welcome.