[libvirt] [PATCH 3/3] libvirt-domain: virConnectListDomains: Return 0 on zero-size buffer

[Daniel P. Berrangé]

On Mon, Nov 18, 2019 at 03:20:00PM +0100, Erik Skultety wrote:
> On Mon, Nov 18, 2019 at 01:50:03PM +0000, Daniel P. Berrangé wrote:
> > On Mon, Nov 18, 2019 at 02:26:31PM +0100, Erik Skultety wrote:
> > > On Mon, Nov 18, 2019 at 01:04:01PM +0000, Daniel P. Berrangé wrote:
> > > > On Mon, Nov 18, 2019 at 01:18:19PM +0100, Erik Skultety wrote:
> > > > > It doesn't make sense to pass a target buffer into an API, declaring its
> > > > > size as 0 and expect some meaningful result. Since this used to work
> > > > > pre-Glib era, we shouldn't end with an error, but we can return 0
> > > > > for the number of domains immediately, instead of calling into the
> > > > > daemon, which is exactly what the daemon would have returned anyway.
> > > >
> > > > Passing in size as 0 is going to be normal practice, given the calling
> > > > convention of this API design.
> > > >
> > > > >
> > > > > Signed-off-by: Erik Skultety <eskultet at redhat.com>
> > > > > ---
> > > > >  src/libvirt-domain.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> > > > > index 02622cb2ca..0def40fdf7 100644
> > > > > --- a/src/libvirt-domain.c
> > > > > +++ b/src/libvirt-domain.c
> > > > > @@ -62,6 +62,9 @@ virConnectListDomains(virConnectPtr conn, int *ids, int maxids)
> > > > >      virCheckNonNullArgGoto(ids, error);
> > > > >      virCheckNonNegativeArgGoto(maxids, error);
> > > > >
> > > > > +    if (maxids == 0)
> > > > > +        return 0;
> > > >
> > > > This is too late really, as we alrady checked 'ids'.
> > >
> > > Why is ^this a problem? The pointer has to be allocated prior to calling into
> > > the API, so a failure on a NULL pointer on client-side is fine. On server side,
> > > the issue is remediated much earlier in the RPC dispatch code.
> >
> > The current regression is caused server-side, but I'm saying the
> > problem in general is pre-existing even client side. Chances are
> > that people have already had to work around this when calling it
> > client side.
> >
> > If we fix this though, we should fix it so that we aovid the
> > problem both client & server side.
> >
> >
> > IOW, rather than taking the approach to avoiding calling the
> > API when num==0, make it valid to do the call, so that our
> > public API's behaviour isn't dependant on whether the client's
> > malloc() returns NULL or not for a zero sized allocation.
> 
> I could move the same check a few lines earlier above the
> virCheckNonNullArgGoto check, but that doesn't go with your idea of allowing
> the client to make the RPC call. Personally, I find it rather pointless to make
> an RPC call if we can deterministically tell that the result is going to be 0,
> effectively a NOP.

Well there's access control checks performed server side. We're not going
to leak any data by not doing the call, but we will hide the access
control failure message if we bypass it. IMHO, we shouldn't be making
assumptions client side about what the server side will do.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|