[libvirt] PATCH: Fix misc memory alloc/free/usage bugs
Daniel P. Berrange
berrange at redhat.com
Tue Mar 31 13:29:02 UTC 2009
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 :|
More information about the libvir-list
mailing list