[libvirt] [PATCH 4/4] Need better validation of <sysinfo> uuid

[John Ferlan]

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