[libvirt] [PATCH] maint: avoid potential promotion issues with [ug]id_t
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)"), -- 1.8.0.2
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.
On 01/07/2013 08:12 PM, Guannan Ren wrote:
* src/locking/lock_driver_sanlock.c (virLockManagerSanlockSetupLockspace, virLockManagerSanlockInit) (virLockManagerSanlockCreateLease): Cast -1 to proper type before comparing with uid_t or gid_t.
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.
No, actually 'driver->user = -1' is always the right value, no matter what type uid_t is, because of C promotion rules (C guarantees that -1 promotes to the all-bits-set version of any unsigned type of any width). Where you get into problems is 'driver->user = driver->group = -1', in the theoretical platform where driver->group is narrower than driver->user and both types are unsigned (then you could end up with 0xffff instead of the intended 0xffffffff for driver->user). Likewise, on a platform where gid_t is 16-bit unsigned, then (gid_t)-1 == -1 promotes to '0xffff == -1' which is false. Thankfully, I don't know of any such perverse platforms, but better safe than sorry.
http://pubs.opengroup.org/onlinepubs/000095399/functions/chown.html use (uid_t) -1 too.
ACK.
Pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Guannan Ren