9:45 a.m.
From: "Daniel P. Berrange" <berrange(a)redhat.com>
When linuxNodeInfoCPUPopulate() method triggered use of an
uninitialize value, since it did not initialize the 'sockets'
field in the virNodeInfoPtr struct:
==30020== Conditional jump or move depends on uninitialised value(s)
==30020== at 0x5125DBD: linuxNodeInfoCPUPopulate (nodeinfo.c:513)
==30020== by 0x51261A0: nodeGetInfo (nodeinfo.c:884)
==30020== by 0x149B9B10: qemuCapsInit (qemu_capabilities.c:846)
==30020== by 0x14A11B25: qemuCreateCapabilities (qemu_driver.c:424)
==30020== by 0x14A12426: qemuStartup (qemu_driver.c:874)
==30020== by 0x512A7AF: virStateInitialize (libvirt.c:822)
==30020== by 0x40DE04: daemonRunStateInit (libvirtd.c:877)
==30020== by 0x50ADCE5: virThreadHelper (virthreadpthread.c:161)
==30020== by 0x328CA07D14: start_thread (pthread_create.c:308)
==30020== by 0x328C6F246C: clone (clone.S:114)
(happened twice)
if (socks > nodeinfo->sockets) <--- here
nodeinfo->sockets = socks;
Rather than doing this for each field, just make the caller memset
the entire struct to zero.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/nodeinfo.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 477104f..5b91a12 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -387,11 +387,6 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
char *sysfs_nodedir = NULL;
char *sysfs_cpudir = NULL;
- nodeinfo->cpus = 0;
- nodeinfo->mhz = 0;
- nodeinfo->cores = 0;
- nodeinfo->nodes = 0;
-
/* Start with parsing CPU clock speed from /proc/cpuinfo */
while (fgets(line, sizeof(line), cpuinfo) != NULL) {
# if defined(__x86_64__) || \
@@ -868,6 +863,8 @@ int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr
nodeinfo)
{
virArch hostarch = virArchFromHost();
+ memset(nodeinfo, 0, sizeof(nodeinfo));
+
if (virStrcpyStatic(nodeinfo->model, virArchToString(hostarch)) == NULL)
return -1;
--
1.8.1
9:48 a.m.
On 01/23/13 16:45, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
When linuxNodeInfoCPUPopulate() method triggered use of an
uninitialize value, since it did not initialize the 'sockets'
field in the virNodeInfoPtr struct:
==30020== Conditional jump or move depends on uninitialised value(s)
==30020== at 0x5125DBD: linuxNodeInfoCPUPopulate (nodeinfo.c:513)
==30020== by 0x51261A0: nodeGetInfo (nodeinfo.c:884)
==30020== by 0x149B9B10: qemuCapsInit (qemu_capabilities.c:846)
==30020== by 0x14A11B25: qemuCreateCapabilities (qemu_driver.c:424)
==30020== by 0x14A12426: qemuStartup (qemu_driver.c:874)
==30020== by 0x512A7AF: virStateInitialize (libvirt.c:822)
==30020== by 0x40DE04: daemonRunStateInit (libvirtd.c:877)
==30020== by 0x50ADCE5: virThreadHelper (virthreadpthread.c:161)
==30020== by 0x328CA07D14: start_thread (pthread_create.c:308)
==30020== by 0x328C6F246C: clone (clone.S:114)
(happened twice)
if (socks > nodeinfo->sockets) <--- here
nodeinfo->sockets = socks;
Rather than doing this for each field, just make the caller memset
the entire struct to zero.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/nodeinfo.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
ACK.
Peter
10:12 a.m.
On Wed, Jan 23, 2013 at 03:45:01PM +0000, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
When linuxNodeInfoCPUPopulate() method triggered use of an
uninitialize value, since it did not initialize the 'sockets'
field in the virNodeInfoPtr struct:
==30020== Conditional jump or move depends on uninitialised value(s)
==30020== at 0x5125DBD: linuxNodeInfoCPUPopulate (nodeinfo.c:513)
==30020== by 0x51261A0: nodeGetInfo (nodeinfo.c:884)
==30020== by 0x149B9B10: qemuCapsInit (qemu_capabilities.c:846)
==30020== by 0x14A11B25: qemuCreateCapabilities (qemu_driver.c:424)
==30020== by 0x14A12426: qemuStartup (qemu_driver.c:874)
==30020== by 0x512A7AF: virStateInitialize (libvirt.c:822)
==30020== by 0x40DE04: daemonRunStateInit (libvirtd.c:877)
==30020== by 0x50ADCE5: virThreadHelper (virthreadpthread.c:161)
==30020== by 0x328CA07D14: start_thread (pthread_create.c:308)
==30020== by 0x328C6F246C: clone (clone.S:114)
(happened twice)
if (socks > nodeinfo->sockets) <--- here
nodeinfo->sockets = socks;
Rather than doing this for each field, just make the caller memset
the entire struct to zero.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/nodeinfo.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 477104f..5b91a12 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -387,11 +387,6 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
char *sysfs_nodedir = NULL;
char *sysfs_cpudir = NULL;
- nodeinfo->cpus = 0;
- nodeinfo->mhz = 0;
- nodeinfo->cores = 0;
- nodeinfo->nodes = 0;
-
/* Start with parsing CPU clock speed from /proc/cpuinfo */
while (fgets(line, sizeof(line), cpuinfo) != NULL) {
# if defined(__x86_64__) || \
@@ -868,6 +863,8 @@ int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr
nodeinfo)
{
virArch hostarch = virArchFromHost();
+ memset(nodeinfo, 0, sizeof(nodeinfo));
+
if (virStrcpyStatic(nodeinfo->model, virArchToString(hostarch)) == NULL)
return -1;
--
1.8.1
Not tested, but it seems like a better, future-proof approach so ACK.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
11:06 a.m.
On Wed, Jan 23, 2013 at 03:45:01PM +0000, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
When linuxNodeInfoCPUPopulate() method triggered use of an
uninitialize value, since it did not initialize the 'sockets'
field in the virNodeInfoPtr struct:
==30020== Conditional jump or move depends on uninitialised value(s)
==30020== at 0x5125DBD: linuxNodeInfoCPUPopulate (nodeinfo.c:513)
==30020== by 0x51261A0: nodeGetInfo (nodeinfo.c:884)
==30020== by 0x149B9B10: qemuCapsInit (qemu_capabilities.c:846)
==30020== by 0x14A11B25: qemuCreateCapabilities (qemu_driver.c:424)
==30020== by 0x14A12426: qemuStartup (qemu_driver.c:874)
==30020== by 0x512A7AF: virStateInitialize (libvirt.c:822)
==30020== by 0x40DE04: daemonRunStateInit (libvirtd.c:877)
==30020== by 0x50ADCE5: virThreadHelper (virthreadpthread.c:161)
==30020== by 0x328CA07D14: start_thread (pthread_create.c:308)
==30020== by 0x328C6F246C: clone (clone.S:114)
(happened twice)
if (socks > nodeinfo->sockets) <--- here
nodeinfo->sockets = socks;
Rather than doing this for each field, just make the caller memset
the entire struct to zero.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/nodeinfo.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 477104f..5b91a12 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -387,11 +387,6 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
char *sysfs_nodedir = NULL;
char *sysfs_cpudir = NULL;
- nodeinfo->cpus = 0;
- nodeinfo->mhz = 0;
- nodeinfo->cores = 0;
- nodeinfo->nodes = 0;
-
/* Start with parsing CPU clock speed from /proc/cpuinfo */
while (fgets(line, sizeof(line), cpuinfo) != NULL) {
# if defined(__x86_64__) || \
@@ -868,6 +863,8 @@ int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr
nodeinfo)
{
virArch hostarch = virArchFromHost();
+ memset(nodeinfo, 0, sizeof(nodeinfo));
+
Of course i meant sizeof(*nodeinfo)
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
12:02 p.m.
On Wed, Jan 23, 2013 at 05:06:35PM +0000, Daniel P. Berrange wrote:
> + memset(nodeinfo, 0, sizeof(nodeinfo));
> +
Of course i meant sizeof(*nodeinfo)
Yikes. Doesn't libvirt have some type of VIR_* macro to either catch
that error systematically or to allocate pre-cleared memory?
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
12:07 p.m.
On Wed, Jan 23, 2013 at 06:02:59PM +0000, Richard W.M. Jones wrote:
On Wed, Jan 23, 2013 at 05:06:35PM +0000, Daniel P. Berrange wrote:
> > + memset(nodeinfo, 0, sizeof(nodeinfo));
> > +
>
> Of course i meant sizeof(*nodeinfo)
Yikes. Doesn't libvirt have some type of VIR_* macro to either catch
that error systematically or to allocate pre-cleared memory?
No, but i was actually just about to suggest a VIR_ZERO() macro for
precisely this problem
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
12:12 p.m.
On 01/23/2013 11:02 AM, Richard W.M. Jones wrote:
On Wed, Jan 23, 2013 at 05:06:35PM +0000, Daniel P. Berrange wrote:
>> + memset(nodeinfo, 0, sizeof(nodeinfo));
>> +
>
> Of course i meant sizeof(*nodeinfo)
Yikes. Doesn't libvirt have some type of VIR_* macro to either catch
that error systematically or to allocate pre-cleared memory?
VIR_MALLOC guarantees pre-cleared memory, but in this case, nodeinfo was
passed in by the caller rather than something that we control directly.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:13 p.m.
On Wed, Jan 23, 2013 at 11:12:03AM -0700, Eric Blake wrote:
On 01/23/2013 11:02 AM, Richard W.M. Jones wrote:
> On Wed, Jan 23, 2013 at 05:06:35PM +0000, Daniel P. Berrange wrote:
>>> + memset(nodeinfo, 0, sizeof(nodeinfo));
>>> +
>>
>> Of course i meant sizeof(*nodeinfo)
>
> Yikes. Doesn't libvirt have some type of VIR_* macro to either catch
> that error systematically or to allocate pre-cleared memory?
VIR_MALLOC guarantees pre-cleared memory, but in this case, nodeinfo was
passed in by the caller rather than something that we control directly.
Of course there are plenty of places where we stack allocate
stuff which we then memset(), so I believe a VIR_ZERO is worth
while
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
12:18 p.m.
On 01/23/2013 11:13 AM, Daniel P. Berrange wrote:
On Wed, Jan 23, 2013 at 11:12:03AM -0700, Eric Blake wrote:
> On 01/23/2013 11:02 AM, Richard W.M. Jones wrote:
>> On Wed, Jan 23, 2013 at 05:06:35PM +0000, Daniel P. Berrange wrote:
>>>> + memset(nodeinfo, 0, sizeof(nodeinfo));
>>>> +
>>>
>>> Of course i meant sizeof(*nodeinfo)
>>
>> Yikes. Doesn't libvirt have some type of VIR_* macro to either catch
>> that error systematically or to allocate pre-cleared memory?
>
> VIR_MALLOC guarantees pre-cleared memory, but in this case, nodeinfo was
> passed in by the caller rather than something that we control directly.
Of course there are plenty of places where we stack allocate
stuff which we then memset(), so I believe a VIR_ZERO is worth
while
Agreed. Also, in the case of virNodeGetInfo(), I wonder if we should
just hoist the VIR_ZERO into the libvirt.c entry point, rather than
making every driver have to worry about clearing out potentially
uninitialized incoming data on all error paths.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:19 p.m.
On Wed, Jan 23, 2013 at 11:18:04AM -0700, Eric Blake wrote:
On 01/23/2013 11:13 AM, Daniel P. Berrange wrote:
> On Wed, Jan 23, 2013 at 11:12:03AM -0700, Eric Blake wrote:
>> On 01/23/2013 11:02 AM, Richard W.M. Jones wrote:
>>> On Wed, Jan 23, 2013 at 05:06:35PM +0000, Daniel P. Berrange wrote:
>>>>> + memset(nodeinfo, 0, sizeof(nodeinfo));
>>>>> +
>>>>
>>>> Of course i meant sizeof(*nodeinfo)
>>>
>>> Yikes. Doesn't libvirt have some type of VIR_* macro to either catch
>>> that error systematically or to allocate pre-cleared memory?
>>
>> VIR_MALLOC guarantees pre-cleared memory, but in this case, nodeinfo was
>> passed in by the caller rather than something that we control directly.
>
> Of course there are plenty of places where we stack allocate
> stuff which we then memset(), so I believe a VIR_ZERO is worth
> while
Agreed. Also, in the case of virNodeGetInfo(), I wonder if we should
just hoist the VIR_ZERO into the libvirt.c entry point, rather than
making every driver have to worry about clearing out potentially
uninitialized incoming data on all error paths.
Makes sense - that's what we do for virDomainGetInfo. Might be other
APIs neeeding similar change
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
4321
days inactive
4321
days old
9 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa -
Richard W.M. Jones