9:18 a.m.
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 <phrdina(a)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
9:48 a.m.
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 <phrdina(a)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
10:36 a.m.
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 <phrdina(a)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
9:21 a.m.
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 *
3944
days inactive
3945
days old
3 comments
3 participants
participants (3)
-
Eric Blake -
Ján Tomko -
Pavel Hrdina