Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/4] qapi: output visitor crashes qemu if it encounters a NULL value

[Michael Roth]

Quoting Luiz Capitulino (2014-05-14 13:25:16)
> On Wed, 14 May 2014 20:29:37 +0300
> Marcel Apfelbaum <address@hidden> wrote:
> 
> > On Wed, 2014-05-14 at 19:00 +0200, Andreas Färber wrote:
> > > Am 13.05.2014 21:08, schrieb Eric Blake:
> > > > On 05/13/2014 11:36 AM, Andreas Färber wrote:
> > > >> Am 07.05.2014 16:42, schrieb Marcel Apfelbaum:
> > > >>> A NULL value is not added to visitor's stack, but there is no
> > > >>> check for that when the visitor tries to return that value,
> > > >>> leading to Qemu crash.
> > > >>> 
> > > >>> Reviewed-by: Eric Blake <address@hidden> Signed-off-by:
> > > >>> Marcel Apfelbaum <address@hidden>
> > > >> 
> > > >> Where does the Rb come from on this v1? Is it in any tree
> > > >> already?
> > > >> 
> > > > 
> > > > The (weak) R-b was here: 
> > > > https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg02861.html
> > > 
> > > Thanks.
> > > > 
> > > So Luiz was okay with it too, but his last message seems to be
> > > indicating this needs to be fixed somewhere else, too:
> > > 
> > > https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg05228.html
> > > https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg00217.html
> > > 
> > > Can/should that be addressed as a follow-up? Or is there a test case
> > > that breaks?
> > Simple and "popular" test case: the user does not use the -kernel-cmdline 
> > parameter.
> > The patch is needed because otherwise the main function will fail
> > if no value is passed by the user to string parameters. 
> > 
> > Regarding Luiz's concern, it can be a follow-up as I am not aware of
> > any problem with that.
> 
> My concern was that I wasn't sure if this is the right fix for the issue
> or if it's papering over the real bug. I quickly checked the code and it
> seemed to make sense, but I didn't have time to study it deeper.

Not sure the fix is bad or not, but the cause might be a little more subtle
than NULL string values as mentioned in the other thread. QmpOutputVisitor
encodes NULL strings as "" via qmp_output_type_str(), so the problem doesn't
seem to lie there: it shouldn't generate NULL values on the stack.

I think the real issue is that object_property_get_str() actually calls an
accessor via property_get_str to get the string, then explicitly *skips*
the call to visit_type_str() if it is NULL (as it would be in the case of,
say, kernel_cmdline option being NULL). So I wonder if maybe the real issue
we're fixing is a corner case where you call qmp_output_get_qobject() on
an "empty" QmpOutputVisitor.

Surprised that's not covered by tests, but didn't see any coverage doing
a cursory glance. Actually, might as well just add one..

diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index e073d83..f190eaa 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -434,6 +434,17 @@ static void test_visitor_out_union(TestOutputVisitorData 
*data,
     QDECREF(qdict);
 }
 
+static void test_visitor_out_empty(TestOutputVisitorData *data,
+                                   const void *unused)
+{
+    QObject *arg;
+    QDict *qdict;
+
+    arg = qmp_output_get_qobject(data->qov);
+    qdict = qobject_to_qdict(arg);
+    QDECREF(qdict);
+}
+
 static void init_native_list(UserDefNativeListUnion *cvalue)
 {
     int i;
@@ -782,6 +793,8 @@ int main(int argc, char **argv)
                             &out_visitor_data, 
test_visitor_out_list_qapi_free);
     output_visitor_test_add("/visitor/output/union",
                             &out_visitor_data, test_visitor_out_union);
+    output_visitor_test_add("/visitor/output/empty",
+                            &out_visitor_data, test_visitor_out_empty);
     output_visitor_test_add("/visitor/output/native_list/int",
                             &out_visitor_data, 
test_visitor_out_native_list_int);
     output_visitor_test_add("/visitor/output/native_list/int8",

address@hidden:~/w/qemu-build$ tests/test-qmp-output-visitor 
/visitor/output/int: OK
/visitor/output/bool: OK
/visitor/output/number: OK
/visitor/output/string: OK
/visitor/output/no-string: OK
/visitor/output/enum: OK
/visitor/output/enum-errors: OK
/visitor/output/struct: OK
/visitor/output/struct-nested: OK
/visitor/output/struct-errors: OK
/visitor/output/list: OK
/visitor/output/list-qapi-free: OK
/visitor/output/union: OK
/visitor/output/empty: Segmentation fault (core dumped)

So I guess the question is whether we should support converting an empty
QmpOutputVisitor to a QObject. I would say yes, and that a NULL value is
probably the most reasonable value.

I would ask that commit/code is a little more explicit about what corner case
is being handled though, and that something like the above unit test be
included with the series.

> 
> We could ask Michael Roth or Anthony, but I wouldn't hold this series
> because of that. Here's my ACK if you need it:
> 
> Acked-by: Luiz Capitulino <address@hidden>