Re: [libvirt] [PATCH 4/4] Need better validation of <sysinfo> uuid
On 05/10/2013 10:57 AM, Eric Blake wrote:
On 05/10/2013 05:07 AM, Martin Kletzander wrote:
>
> I must admit I have no idea why we allow spaces and hyphens everywhere
> in the UUID string. Changing it to proper UUID parser is not just
> fixing this issue, but also removing more lines than adding, since the
> parser could get halved. That's why I was thinking more about taking
> that route. I'd love to hear some input from someone else in case this
> looks like it would break something. IMNSHO that's a bad usage unless
> we explicitly allowed spaces and hyphens somewhere in the docs.
We're lax in what we parse and strict in what we output. We can't
change the lax parser, because there really is code out there that
doesn't format valid uuids itself and relies on libvirt to clean up the
mess, but I don't think the current lax parser is all that bad (as long
as we see exactly enough hex digits after skipping all dashes and
spaces, we have something we can then turn into valid output).
> If I read the code correctly, we are properly saving the UUID in the
> definition in our format and using virUUIDFormat() properly to build the
> command line. Aren't we doing the same with system_uuid? If not, that
> could be helpful even for the future (actually until someone files a bug
> about dumpxml generating invalid UUIDs ;-) )
>
> I can't help myself, but parsing, formatting and throwing away the
> result just to check the validity seems weird.
If we parse and then format, we should use what we formatted from then
on, instead of the original user's (potentially unusual) formatting.
It would seem that you are thus advocating we change what was read
to be formatted properly and don't tell the user we did that, e.g.:
if (def->sysinfo->system_uuid != NULL) {
unsigned char uuidbuf[VIR_UUID_BUFLEN];
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
if (virUUIDParse(def->sysinfo->system_uuid, uuidbuf) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("malformed uuid element"));
+ "%s", _("malformed <sysinfo> uuid
element"));
goto error;
}
if (uuid_generated)
@@ -11593,6 +11594,45 @@ virDomainDefParseXML(xmlDocPtr xml,
"<sysinfo>"));
goto error;
}
+ /* Although we've validated the UUID as good, virUUIDParse() is
+ * lax with respect to allowing extraneous "-" and " ",
but the
+ * underlying hypervisor may be less forgiving. Use virUUIDFormat()
+ * to validate format in xml is right. If not, then format it
+ * properly so that it's used correctly later.
+ */
+ virUUIDFormat(uuidbuf, uuidstr);
+ if (memcmp(def->sysinfo->system_uuid, uuidstr,
+ VIR_UUID_STRING_BUFLEN) != 0) {
+ VIR_FREE(def->sysinfo->system_uuid);
+ if (VIR_STRDUP(def->sysinfo->system_uuid, uudstr) < 0) {
+ goto error;
+ }
+ }
+ }
Would it be better to see a v2 with everything?
Or push patches 1-3 and make a v2 of just this one?
John