[libvirt] [PATCH 0/7] Various Coverity based concerns

John Ferlan posted 7 patches 5 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180928152810.27537-1-jferlan@redhat.com
src/libxl/libxl_migration.c       |  4 ++--
src/lxc/lxc_driver.c              |  2 +-
src/nwfilter/nwfilter_dhcpsnoop.c |  9 ++++-----
src/util/virutil.c                | 11 +++++------
tests/commandtest.c               |  4 ++--
tests/libxlxml2domconfigtest.c    | 11 +++++------
tests/virhostcputest.c            | 12 ++++++++++--
7 files changed, 29 insertions(+), 24 deletions(-)
[libvirt] [PATCH 0/7] Various Coverity based concerns
Posted by John Ferlan 5 years, 6 months ago
I'm sure it'll be felt one or two could just be false positives,
but I have 35-40 of true false positives and it seems at least
these go above just noise on the channel.

Perhaps the most difficult one to immediately see was the libxl
refcnt patch. That involves a little bit of theory and has been
in my noise pile for a while until I noted that the @args is
being added in a loop to a callback function that just Unref's
it when done. So if there was more than 1 IP Address, then all
sorts of fun things could happen. Without any change, the Alloc
is matched by the Unref, but with the change we add a Ref to
match each Unref in the I/O loop and we just ensure the Unref
is done for the path that put @args into the I/O callback.

I also think the nwfilter patch was "interesting" insomuch as
it has my "favorite" 'if (int-value) {' condition. IOW, if
not zero, then do something. What became "interesting" is that
the virNWFilterIPAddrMapDelIPAddr could return -1 if the
virHashLookup on @req->binding->portdevname returned NULL,
so when "shrinking" the code to only call the instantiation
for/when there was an IP Address found resolves a couple of
issues in the code.

John Ferlan (7):
  lxc: Only check @nparams in lxcDomainBlockStatsFlags
  libxl: Fix possible object refcnt issue
  tests: Inline a sysconf call for linuxCPUStatsToBuf
  util: Data overrun may lead to divide by zero
  tests: Alter logic in testCompareXMLToDomConfig
  tests: Use STRNEQ_NULLABLE
  nwfilter: Alter virNWFilterSnoopReqLeaseDel logic

 src/libxl/libxl_migration.c       |  4 ++--
 src/lxc/lxc_driver.c              |  2 +-
 src/nwfilter/nwfilter_dhcpsnoop.c |  9 ++++-----
 src/util/virutil.c                | 11 +++++------
 tests/commandtest.c               |  4 ++--
 tests/libxlxml2domconfigtest.c    | 11 +++++------
 tests/virhostcputest.c            | 12 ++++++++++--
 7 files changed, 29 insertions(+), 24 deletions(-)

-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/7] Various Coverity based concerns
Posted by Michal Privoznik 5 years, 6 months ago
On 09/28/2018 05:28 PM, John Ferlan wrote:
> I'm sure it'll be felt one or two could just be false positives,
> but I have 35-40 of true false positives and it seems at least
> these go above just noise on the channel.
> 
> Perhaps the most difficult one to immediately see was the libxl
> refcnt patch. That involves a little bit of theory and has been
> in my noise pile for a while until I noted that the @args is
> being added in a loop to a callback function that just Unref's
> it when done. So if there was more than 1 IP Address, then all
> sorts of fun things could happen. Without any change, the Alloc
> is matched by the Unref, but with the change we add a Ref to
> match each Unref in the I/O loop and we just ensure the Unref
> is done for the path that put @args into the I/O callback.
> 
> I also think the nwfilter patch was "interesting" insomuch as
> it has my "favorite" 'if (int-value) {' condition. IOW, if
> not zero, then do something. What became "interesting" is that
> the virNWFilterIPAddrMapDelIPAddr could return -1 if the
> virHashLookup on @req->binding->portdevname returned NULL,
> so when "shrinking" the code to only call the instantiation
> for/when there was an IP Address found resolves a couple of
> issues in the code.
> 
> John Ferlan (7):
>   lxc: Only check @nparams in lxcDomainBlockStatsFlags
>   libxl: Fix possible object refcnt issue
>   tests: Inline a sysconf call for linuxCPUStatsToBuf
>   util: Data overrun may lead to divide by zero
>   tests: Alter logic in testCompareXMLToDomConfig
>   tests: Use STRNEQ_NULLABLE
>   nwfilter: Alter virNWFilterSnoopReqLeaseDel logic
> 
>  src/libxl/libxl_migration.c       |  4 ++--
>  src/lxc/lxc_driver.c              |  2 +-
>  src/nwfilter/nwfilter_dhcpsnoop.c |  9 ++++-----
>  src/util/virutil.c                | 11 +++++------
>  tests/commandtest.c               |  4 ++--
>  tests/libxlxml2domconfigtest.c    | 11 +++++------
>  tests/virhostcputest.c            | 12 ++++++++++--
>  7 files changed, 29 insertions(+), 24 deletions(-)
> 

ACK

Michal

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