Re: [Qemu-devel] [PATCH for-1.4 01/12] qmp: Fix design bug and read beyond buffer in memchar-write

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Tue,  5 Feb 2013 17:22:04 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> Command memchar-write takes data and size parameter.  Begs the
>> question what happens when data doesn't match size.
>> 
>> With format base64, qmp_memchar_write() copies the full data argument,
>> regardless of size argument.
>> 
>> With format utf8, qmp_memchar_write() copies size bytes from data,
>> happily reading beyond data.  Copies crap from the heap or even
>> crashes.
>> 
>> Drop the size parameter, and always copy the full data argument.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  hmp.c            | 4 +---
>>  qapi-schema.json | 4 +---
>>  qemu-char.c      | 8 +++-----
>>  qmp-commands.hx  | 4 +---
>>  4 files changed, 6 insertions(+), 14 deletions(-)
>> 
>> diff --git a/hmp.c b/hmp.c
>> index 1689e6f..9fdf1ce 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -664,13 +664,11 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict)
>>  
>>  void hmp_memchar_write(Monitor *mon, const QDict *qdict)
>>  {
>> -    uint32_t size;
>>      const char *chardev = qdict_get_str(qdict, "device");
>>      const char *data = qdict_get_str(qdict, "data");
>>      Error *errp = NULL;
>>  
>> -    size = strlen(data);
>> -    qmp_memchar_write(chardev, size, data, false, 0, &errp);
>> +    qmp_memchar_write(chardev, data, false, 0, &errp);
>>  
>>      hmp_handle_error(mon, &errp);
>>  }
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index cdd8384..9e2cbbd 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -346,8 +346,6 @@
>>  #
>>  # @device: the name of the memory char device.
>>  #
>> -# @size: the size to write in bytes.
>> -#
>>  # @data: the source data write to memchar.
>
> I agree with this change, but I have an observation.
>
> This is fine for QMP on the wire, as data will always be a string. But
> for QMP as a C interface, this means that we require data to be passed
> as a string (ie. null-terminated).
>
> I've just discussed this with Markus on IRC, and his suggestion is
> that the C interface should be different. I agree with him.
>
> But, should we at least make this explicity? Like changing the
> @DataFormat doc for @utf8 to:
>
>  @utf8: The data format is a null-terminated utf8 string

But what does that mean?  See below.

>  or
>
>  @utf8: The data format is a json string

That's trivially true for any json-string argument, including the data
argument with format base64.

Have a look at PATCH 12/12:

 # @memchar-write:
[...]
+# @format: #optional data encoding (default 'utf8').
+#          - base64: data must be base64 encoded text.  It's binary
+#            decoding gets written.
+#          - utf8: data's UTF-8 encoding is written
+#          - data itself is always Unicode regardless of format, like
+#            any other string.
[...]
 # @memchar-read:
[...]
+# @format: #optional data encoding (default 'utf8').
+#          - base64: the data read is returned in base64 encoding.
+#          - utf8: the data read is interpreted as UTF-8.
[Bug doc snipped for clarity...]
+#          - The return value is always Unicode regardless of format,
+#            like any other string.

Back to your question, namely how to document enumeration DataFormat.
Perhaps:

##
# @DataFormat:
#
# An enumeration of data format.
#
# @utf8: Data is a UTF-8 string (RFC 3629)
#
# @base64: Data is a Base64 encoded binary (RFC 3548)
#
# Since: 1.4
##

This omits the fact that data itself is a JSON string, because that's
"obvious".  Hmm.  Let me try -v:

##
# @DataFormat:
#
# An enumeration describing how a data string is to be converted.
#
# @utf8: The string is to be converted to UTF-8 (RFC 3629).  Always
#        possible, because the string is in Unicode.
#
# @base64: The string must be Base64 (RFC 3548) encoded binary, and is
#          to be converted back to binary.
#
# Since: 1.4
##