[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