Re: [libvirt] [PATCH 4/4] Need better validation of <sysinfo> uuid
On 05/09/2013 06:42 PM, John Ferlan wrote:
On 05/09/2013 06:27 AM, Martin Kletzander wrote:
> On 04/30/2013 08:19 PM, John Ferlan wrote:
>> If the <sysinfo> system table 'uuid' field is improperly
formatted,
>> then qemu will fail to start the guest with the error:
>>
>> virsh start dom
>> error: Failed to start domain dom
>> error: internal error process exited while connecting to monitor: Invalid SMBIOS
UUID string
>>
>> In this case the "system_uuid" field was
a94b4335-6a14-8bc4-d6da-f7ea590b68-16
>> which passed the virUUIDParse() code because 32 hexadecimal digits were found
>> and the extra hyphen in the last section was ignored.
>>
>> Add checks to not only parse the read field, but then use virUUIDFormat() to
>> validate that what gets formatted matches what was read - if not, then fail
>> the edit.
>
> I feel like we could do better. Either 1) such UUID is not valid (which
> I think it really isn't [1]) and we should fail when when parsing it or
> 2) it is valid, but qemu doesn't like it, so we should fixup the UUID
> before passing it to qemu (and maybe request proper UUID parsing from
> qemu guys).
>
> What do you think?
That seemed to be a much larger fish to fry than I wanted to take on
with this particular patch. That is by avoiding the dashes ("-") in
virUUIDParse() already leads to this particular validation. The numbers
are all correct, it's just additional dashes that were a problem.
Changing that algorithm to be less forgiving led me down a path of
wondering why that code is forgiving w/r/t "-" and " " and what
could
fall out from changing that?
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.
I suppose for this particular case it could have been possible to
take
the provided 'sysinfo' uuid field in qemuBuildSmbiosSystemStr() and make
the virUUIDParse() and virUUIDFormat() calls prior to the
virBufferAsprintf() which adds it to the command line. If that's
desired, I can take that route.
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.
[...]
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
"%s",
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>
> Pre-existing, but it looks like another abuse of internal error.
>
To be consistent w/ the bios_date change, I changed from INTERNAL_ERROR
to XML_DETAIL
Great, I know we can never eliminate even half of them, but we can
definitely try :)
Martin