Re: [libvirt] [PATCH] virpcitest: fix coverity issues
On 02/06/2014 05:36 PM, Pavel Hrdina wrote:
On 6.2.2014 16:48, Eric Blake wrote:
On 02/06/2014 08:18 AM, Pavel Hrdina wrote:
diff --git a/tests/virpcitest.c b/tests/virpcitest.c
index 994b300..8ff3b1d 100644
--- a/tests/virpcitest.c
+++ b/tests/virpcitest.c
@@ -248,6 +248,7 @@ testVirPCIDeviceDetachSingle(const void *opaque)
if (!dev)
goto cleanup;
+/* coverity[freed_arg] */
if (virPCIDeviceSetStubDriver(dev, pci-stub) 0 ||
virPCIDeviceDetach(dev, NULL, NULL) 0)
goto cleanup;
Is this something where an sa_assert() could do the trick in a more
tool-agnostic manner?
I'm looking at the code again and probably the issue found by coverity
could happen if the STRDUP fails inside of the
virPCIDeviceSetStubDriver.
If STRDUP fails, then SetStubDriver should return -1 and we wouldn't be
calling DeviceDetach.
It would be nice if we could 'fix' virPCIDeviceSetStubDriver in a way coverity
would understand (fixing both complaints, but still checking if we're not
calling virKModLoad with a NULL argument). Does this patch make it any happier?
Jan
diff --git a/src/util/virpci.c b/src/util/virpci.c
index c3d211f..88002c9 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1657,7 +1657,7 @@ int
virPCIDeviceSetStubDriver(virPCIDevicePtr dev, const char *driver)
{
VIR_FREE(dev-stubDriver);
-return driver ? VIR_STRDUP(dev-stubDriver, driver) : 0;
+return VIR_STRDUP(dev-stubDriver, driver);
}
const char *
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virpcitest: fix coverity issues
The coverity server complains about the dev-stubDriver that it was freed by the virPCIDeviceSetStubDriver function, but it somehow don't get the fact that into the variable is immediately assigned new string. Signed-off-by: Pavel Hrdina phrd...@redhat.com --- tests/virpcitest.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 994b300..8ff3b1d 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -248,6 +248,7 @@ testVirPCIDeviceDetachSingle(const void *opaque) if (!dev) goto cleanup; +/* coverity[freed_arg] */ if (virPCIDeviceSetStubDriver(dev, pci-stub) 0 || virPCIDeviceDetach(dev, NULL, NULL) 0) goto cleanup; @@ -269,6 +270,7 @@ testVirPCIDeviceDetachFail(const void *opaque) if (!dev) goto cleanup; +/* coverity[freed_arg] */ if (virPCIDeviceSetStubDriver(dev, vfio-pci) 0) goto cleanup; -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virpcitest: fix coverity issues
On 02/06/2014 08:18 AM, Pavel Hrdina wrote: The coverity server complains about the dev-stubDriver that it was freed by the virPCIDeviceSetStubDriver function, but it somehow don't get the fact that into the variable is immediately assigned new string. Signed-off-by: Pavel Hrdina phrd...@redhat.com --- tests/virpcitest.c | 2 ++ 1 file changed, 2 insertions(+) What's the exact complaint? diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 994b300..8ff3b1d 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -248,6 +248,7 @@ testVirPCIDeviceDetachSingle(const void *opaque) if (!dev) goto cleanup; +/* coverity[freed_arg] */ if (virPCIDeviceSetStubDriver(dev, pci-stub) 0 || virPCIDeviceDetach(dev, NULL, NULL) 0) goto cleanup; Is this something where an sa_assert() could do the trick in a more tool-agnostic manner? -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virpcitest: fix coverity issues
On 6.2.2014 16:48, Eric Blake wrote: On 02/06/2014 08:18 AM, Pavel Hrdina wrote: The coverity server complains about the dev-stubDriver that it was freed by the virPCIDeviceSetStubDriver function, but it somehow don't get the fact that into the variable is immediately assigned new string. Signed-off-by: Pavel Hrdina phrd...@redhat.com --- tests/virpcitest.c | 2 ++ 1 file changed, 2 insertions(+) What's the exact complaint? The first one that coverity don't get that after free the new string is assigned into the dev-stubDriver: Event freed_arg: virPCIDeviceSetStubDriver frees dev-stubDriver. This is the root cause for another complaint that the dev-stubDriver is dereferenced after free: Event deref_arg: Calling virPCIDeviceDetach dereferences freed pointer dev-stubDriver. diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 994b300..8ff3b1d 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -248,6 +248,7 @@ testVirPCIDeviceDetachSingle(const void *opaque) if (!dev) goto cleanup; +/* coverity[freed_arg] */ if (virPCIDeviceSetStubDriver(dev, pci-stub) 0 || virPCIDeviceDetach(dev, NULL, NULL) 0) goto cleanup; Is this something where an sa_assert() could do the trick in a more tool-agnostic manner? I'm looking at the code again and probably the issue found by coverity could happen if the STRDUP fails inside of the virPCIDeviceSetStubDriver. This is a corner case where it could leads to creating a wrong modprobe command because the dev-stubDriver also as driver and then as module wouldn't be included in the command line. virPCIDeviceDetach()-virPCIProbeStubDriver()-virKModLoad()- doModprobe() For the virKmodLoad() function the coverity found this issue: Event deref_parm_in_call: Function virKModLoad dereferences driver. (The dereference is assumed on the basis of the 'nonnull' parameter attribute.) I'm not sure what to do with this. At first I've thought that it's only the coverity mistake. Thanks for the review, Pavel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
