Re: [Qemu-devel] [PATCH v8 6/8] vmport_rpc: Add QMP access to vmport_rpc object.

[Markus Armbruster]

Don Slutz <address@hidden> writes:

> On 07/03/15 13:56, Markus Armbruster wrote:
>> Don Slutz <address@hidden> writes:
>>
>>> On 07/02/15 10:11, Markus Armbruster wrote:
>>>> Don Slutz <address@hidden> writes:
[...]
>>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>>> index 106008c..5d9e9e2 100644
>>>>> --- a/qapi-schema.json
>>>>> +++ b/qapi-schema.json
>>>>> @@ -1429,6 +1429,107 @@
>>>>>    { 'command': 'inject-nmi' }
>>>>>      ##
>>>>> +# @VmportAction:
>>>>> +#
>>>>> +# An enumeration of actions that can be requested via vmport RPC.
>>>>> +#
>>>>> +# @reboot: Ask the guest via VMware tools to reboot
>>>>> +#
>>>>> +# @halt: Ask the guest via VMware tools to halt
>>>>> +#
>>>>> +# Since: 2.4
>>>>> +##
>>>>> +{ 'enum': 'VmportAction',
>>>>> +  'data': [ 'reboot', 'halt' ] }
>>>>> +
>>>>> +##
>>>>> +# @inject-vmport-action:
>>>>> +#
>>>>> +# Injects a VMWare Tools action to the guest.
>>>>> +#
>>>>> +# Returns:  If successful, nothing
>>>>> +#
>>>>> +# Since:  2.4
>>>>> +#
>>>>> +##
>>>>> +{ 'command': 'inject-vmport-action',
>>>>> +  'data': {'action': 'VmportAction'} }
>>>>> +
>>>>> +##
>>>>> +# @vmport-guestinfo-set:
>>>>> +#
>>>>> +# Set a VMWare Tools guestinfo key to a value
>>>>> +#
>>>>> +# @key: the key to set
>>>>> +#
>>>>> +# @value: The data to set the key to
>>>>> +#
>>>>> +# @format: #optional value encoding (default 'utf8').
>>>>> +#          - base64: value is assumed to be base64 encoded text.  Its 
>>>>> binary
>>>>> +#            decoding gets set.
>>>> Shouldn't you copy ringbuf-write's bug note?
>>>>
>>>>      #            Bug: invalid base64 is currently not rejected.
>>>>
>>> Nope.  From the cover letter:
>>> ----------------------------------------------------------------------------------------
>>>
>>> Much more on format=base64:
>>>
>>> If there is a bug it is in GLIB.  However the Glib reference manual
>>> refers to RFC 1421 and RFC 2045 and MIME encoding.  Based on all
>>> that (which seems to match:
>>>
>>> http://en.wikipedia.org/wiki/Base64
>>>
>>> ) MIME states that all characters outside the (base64) alphabet are
>>> to be ignored.  Testing shows that g_base64_decode() does this.
>>>
>>> The confusion is that most non-MIME uses reject a base64 string that
>>> contain characters outside the alphabet.  I was just following the
>>> other uses of base64 in this file.
>>>
>>> DataFormat refers to RFC 3548, which has the info:
>>>
>>> "
>>>     Implementations MUST reject the encoding if it contains
>>>     characters outside the base alphabet when interpreting base
>>>     encoded data, unless the specification referring to this document
>>>     explicitly states otherwise.  Such specifications may, as MIME
>>>     does, instead state that characters outside the base encoding
>>>     alphabet should simply be ignored when interpreting data ("be
>>>     liberal in what you accept").
>>> "
>>>
>>> So with GLIB going the MIME way, I do not think this is a QEMU bug
>>> (you could consider this a GLIB bug, but the document I found says
>>> that GLIB goes the MIME way and so does not reject anything).
>>> ----------------------------------------------------------------------------------------
>>>
>>>
>>> Based on all this, I did drop it.
>> The QMP user isn't at all interested whether QEMU or GLib or any other
>> library screws this up, only in what the command's specification is, and
>> whatever bugs its implementation may have.
>>
>> We obviously have leeway with the specification.  We *can* override RFC
>> 3548 by "explicitly stating otherwise".
>>
>> For ringbuf-write, we deliberately chose not to.  Instead we declared
>> the non-rejection of invalid characters a bug.
>>
>> The exact same reasoning applies to your command.  If you disagree with
>> the reasoning, it's certainly open to debate.
>
> I feel that in real use (not testing) only valid base64 would be passed 
> in.  However I do not want to delay this patch on this issue.
>
>
>> Regardless of what consensus emerges, all uses of base64 DataFormat in
>> QMP should be consistent.  Right now they aren't with your patch
>> applied.
>
> Ok, I will copy the "bug comment line" so that it is consistent.

Thanks!

>>>>> +#          - utf8: value's UTF-8 encoding is used to set.
>>>>> +#
>>>>> +# Returns: Nothing on success
>>>>> +#
>>>>> +# Since: 2.4
>>>>> +##
>>>>> +{ 'command': 'vmport-guestinfo-set',
>>>>> +  'data': {'key': 'str', 'value': 'str',
>>>>> +           '*format': 'DataFormat'} }
>>>>> +
>>>>> +##
>>>>> +# @VmportGuestInfoValue:
>>>>> +#
>>>>> +# Information about a single VMWare Tools guestinfo
>>>>> +#
>>>>> +# @value: The value for a key
>>>>> +#
>>>>> +# Since: 2.4
>>>>> +##
>>>>> +{ 'struct': 'VmportGuestInfoValue', 'data': {'value': 'str'} }
>>>>> +
>>>>> +##
>>>>> +# @vmport-guestinfo-get:
>>>>> +#
>>>>> +# Get a VMWare Tools guestinfo value for a key
>>>>> +#
>>>>> +# @key: the key to get
>>>> Ignorant question: is the set of keys fixed (e.g. specified by some
>>>> authority), completely dynamic (i.e. whatever has been set), or
>>>> something else?
>>> completely dynamic
>> Thanks.
>>
>>>>> +#
>>>>> +# @format: #optional data encoding (default 'utf8').
>>>>> +#          - base64: the value is returned in base64 encoding.
>>>>> +#          - utf8: the value is interpreted as UTF-8.
>>>>> +#
>>>>> +# Returns: value for the guest info key
>>>> ringbuf-read carries this bug note:
>>>>
>>>> #            Bug: can screw up when the buffer contains invalid UTF-8
>>>> #            sequences, NUL characters, after the ring buffer lost
>>>> #            data, and when reading stops because the size limit is
>>>> #            reached.
>>>>
>>>> Doesn't it apply here, too, with "the buffer" replaced?
>>> Not directly.  The best description I have so far to match the FIXME
>>> would be:
>>>
>>> -----------------------------------------------------------------------------------
>>> Bug: format=utf8 can cause QEMU to screw up (may crash) when the value
>>> contains invalid UTF-8
>>>
>>> sequences. NUL characters will cause loss of data.
>>>
>>> ----------------------------------------------------------------------------------
>>>
>>> I have not spent the time investigating the issues with QMP and
>>> invalid utf8 because it looks to be very wide spread and not just
>>> restricted to this one routine.
>> The problem with ringbuf-read with format=utf8 is that it attempts to
>> interpret the ring buffer's contents, which could be anything, as UTF-8
>> by design (not a problem), and the code doing so is insufficiently
>> robust (this is the bug; patch welcome, as usual).
>>
>> As far as I can tell, vmport-guestinfo-get is a similar case: the value
>> associated with the key could again be anything (if written with
>> format=base64), and we attempt to interpret it as UTF-8 by design.
>>
>> What makes you think the problem is very widespread?
>
> I only did a quick look and did not find what I would have expected.  I 
> will do more looking and reply with those results.

Appreciated!

> I am happy to apply either the above Bug comment or an adjusted version 
> of the ringbuf-read one for now (I have been working on getting this 
> accepted for a while now, 1st posted on Fri, 30 Jan 2015 16:06:24 -0500).

We can certainly discuss declaring the sloppy base64 parsing a feature
across the board.  You can choose to make your patch series depend on
the outcome of that discussion, but you absolutely don't have to: just
follow ringbuf-read precedence now.  If we later decide to change our
base64 parsing rules, we'll have to fix up two places rather than one,
but that's fine.

Getting patches in can be arduous and frustrating.  Thanks for hanging
in.  I wish I had found the time to review an earlier revision, could
have saved you an iteration.