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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/05/2013 09:22 AM, Markus Armbruster 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(-)
>
>>      if (has_format && (format == DATA_FORMAT_BASE64)) {
>>          write_data = g_base64_decode(data, &write_count);
>>      } else {
>>          write_data = (uint8_t *)data;
>> +        write_count = strlen(data);
>>      }
>
> Obviously, base64 is the only way to write an embedded NUL.  But what

Consider the JSON string "this \\u0000 is fun".  But our JSON parser
chokes on it, so we can ignore it until that's fixed.  See my "[PATCH]
check-qjson: More thorough testing of UTF-8 in strings", specifically
the comment right at the beginning of utf8_string().

> happens if the user requests base64 encoding, but the utf8 string that
> got passed in through JSON is not a valid base64-encoded string?  Does
> g_base64_decode report an error in that case, and should you be handling
> the error here?

Good question.  I wish it had occured to GLib developers:
http://developer.gnome.org/glib/stable/glib-Base64-Encoding.html#g-base64-decode

Seriously, I need to find out what the heck g_base64_decode() does when
it's fed crap.  If it fails in a reliable and detectable manner, I need
to handle the failure.  If not, I need to replace the function.

Moreover, I should document which characters outside the base64 alphabet
are silently ignored, if any.  All whitespace?  Just newlines?