[PATCHv2] storage: fix uid_t|gid_t handling on 32 bit Linux

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



uid_t and gid_t are opaque types, ranging from s32 to u32 to u64.

Unfortunately libvirt uses the value -1 to mean "current process", which
on Linux32 gets converted to UINT_MAX := +(2^32-1) = 4294967295.

Different libvirt versions used different formatting in the past, which
break one or the other parsing function:
virXPathLong(): (signed long)-1 != (double)UINT_MAX
virXPathULong(): (unsigned long)-1 != (double)-1

Roll our own version of virXPathULong(), which also accepts -1, which is
silently converted to UINT_MAX.

For output: do the reverse and print -1 instead of UINT_MAX.

Allow -1 for owner and group in schema for storage volumes.

Change UINT_MAX in one test case to -1 to follow the changes from above.

Signed-off-by: Philipp Hahn <hahn@xxxxxxxxxxxxx>
---
[v2]
 - change schema for storage volume to accept -1 for owner and group
 - Use UINT_MAX instead of ALLONE
 - Windows64 uses s64, mention s16 and u16 as well
 - Fix Sheepdog test case to use -1 instead of UINT_MAX
---
 docs/schemas/storagevol.rng                 |   10 +++-
 src/conf/storage_conf.c                     |   76 ++++++++++++++++++++++-----
 tests/storagevolxml2xmlout/vol-sheepdog.xml |    4 +-
 3 files changed, 72 insertions(+), 18 deletions(-)

diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng
index d4a29c7..ba95c55 100644
--- a/docs/schemas/storagevol.rng
+++ b/docs/schemas/storagevol.rng
@@ -49,10 +49,16 @@
           <ref name='octalMode'/>
         </element>
         <element name='owner'>
-          <ref name='unsignedInt'/>
+          <choice>
+            <ref name='unsignedInt'/>
+            <value>-1</value>
+          </choice>
         </element>
         <element name='group'>
-          <ref name='unsignedInt'/>
+          <choice>
+            <ref name='unsignedInt'/>
+            <value>-1</value>
+          </choice>
         </element>
         <optional>
           <element name='label'>
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 9134a22..617b19f 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -659,13 +659,14 @@ cleanup:
 
     return ret;
 }
+
 static int
 virStorageDefParsePerms(xmlXPathContextPtr ctxt,
                         virStoragePermsPtr perms,
                         const char *permxpath,
                         int defaultmode) {
     char *mode;
-    long v;
+    double d;
     int ret = -1;
     xmlNodePtr relnode;
     xmlNodePtr node;
@@ -699,26 +700,58 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
         VIR_FREE(mode);
     }
 
+    /* uid_t and gid_t are *opaque* types: on Linux they're u32, on Windows64
+     * they're s64, on Solaris they were s32 in the past, and u16 and s16 have
+     * been used as well.
+     *
+     * Unfortunately libvirt uses the value -1 to mean "current process", which
+     * on Linux32 gets converted to UINT_MAX := +(2^32-1).
+     *
+     * Different libvirt versions used different formatting in the past, which
+     * break one or the other parsing function:
+     * virXPathLong(): (signed long)-1 != (double)UINT_MAX
+     * virXPathULong(): (unsigned long)-1 != (double)-1
+     */
     if (virXPathNode("./owner", ctxt) == NULL) {
         perms->uid = (uid_t) -1;
     } else {
-        if (virXPathLong("number(./owner)", ctxt, &v) < 0) {
+        ret = virXPathNumber("number(./owner)", ctxt, &d);
+        if (ret == 0) {
+            if (d == (double) -1) {
+                perms->uid = (uid_t) -1;
+            } else {
+                perms->uid = (uid_t) (unsigned long) d;
+                if (perms->uid != d) {
+                    ret = -2;
+                }
+            }
+        }
+        if (ret < 0) {
             virReportError(VIR_ERR_XML_ERROR,
                            "%s", _("malformed owner element"));
             goto error;
         }
-        perms->uid = (int)v;
     }
 
     if (virXPathNode("./group", ctxt) == NULL) {
         perms->gid = (gid_t) -1;
     } else {
-        if (virXPathLong("number(./group)", ctxt, &v) < 0) {
+        ret = virXPathNumber("number(./group)", ctxt, &d);
+        if (ret == 0) {
+            if (d == (double) -1) {
+                perms->gid = (gid_t)-1;
+            } else {
+                perms->gid = (gid_t) (unsigned long) d;
+                if (perms->gid != d) {
+                    ret = -2;
+                }
+            }
+        }
+        if (ret < 0) {
             virReportError(VIR_ERR_XML_ERROR,
                            "%s", _("malformed group element"));
             goto error;
         }
-        perms->gid = (int)v;
     }
 
     /* NB, we're ignoring missing labels here - they'll simply inherit */
@@ -1060,10 +1093,18 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) {
         virBufferAddLit(&buf,"    <permissions>\n");
         virBufferAsprintf(&buf,"      <mode>0%o</mode>\n",
                           def->target.perms.mode);
-        virBufferAsprintf(&buf,"      <owner>%d</owner>\n",
-                          (int) def->target.perms.uid);
-        virBufferAsprintf(&buf,"      <group>%d</group>\n",
-                          (int) def->target.perms.gid);
+        if (def->target.perms.uid == (uid_t) -1) {
+            virBufferAddLit(&buf, "      <owner>-1</owner>\n");
+        } else {
+            virBufferAsprintf(&buf, "      <owner>%u</owner>\n",
+                              (unsigned int) def->target.perms.uid);
+        }
+        if (def->target.perms.gid == (gid_t) -1) {
+            virBufferAddLit(&buf, "      <group>-1</group>\n");
+        } else {
+            virBufferAsprintf(&buf, "      <group>%u</group>\n",
+                              (unsigned int) def->target.perms.gid);
+        }
 
         if (def->target.perms.label)
             virBufferAsprintf(&buf,"      <label>%s</label>\n",
@@ -1313,11 +1354,18 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options,
     virBufferAddLit(buf,"    <permissions>\n");
     virBufferAsprintf(buf,"      <mode>0%o</mode>\n",
                       def->perms.mode);
-    virBufferAsprintf(buf,"      <owner>%u</owner>\n",
-                      (unsigned int) def->perms.uid);
-    virBufferAsprintf(buf,"      <group>%u</group>\n",
-                      (unsigned int) def->perms.gid);
-
+    if (def->perms.uid == (uid_t) -1) {
+        virBufferAddLit(buf, "      <owner>-1</owner>\n");
+    } else {
+        virBufferAsprintf(buf, "      <owner>%u</owner>\n",
+                          (unsigned int) def->perms.uid);
+    }
+    if (def->perms.gid == (gid_t) -1) {
+        virBufferAddLit(buf, "      <group>-1</group>\n");
+    } else {
+        virBufferAsprintf(buf, "      <group>%u</group>\n",
+                          (unsigned int) def->perms.gid);
+    }
 
     if (def->perms.label)
         virBufferAsprintf(buf,"      <label>%s</label>\n",
diff --git a/tests/storagevolxml2xmlout/vol-sheepdog.xml b/tests/storagevolxml2xmlout/vol-sheepdog.xml
index 2f19af8..fc04f19 100644
--- a/tests/storagevolxml2xmlout/vol-sheepdog.xml
+++ b/tests/storagevolxml2xmlout/vol-sheepdog.xml
@@ -10,8 +10,8 @@
     <format type='unknown'/>
     <permissions>
       <mode>0600</mode>
-      <owner>4294967295</owner>
-      <group>4294967295</group>
+      <owner>-1</owner>
+      <group>-1</group>
     </permissions>
   </target>
 </volume>
-- 
1.7.10.4

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]