[libvirt] [PATCH v2 0/4] Fix remote dispatch code trying to allocate 0-sized return buffers

Erik Skultety posted 4 patches 4 years, 4 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/cover.1574326693.git.eskultet@redhat.com
src/internal.h                      | 13 +++++++++++++
src/libvirt-domain-snapshot.c       |  4 ++--
src/libvirt-domain.c                | 21 ++++++---------------
src/libvirt-host.c                  |  2 +-
src/libvirt-interface.c             |  4 ++--
src/libvirt-network.c               |  4 ++--
src/libvirt-nodedev.c               |  4 ++--
src/libvirt-nwfilter.c              |  2 +-
src/libvirt-secret.c                |  2 +-
src/libvirt-storage.c               |  6 +++---
src/remote/remote_daemon_dispatch.c |  6 ++++++
src/rpc/gendispatch.pl              | 12 ++++++++++--
12 files changed, 49 insertions(+), 31 deletions(-)
[libvirt] [PATCH v2 0/4] Fix remote dispatch code trying to allocate 0-sized return buffers
Posted by Erik Skultety 4 years, 4 months ago
This happens because of the switch to glib whose method g_malloc0 actually
correctly returns NULL on size 0 which doesn't make sense to do. The outcome of
that is that because virAllocN always returns 0, the dispatch code never fails
allocation and passes the NULL pointer to the server-side public API which
actually checks that and fails.

https://bugzilla.redhat.com/show_bug.cgi?id=1772842

Since v1:
    - reworked the original patch 3/3 (now 4/4) by introducing a
      virCheckNonNull-style macro which checks whether the caller-provided array
      isn't by any chance NULL while the declared size of this array is a
      positive int (Dan's suggestion)
        -> apart from the single scenario check (NULL && n > 0), the other
           combinations are allowed for the legacy APIs letting the server-side
           handle this
    - added patch 3/4 when I realized one of the legacy APIs which didn't have
      the dispatch code autogenerated needed adjustment too

    - I deliberately didn't touch the APIs using typed params, as half of them
      are setters and the other half is supposed to be called twice, so we
      already have various NULL checks in place

    - there were a few legacy APIs that already had some protection in the
      remote dispatch code, so I didn't touch the client side for those, IMO we
      can always treat those on case-by-case basis if a similar issue arises
      than risk breaking one third of libvirt now :P

Erik Skultety (4):
  rpc: gendispatch: Fix a couple of places adding trailing spaces
  rpc: gendispatch: Add a check for zero size client-side buffers
  remote: Add a check for zero sized client-side buffers
  libvirt-<module>: Check caller-provided buffers to be NULL with size >
    0

 src/internal.h                      | 13 +++++++++++++
 src/libvirt-domain-snapshot.c       |  4 ++--
 src/libvirt-domain.c                | 21 ++++++---------------
 src/libvirt-host.c                  |  2 +-
 src/libvirt-interface.c             |  4 ++--
 src/libvirt-network.c               |  4 ++--
 src/libvirt-nodedev.c               |  4 ++--
 src/libvirt-nwfilter.c              |  2 +-
 src/libvirt-secret.c                |  2 +-
 src/libvirt-storage.c               |  6 +++---
 src/remote/remote_daemon_dispatch.c |  6 ++++++
 src/rpc/gendispatch.pl              | 12 ++++++++++--
 12 files changed, 49 insertions(+), 31 deletions(-)

-- 
2.23.0

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