[libvirt] [PATCH] Ensure nodeinfo struct is initialized to zero
From: "Daniel P. Berrange" <berrange@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@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
On 01/23/13 16:45, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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@redhat.com> --- src/nodeinfo.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
ACK. Peter
On Wed, Jan 23, 2013 at 03:45:01PM +0000, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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@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#)
On Wed, Jan 23, 2013 at 03:45:01PM +0000, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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@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 :|
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
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 :|
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
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 :|
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
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 :|
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa -
Richard W.M. Jones