[PATCH 4/4] storage: fix uid_t|gid_t handling on 32 bit Linux

[Philipp Hahn <hahn@xxxxxxxxxxxxx>]

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 ALLONE := +(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)ALLONE
virXPathULong(): (unsigned long)-1 != (double)-1

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

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

Signed-off-by: Philipp Hahn <hahn@xxxxxxxxxxxxx>
---
 src/conf/storage_conf.c |   74 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 60 insertions(+), 14 deletions(-)

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 9134a22..b267f00 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -665,7 +665,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
                         const char *permxpath,
                         int defaultmode) {
     char *mode;
-    long v;
+    double d;
     int ret = -1;
     xmlNodePtr relnode;
     xmlNodePtr node;
@@ -699,26 +699,57 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
         VIR_FREE(mode);
     }
 
+    /* uid_t and gid_t are *opaque* types: on Linux they're u32, on Windows64
+     * they're u64, on Solaris they were s32 in the past. There may be others.
+     *
+     * Unfortunately libvirt uses the value -1 to mean "current process", which
+     * on Linux32 gets converted to ALLONE:=+(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)ALLONE
+     * 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 +1091,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 +1352,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",
-- 
1.7.10.4

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