diff mbox

[v2] qapi: fix memory leak in QmpOutputVisitor

Message ID 32b6baf4-cd23-c277-4dfb-79922c710efe@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Blake Oct. 21, 2016, 10:10 p.m. UTC
On 10/21/2016 04:32 PM, Eric Blake wrote:
> In fact, applying this patch regresses to the very state that f24582d
> tried to prevent.  However, I'm unable to see a difference in valgrind
> on tests/test-qmp-output-visitor either with or without this patch,
> which sadly means our testsuite is not actually testing this scenario.
> 
>>> Should this go into -stable?
> 
> NACK.
> 
> As mentioned in the v1 thread, the leak that Pino was seeing is fixed by
> http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg04023.html
> I don't think we don't want this patch.

And below is my hack to test-qmp-output-visitor, which shows (at least
under valgrind) that we don't want the patch. I should probably turn
that into a formal patch submission; but should I piggyback the existing
test or add a new one?  With the patch omitted, valgrind is happy; with
the patch applied, I see:

/visitor/output/struct-errors: ==8458== Invalid read of size 8
==8458==    at 0x175DC2: qobject_decref (qobject.h:81)
==8458==    by 0x1767A5: qentry_destroy (qdict.c:413)
==8458==    by 0x17690B: qdict_destroy_obj (qdict.c:451)
==8458==    by 0x1784AB: qobject_destroy (qobject.c:29)
==8458==    by 0x171927: qobject_decref (qobject.h:83)
==8458==    by 0x1721B4: qmp_output_free (qmp-output-visitor.c:223)
==8458==    by 0x16ED8E: visit_free (qapi-visit-core.c:34)
==8458==    by 0x130F39: visitor_output_teardown
(test-qmp-output-visitor.c:44)
==8458==    by 0x130FE1: visitor_reset (test-qmp-output-visitor.c:59)
==8458==    by 0x1326EF: test_visitor_out_struct_errors
(test-qmp-output-visitor.c:283)
==8458==    by 0x50A8940: ??? (in /usr/lib64/libglib-2.0.so.0.4800.2)
==8458==    by 0x50A8B0E: ??? (in /usr/lib64/libglib-2.0.so.0.4800.2)
==8458==  Address 0x8259db8 is 8 bytes inside a block of size 4,120 free'd
==8458==    at 0x4C2CD5A: free (vg_replace_malloc.c:530)
==8458==    by 0x5088F2D: g_free (in /usr/lib64/libglib-2.0.so.0.4800.2)
==8458==    by 0x176937: qdict_destroy_obj (qdict.c:456)
==8458==    by 0x1784AB: qobject_destroy (qobject.c:29)
==8458==    by 0x171927: qobject_decref (qobject.h:83)
==8458==    by 0x1721B4: qmp_output_free (qmp-output-visitor.c:223)
==8458==    by 0x16ED8E: visit_free (qapi-visit-core.c:34)
==8458==    by 0x130F39: visitor_output_teardown
(test-qmp-output-visitor.c:44)
==8458==    by 0x130FE1: visitor_reset (test-qmp-output-visitor.c:59)
==8458==    by 0x1326EF: test_visitor_out_struct_errors
(test-qmp-output-visitor.c:283)
==8458==    by 0x50A8940: ??? (in /usr/lib64/libglib-2.0.so.0.4800.2)
==8458==    by 0x50A8B0E: ??? (in /usr/lib64/libglib-2.0.so.0.4800.2)
==8458==  Block was alloc'd at
==8458==    at 0x4C2DA60: calloc (vg_replace_malloc.c:711)
==8458==    by 0x5088E70: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.4800.2)
==8458==    by 0x175EAF: qdict_new (qdict.c:33)
==8458==    by 0x171CAB: qmp_output_start_struct (qmp-output-visitor.c:108)
==8458==    by 0x16EE4A: visit_start_struct (qapi-visit-core.c:47)
==8458==    by 0x1326E3: test_visitor_out_struct_errors
(test-qmp-output-visitor.c:282)
==8458==    by 0x50A8940: ??? (in /usr/lib64/libglib-2.0.so.0.4800.2)
==8458==    by 0x50A8B0E: ??? (in /usr/lib64/libglib-2.0.so.0.4800.2)
==8458==    by 0x50A8B0E: ??? (in /usr/lib64/libglib-2.0.so.0.4800.2)
==8458==    by 0x50A8D1D: g_test_run_suite (in
/usr/lib64/libglib-2.0.so.0.4800.2)
==8458==    by 0x50A8D40: g_test_run (in /usr/lib64/libglib-2.0.so.0.4800.2)
==8458==    by 0x134F8D: main (test-qmp-output-visitor.c:899)

Comments

Markus Armbruster Oct. 24, 2016, 6:56 a.m. UTC | #1
Eric Blake <eblake@redhat.com> writes:

> On 10/21/2016 04:32 PM, Eric Blake wrote:
>> In fact, applying this patch regresses to the very state that f24582d
>> tried to prevent.  However, I'm unable to see a difference in valgrind
>> on tests/test-qmp-output-visitor either with or without this patch,
>> which sadly means our testsuite is not actually testing this scenario.
>> 
>>>> Should this go into -stable?
>> 
>> NACK.
>> 
>> As mentioned in the v1 thread, the leak that Pino was seeing is fixed by
>> http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg04023.html
>> I don't think we don't want this patch.
>
> And below is my hack to test-qmp-output-visitor, which shows (at least
> under valgrind) that we don't want the patch. I should probably turn
> that into a formal patch submission; but should I piggyback the existing
> test or add a new one?  With the patch omitted, valgrind is happy; with
> the patch applied, I see:
>
> /visitor/output/struct-errors: ==8458== Invalid read of size 8
> ==8458==    at 0x175DC2: qobject_decref (qobject.h:81)
> ==8458==    by 0x1767A5: qentry_destroy (qdict.c:413)
> ==8458==    by 0x17690B: qdict_destroy_obj (qdict.c:451)
> ==8458==    by 0x1784AB: qobject_destroy (qobject.c:29)
> ==8458==    by 0x171927: qobject_decref (qobject.h:83)
> ==8458==    by 0x1721B4: qmp_output_free (qmp-output-visitor.c:223)
> ==8458==    by 0x16ED8E: visit_free (qapi-visit-core.c:34)
> ==8458==    by 0x130F39: visitor_output_teardown
> (test-qmp-output-visitor.c:44)
> ==8458==    by 0x130FE1: visitor_reset (test-qmp-output-visitor.c:59)
> ==8458==    by 0x1326EF: test_visitor_out_struct_errors
> (test-qmp-output-visitor.c:283)
[...]
>
>
> diff --git i/tests/test-qmp-output-visitor.c
> w/tests/test-qmp-output-visitor.c
> index 5e926d3..f1b5591 100644
> --- i/tests/test-qmp-output-visitor.c
> +++ w/tests/test-qmp-output-visitor.c
> @@ -265,18 +265,22 @@ static void
> test_visitor_out_struct_errors(TestOutputVisitorData *data,
>      EnumOne bad_values[] = { ENUM_ONE__MAX, -1 };
>      UserDefOne u = {0};
>      UserDefOne *pu = &u;
> -    Error *err;
> +    Error *err = NULL;
>      int i;
>
> +    /* First check: error within struct is detected */
>      for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
> -        err = NULL;
>          u.has_enum1 = true;
>          u.enum1 = bad_values[i];
>          visit_type_UserDefOne(data->ov, "unused", &pu, &err);
> -        g_assert(err);
> -        error_free(err);
> +        error_free_or_abort(&err);
>          visitor_reset(data);
>      }

Just cleanup so far.

> +
> +    /* Second check: aborting visit early doesn't leak */
> +    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
> +    visit_start_struct(data->ov, "nested", NULL, 0, &error_abort);
> +    visitor_reset(data);
>  }

Since this isn't about errors, adding it to
test_visitor_out_struct_errors() is perhaps questionable.  Use your
judgement.
diff mbox

Patch

diff --git i/tests/test-qmp-output-visitor.c
w/tests/test-qmp-output-visitor.c
index 5e926d3..f1b5591 100644
--- i/tests/test-qmp-output-visitor.c
+++ w/tests/test-qmp-output-visitor.c
@@ -265,18 +265,22 @@  static void
test_visitor_out_struct_errors(TestOutputVisitorData *data,
     EnumOne bad_values[] = { ENUM_ONE__MAX, -1 };
     UserDefOne u = {0};
     UserDefOne *pu = &u;
-    Error *err;
+    Error *err = NULL;
     int i;

+    /* First check: error within struct is detected */
     for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-        err = NULL;
         u.has_enum1 = true;
         u.enum1 = bad_values[i];
         visit_type_UserDefOne(data->ov, "unused", &pu, &err);
-        g_assert(err);
-        error_free(err);
+        error_free_or_abort(&err);
         visitor_reset(data);
     }
+
+    /* Second check: aborting visit early doesn't leak */
+    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
+    visit_start_struct(data->ov, "nested", NULL, 0, &error_abort);
+    visitor_reset(data);
 }