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

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(a)univention.de&gt; --- [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