[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] PATCH: Fix misc memory alloc/free/usage bugs



On Tue, Mar 31, 2009 at 11:07:07AM +0100, Daniel P. Berrange wrote:
> On Tue, Mar 31, 2009 at 12:04:00PM +0200, Daniel Veillard wrote:
> > On Mon, Mar 30, 2009 at 11:41:20PM +0900, Ryota Ozaki wrote:
> > > Hi Daniel,
> > > 
> > > On Mon, Mar 30, 2009 at 10:39 PM, Daniel P. Berrange
> > > <berrange redhat com> wrote:
> > > > A few more small bugs
> > > >
> > > >  * src/node_device.c: Don't strdup() a NULL device parent
> > > >  * src/qemu_conf.c: Don't try to run access() on a NULL binary path
> > > >  * src/security_selinux.c Make sure result from readlink() is NULL
> > > >   terminated
> > > >  * src/storage_conf.c: DOn't free 'mode' while it is still used
> > 
> >   Patch looks fine, with the exception
> > 
> > [...]
> > > > +++ b/src/security_selinux.c    Mon Mar 30 14:37:45 2009 +0100
> > > > @@ -303,11 +303,13 @@ SELinuxRestoreSecurityImageLabel(virConn
> > > >         return -1;
> > > >
> > > >     if (S_ISLNK(buf.st_mode)) {
> > > > +        int n;
> > > >         if (VIR_ALLOC_N(newpath, buf.st_size + 1) < 0)
> > > >             return -1;
> > > >
> > > > -        if (readlink(path, newpath, buf.st_size) < 0)
> > > > +        if ((n =readlink(path, newpath, buf.st_size)) < 0)
> > > >             goto err;
> > > > +        buf.st_size[n] = '\0';
> > >            newpath[n] = '\0';
> > > 
> > > correct?
> > 
> >   Yup, I doubt it would compile otherwise :-)
> 
> Yep, I've just realized I made this change on a machine where the
> selinux driver was disabled, so will update this chunk

Here's a correct version:

Daniel


diff -r df287a2cbf9d src/node_device.c
--- a/src/node_device.c	Tue Mar 31 11:40:07 2009 +0100
+++ b/src/node_device.c	Tue Mar 31 11:40:39 2009 +0100
@@ -176,9 +176,14 @@ static char *nodeDeviceGetParent(virNode
         goto cleanup;
     }
 
-    ret = strdup(obj->def->parent);
-    if (!ret)
-        virReportOOMError(dev->conn);
+    if (obj->def->parent) {
+        ret = strdup(obj->def->parent);
+        if (!ret)
+            virReportOOMError(dev->conn);
+    } else {
+        virNodeDeviceReportError(dev->conn, VIR_ERR_INTERNAL_ERROR,
+                                 "%s", _("no parent for this device"));
+    }
 
 cleanup:
     if (obj)
diff -r df287a2cbf9d src/qemu_conf.c
--- a/src/qemu_conf.c	Tue Mar 31 11:40:07 2009 +0100
+++ b/src/qemu_conf.c	Tue Mar 31 11:40:39 2009 +0100
@@ -269,7 +269,7 @@ qemudCapsInitGuest(virCapsPtr caps,
      * which can be used with magic cpu choice
      */
     hasbase = (access(info->binary, X_OK) == 0);
-    hasaltbase = (access(info->altbinary, X_OK) == 0);
+    hasaltbase = (info->altbinary && access(info->altbinary, X_OK) == 0);
 
     /* Can use acceleration for KVM/KQEMU if
      *  - host & guest arches match
diff -r df287a2cbf9d src/security_selinux.c
--- a/src/security_selinux.c	Tue Mar 31 11:40:07 2009 +0100
+++ b/src/security_selinux.c	Tue Mar 31 11:40:39 2009 +0100
@@ -303,11 +303,13 @@ SELinuxRestoreSecurityImageLabel(virConn
         return -1;
 
     if (S_ISLNK(buf.st_mode)) {
+        int n;
         if (VIR_ALLOC_N(newpath, buf.st_size + 1) < 0)
             return -1;
 
-        if (readlink(path, newpath, buf.st_size) < 0)
+        if ((n =readlink(path, newpath, buf.st_size)) < 0)
             goto err;
+        newpath[n] = '\0';
         path = newpath;
         if (stat(path, &buf) != 0)
             goto err;
diff -r df287a2cbf9d src/storage_conf.c
--- a/src/storage_conf.c	Tue Mar 31 11:40:07 2009 +0100
+++ b/src/storage_conf.c	Tue Mar 31 11:40:39 2009 +0100
@@ -401,12 +401,13 @@ virStorageDefParsePerms(virConnectPtr co
     } else {
         char *end = NULL;
         perms->mode = strtol(mode, &end, 8);
-        VIR_FREE(mode);
         if (*end || perms->mode < 0 || perms->mode > 0777) {
+            VIR_FREE(mode);
             virStorageReportError(conn, VIR_ERR_XML_ERROR,
                                   "%s", _("malformed octal mode"));
             goto error;
         }
+        VIR_FREE(mode);
     }
 
     if (virXPathNode(conn, "./owner", ctxt) == NULL) {



-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]