[libvirt] [PATCH] maint: avoid potential promotion issues with [ug]id_t
Guannan Ren
gren at redhat.com
Tue Jan 8 03:12:25 UTC 2013
On 01/08/2013 07:06 AM, Eric Blake wrote:
> POSIX does not guarantee whether uid_t and gid_t are signed or
> unsigned, nor does it guarantee whether they are smaller, same
> size, or larger than int (or even the same size as one another).
> Therefore, it is possible to have platforms where '(uid_t)-1==-1'
> is false or where 'uid = gid = -1' sets uid to the wrong value,
> thanks to integer promotion rules. The only portable way to use
> the placeholder value of these two types is to always use a cast.
> Thankfully, the issue is mostly theoretical - sanlock only
> compiles on Linux for now, and on Linux, these types do not
> suffer from strange promotion problems.
>
> * src/locking/lock_driver_sanlock.c
> (virLockManagerSanlockSetupLockspace, virLockManagerSanlockInit)
> (virLockManagerSanlockCreateLease): Cast -1 to proper type before
> comparing with uid_t or gid_t.
> ---
> src/locking/lock_driver_sanlock.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
> index c955003..d06fa66 100644
> --- a/src/locking/lock_driver_sanlock.c
> +++ b/src/locking/lock_driver_sanlock.c
> @@ -1,7 +1,7 @@
> /*
> * lock_driver_sanlock.c: A lock driver for Sanlock
> *
> - * Copyright (C) 2010-2012 Red Hat, Inc.
> + * Copyright (C) 2010-2013 Red Hat, Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -236,7 +236,7 @@ static int virLockManagerSanlockSetupLockspace(void)
> goto error;
> }
>
> - if (driver->group != -1)
> + if (driver->group != (gid_t) -1)
> perms |= 0060;
>
> if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, perms)) < 0) {
> @@ -249,7 +249,7 @@ static int virLockManagerSanlockSetupLockspace(void)
> VIR_DEBUG("Someone else just created lockspace %s", path);
> } else {
> /* chown() the path to make sure sanlock can access it */
> - if ((driver->user != -1 || driver->group != -1) &&
> + if ((driver->user != (uid_t) -1 || driver->group != (gid_t) -1) &&
> (fchown(fd, driver->user, driver->group) < 0)) {
> virReportSystemError(errno,
> _("cannot chown '%s' to (%u, %u)"),
> @@ -303,8 +303,8 @@ static int virLockManagerSanlockSetupLockspace(void)
> }
> } else if (S_ISREG(st.st_mode)) {
> /* okay, the lease file exists. Check the permissions */
> - if (((driver->user != -1 && driver->user != st.st_uid) ||
> - (driver->group != -1 && driver->group != st.st_gid)) &&
> + if (((driver->user != (uid_t) -1 && driver->user != st.st_uid) ||
> + (driver->group != (gid_t) -1 && driver->group != st.st_gid)) &&
> (chown(path, driver->user, driver->group) < 0)) {
> virReportSystemError(errno,
> _("cannot chown '%s' to (%u, %u)"),
> @@ -314,7 +314,7 @@ static int virLockManagerSanlockSetupLockspace(void)
> goto error;
> }
>
> - if ((driver->group != -1 && (st.st_mode & 0060) != 0060) &&
> + if ((driver->group != (gid_t) -1 && (st.st_mode & 0060) != 0060) &&
> chmod(path, 0660) < 0) {
> virReportSystemError(errno,
> _("cannot chmod '%s' to 0660"),
> @@ -397,7 +397,8 @@ static int virLockManagerSanlockInit(unsigned int version,
> driver->requireLeaseForDisks = true;
> driver->hostID = 0;
> driver->autoDiskLease = false;
> - driver->user = driver->group = -1;
> + driver->user = (uid_t) -1;
> + driver->group = (gid_t) -1;
> if (!(driver->autoDiskLeasePath = strdup(LOCALSTATEDIR "/lib/libvirt/sanlock"))) {
> VIR_FREE(driver);
> virReportOOMError();
> @@ -680,7 +681,7 @@ static int virLockManagerSanlockCreateLease(struct sanlk_resource *res)
> VIR_DEBUG("Someone else just created lockspace %s", res->disks[0].path);
> } else {
> /* chown() the path to make sure sanlock can access it */
> - if ((driver->user != -1 || driver->group != -1) &&
> + if ((driver->user != (uid_t) -1 || driver->group != (gid_t) -1) &&
> (fchown(fd, driver->user, driver->group) < 0)) {
> virReportSystemError(errno,
> _("cannot chown '%s' to (%u, %u)"),
According to GUN C library
"uid_t This is an integer data type used to represent user IDs.
In the GNU library, this is an alias for unsigned int."
So driver->user = -1 will lead to wrong value.
http://pubs.opengroup.org/onlinepubs/000095399/functions/chown.html
use (uid_t) -1 too.
ACK.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130108/a7b08227/attachment-0001.htm>
More information about the libvir-list
mailing list