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

[Luiz Capitulino]

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

 or

 @utf8: The data format is a json string

>  #
>  # @format: #optional the format of the data write to chardev 'memory',
> @@ -359,7 +357,7 @@
>  # Since: 1.4
>  ##
>  { 'command': 'memchar-write',
> -  'data': {'device': 'str', 'size': 'int', 'data': 'str',
> +  'data': {'device': 'str', 'data': 'str',
>             '*format': 'DataFormat'} }
>  
>  ##
> diff --git a/qemu-char.c b/qemu-char.c
> index ac5d62d..9c1dd13 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2753,9 +2753,8 @@ static bool qemu_is_chr(const CharDriverState *chr, 
> const char *filename)
>      return strcmp(chr->filename, filename);
>  }
>  
> -void qmp_memchar_write(const char *device, int64_t size,
> -                       const char *data, bool has_format,
> -                       enum DataFormat format,
> +void qmp_memchar_write(const char *device, const char *data,
> +                       bool has_format, enum DataFormat format,
>                         Error **errp)
>  {
>      CharDriverState *chr;
> @@ -2774,12 +2773,11 @@ void qmp_memchar_write(const char *device, int64_t 
> size,
>          return;
>      }
>  
> -    write_count = (gsize)size;
> -
>      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);
>      }
>  
>      ret = cirmem_chr_write(chr, write_data, write_count);
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index bbb21f3..8468f10 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -467,7 +467,7 @@ EQMP
>  
>      {
>          .name       = "memchar-write",
> -        .args_type  = "device:s,size:i,data:s,format:s?",
> +        .args_type  = "device:s,data:s,format:s?",
>          .mhandler.cmd_new = qmp_marshal_input_memchar_write,
>      },
>  
> @@ -481,7 +481,6 @@ char device.
>  Arguments:
>  
>  - "device": the name of the char device, must be unique (json-string)
> -- "size": the memory size, in bytes, should be power of 2 (json-int)
>  - "data": the source data write to memory (json-string)
>  - "format": the data format write to memory, default is
>              utf8. (json-string, optional)
> @@ -491,7 +490,6 @@ Example:
>  
>  -> { "execute": "memchar-write",
>                  "arguments": { "device": foo,
> -                               "size": 8,
>                                 "data": "abcdefgh",
>                                 "format": "utf8" } }
>  <- { "return": {} }