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

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