[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 12:04:00PM +0200, Daniel Veillard wrote:
> > > +++ 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 :-)
> 
> 
>   I'm still doubtful about this piece of code though.
> Suppose you have path an absolute path to /tmp/bar, and
> bar is a relative symlink to foo, you will get buf.st_size
> which is 3 i.e. the length of "foo" which is insufficient
> to hold the expected path /tmp/foo.
>   Also /tmp/bar may point to /tmp/foo, which itself points to
> /tmp/very_long_filename and again readlink will fail to provide
> the full path.
>   Seems to me that if you're to use readlink there is no other
> way than to allocate a PATH_MAX (+1) buffer and use that for
> the link resolution.

Actually, the buffer is the exact right size, because that
'buf.st_size'  was filled by a lstat() call, and the POSIX
semantics for lstat() are that 'st_size' will contain the
number of bytes in the destination filename. At least that's
what this post claims....

http://lists.debian.org/debian-hurd/2001/07/msg00301.html

Here's an updated patch which makes a helper function for
all this


Daniel

diff -r b6f96d7d7b11 src/libvirt_private.syms
--- a/src/libvirt_private.syms	Tue Mar 31 13:48:24 2009 +0100
+++ b/src/libvirt_private.syms	Tue Mar 31 14:26:15 2009 +0100
@@ -306,6 +306,7 @@ virStrToLong_ll;
 virStrToLong_ull;
 virStrToLong_ui;
 virFileLinkPointsTo;
+virFileResolveLink;
 saferead;
 safewrite;
 safezero;
diff -r b6f96d7d7b11 src/security_selinux.c
--- a/src/security_selinux.c	Tue Mar 31 13:48:24 2009 +0100
+++ b/src/security_selinux.c	Tue Mar 31 14:26:15 2009 +0100
@@ -293,28 +293,24 @@ SELinuxRestoreSecurityImageLabel(virConn
     struct stat buf;
     security_context_t fcon = NULL;
     int rc = -1;
+    int err;
     char *newpath = NULL;
     const char *path = disk->src;
 
     if (disk->readonly || disk->shared)
         return 0;
 
-    if (lstat(path, &buf) != 0)
-        return -1;
-
-    if (S_ISLNK(buf.st_mode)) {
-        if (VIR_ALLOC_N(newpath, buf.st_size + 1) < 0)
-            return -1;
-
-        if (readlink(path, newpath, buf.st_size) < 0)
-            goto err;
-        path = newpath;
-        if (stat(path, &buf) != 0)
-            goto err;
+    if ((err = virFileResolveLink(path, &newpath)) < 0) {
+        virReportSystemError(conn, err,
+                             _("cannot resolve symlink %s"), path);
+        goto err;
     }
 
-    if (matchpathcon(path, buf.st_mode, &fcon) == 0)  {
-        rc = SELinuxSetFilecon(conn, path, fcon);
+    if (stat(newpath, &buf) != 0)
+        goto err;
+
+    if (matchpathcon(newpath, buf.st_mode, &fcon) == 0)  {
+        rc = SELinuxSetFilecon(conn, newpath, fcon);
     }
 err:
     VIR_FREE(fcon);
diff -r b6f96d7d7b11 src/storage_backend_disk.c
--- a/src/storage_backend_disk.c	Tue Mar 31 13:48:24 2009 +0100
+++ b/src/storage_backend_disk.c	Tue Mar 31 14:26:15 2009 +0100
@@ -362,20 +362,16 @@ virStorageBackendDiskDeleteVol(virConnec
                                unsigned int flags ATTRIBUTE_UNUSED)
 {
     char *part_num = NULL;
-    int n;
-    char devpath[PATH_MAX];
+    int err;
+    char *devpath = NULL;
     char *devname, *srcname;
+    int rc = -1;
 
-    if ((n = readlink(vol->target.path, devpath, sizeof(devpath))) < 0 &&
-        errno != EINVAL) {
-        virReportSystemError(conn, errno,
+    if ((err = virFileResolveLink(vol->target.path, &devpath)) < 0) {
+        virReportSystemError(conn, err,
                              _("Couldn't read volume target path '%s'"),
                              vol->target.path);
-        return -1;
-    } else if (n <= 0) {
-        strncpy(devpath, vol->target.path, PATH_MAX);
-    } else {
-        devpath[n] = '\0';
+        goto cleanup;
     }
 
     devname = basename(devpath);
@@ -386,7 +382,7 @@ virStorageBackendDiskDeleteVol(virConnec
         virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
                               _("Volume path '%s' did not start with parent "
                                 "pool source device name."), devname);
-        return -1;
+        goto cleanup;
     }
 
     part_num = devname + strlen(srcname);
@@ -395,7 +391,7 @@ virStorageBackendDiskDeleteVol(virConnec
         virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
                               _("cannot parse partition number from target "
                                 "'%s'"), devname);
-        return -1;
+        goto cleanup;
     }
 
     /* eg parted /dev/sda rm 2 */
@@ -409,9 +405,12 @@ virStorageBackendDiskDeleteVol(virConnec
     };
 
     if (virRun(conn, prog, NULL) < 0)
-        return -1;
+        goto cleanup;
 
-    return 0;
+    rc = 0;
+cleanup:
+    VIR_FREE(devpath);
+    return rc;
 }
 
 
diff -r b6f96d7d7b11 src/util.c
--- a/src/util.c	Tue Mar 31 13:48:24 2009 +0100
+++ b/src/util.c	Tue Mar 31 14:26:15 2009 +0100
@@ -934,6 +934,53 @@ int virFileLinkPointsTo(const char *chec
             && SAME_INODE (src_sb, dest_sb));
 }
 
+
+
+/*
+ * Attempt to resolve a symbolic link, returning the
+ * real path
+ *
+ * Return 0 if path was not a symbolic, or the link was
+ * resolved. Return -1 upon error
+ */
+int virFileResolveLink(const char *linkpath,
+                       char **resultpath)
+{
+    struct stat st;
+    char *buf;
+    int n;
+
+    *resultpath = NULL;
+
+    if (lstat(linkpath, &st) < 0)
+        return errno;
+
+    if (!S_ISLNK(st.st_mode)) {
+        if (!(*resultpath = strdup(linkpath)))
+            return -ENOMEM;
+        return 0;
+    }
+
+    /* Posix says that 'st_size' field from
+     * result of an lstat() call is filled with
+     * number of bytes in the destination
+     * filename.
+     */
+    if (VIR_ALLOC_N(buf, st.st_size + 1) < 0)
+        return -ENOMEM;
+
+    if ((n = readlink(linkpath, buf, st.st_size)) < 0) {
+        VIR_FREE(buf);
+        return -errno;
+    }
+
+    buf[n] = '\0';
+
+    *resultpath = buf;
+    return 0;
+}
+
+
 int virFileExists(const char *path)
 {
     struct stat st;
diff -r b6f96d7d7b11 src/util.h
--- a/src/util.h	Tue Mar 31 13:48:24 2009 +0100
+++ b/src/util.h	Tue Mar 31 14:26:15 2009 +0100
@@ -87,6 +87,9 @@ int virFileStripSuffix(char *str,
 int virFileLinkPointsTo(const char *checkLink,
                         const char *checkDest);
 
+int virFileResolveLink(const char *linkpath,
+                       char **resultpath);
+
 int virFileExists(const char *path);
 
 int virFileMakePath(const char *path);

-- 
|: 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]