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

On Tue, Mar 31, 2009 at 12:04:00PM +0200, Daniel Veillard wrote: 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 :|