On Tue, 5 Feb 2013 17:22:04 +0100
Markus Armbruster <arm...@redhat.com> 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 <arm...@redhat.com>
> ---
> 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": {} }
