1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/3] Fix remote dispatch code trying to allocate 0-sized return buffers
  4. View patch

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

Erik Skultety posted 3 patches 6 years, 2 months ago
There is a newer version of this series
[libvirt] [PATCH 3/3] libvirt-domain: virConnectListDomains: Return 0 on zero-size buffer
Posted by Erik Skultety 6 years, 2 months ago 
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.

Signed-off-by: Erik Skultety <eskultet@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;
+
     if (conn->driver->connectListDomains) {
         int ret = conn->driver->connectListDomains(conn, ids, maxids);
         if (ret < 0)
-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] libvirt-domain: virConnectListDomains: Return 0 on zero-size buffer
Posted by Daniel P. Berrangé 6 years, 2 months ago 
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@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'.

IMHO, we should only mandate a non-NULL 'ids' parameter when
maxids > 0 a few lines earlier

All the other legacy APIs share the same validation flaw so will
also need fixing.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] libvirt-domain: virConnectListDomains: Return 0 on zero-size buffer
Posted by Erik Skultety 6 years, 2 months ago 
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@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.

>
> IMHO, we should only mandate a non-NULL 'ids' parameter when
> maxids > 0 a few lines earlier
>
> All the other legacy APIs share the same validation flaw so will
> also need fixing.

Right, I was too hasty, since gendispatch took care of the server side
automatically, I forgot to adjust the public APIs manually, will do :).

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] libvirt-domain: virConnectListDomains: Return 0 on zero-size buffer
Posted by Daniel P. Berrangé 6 years, 2 months ago 
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@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.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] libvirt-domain: virConnectListDomains: Return 0 on zero-size buffer
Posted by Erik Skultety 6 years, 2 months ago 
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@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.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] libvirt-domain: virConnectListDomains: Return 0 on zero-size buffer
Posted by Daniel P. Berrangé 6 years, 2 months ago 
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@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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.