[Qemu-devel] [PATCH 0/4]: QMP: Fix segfault in bad input

[Qemu-devel] [PATCH 0/4]: QMP: Fix segfault in bad input

[Luiz Capitulino]

 First, we do some QError usage cleanup in handle_qmp_command() and then
really fix the bug in the last patch.

[Qemu-devel] [PATCH 1/4] QError: New QERR_QMP_BAD_INPUT_OBJECT_MEMBER

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 8d885cd..b6aaec7 100644
--- a/qerror.c
+++ b/qerror.c
@@ -173,6 +173,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Bad QMP input object",
     },
     {
+        .error_fmt = QERR_QMP_BAD_INPUT_OBJECT_MEMBER,
+        .desc      = "QMP input object member '%(member)' expects '%(expected)'",
+    },
+    {
         .error_fmt = QERR_SET_PASSWD_FAILED,
         .desc      = "Could not set password",
     },
diff --git a/qerror.h b/qerror.h
index bae08c0..c98c61a 100644
--- a/qerror.h
+++ b/qerror.h
@@ -145,6 +145,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_QMP_BAD_INPUT_OBJECT \
     "{ 'class': 'QMPBadInputObject', 'data': { 'expected': %s } }"
 
+#define QERR_QMP_BAD_INPUT_OBJECT_MEMBER \
+    "{ 'class': 'QMPBadInputObjectMember', 'data': { 'member': %s, 'expected': %s } }"
+
 #define QERR_SET_PASSWD_FAILED \
     "{ 'class': 'SetPasswdFailed', 'data': {} }"
 
-- 
1.7.0.4.297.g6555b1

[Qemu-devel] [PATCH 2/4] QMP: Use QERR_QMP_BAD_INPUT_OBJECT_MEMBER

[Luiz Capitulino]

The QERR_QMP_BAD_INPUT_OBJECT error is going to be used only
for two problems: the input is not an object or the "execute"
key is missing.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index 709b326..cd350d6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4489,7 +4489,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
         qerror_report(QERR_QMP_BAD_INPUT_OBJECT, "execute");
         goto err_input;
     } else if (qobject_type(obj) != QTYPE_QSTRING) {
-        qerror_report(QERR_QMP_BAD_INPUT_OBJECT, "string");
+        qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "execute", "string");
         goto err_input;
     }
 
-- 
1.7.0.4.297.g6555b1

[Qemu-devel] [PATCH 3/4] QError: Improve QERR_QMP_BAD_INPUT_OBJECT desc

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/qerror.c b/qerror.c
index b6aaec7..034c7de 100644
--- a/qerror.c
+++ b/qerror.c
@@ -170,7 +170,7 @@ static const QErrorStringTable qerror_table[] = {
     },
     {
         .error_fmt = QERR_QMP_BAD_INPUT_OBJECT,
-        .desc      = "Bad QMP input object",
+        .desc      = "Expected '%(expected)' in QMP input",
     },
     {
         .error_fmt = QERR_QMP_BAD_INPUT_OBJECT_MEMBER,
-- 
1.7.0.4.297.g6555b1

[Qemu-devel] [PATCH 4/4] QMP: Check "arguments" member's type

[Luiz Capitulino]

Otherwise the following input crashes QEMU:

{ "execute": "migrate", "arguments": "tcp:0:4446" }

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index cd350d6..91d7da5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4522,6 +4522,9 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
     obj = qdict_get(input, "arguments");
     if (!obj) {
         args = qdict_new();
+    } else if (qobject_type(obj) != QTYPE_QDICT) {
+        qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "arguments", "object");
+        goto err_input;
     } else {
         args = qobject_to_qdict(obj);
         QINCREF(args);
-- 
1.7.0.4.297.g6555b1

Re: [Qemu-devel] [PATCH 0/4]: QMP: Fix segfault in bad input

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

>  First, we do some QError usage cleanup in handle_qmp_command() and then
> really fix the bug in the last patch.

The bug is that we neglect to check that command object member
"arguments" is an object before we access its members.  Crashes when
it's not an object.

The rest of the patch series tweaks diagnostics of malformed command
objects:

* Split QERR_QMP_BAD_INPUT_OBJECT_MEMBER off QERR_QMP_BAD_INPUT_OBJECT.
  I don't care for that at all.  These errors are all of the "hello
  client, you're too broken to live, go away" kind.  Clients won't be
  able to do anything useful with a fine-grained error class there.

* Improve the human-readable messages.  This could be occasionally
  useful for debugging, I guess.

That said, I'm not opposed to merging as is.  There's plenty of useless
error detail already, with more to come, so adding to the pile doesn't
bother me.