After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 778993 - GDBusClientModule memory leak in generated code
GDBusClientModule memory leak in generated code
Status: RESOLVED FIXED
Product: vala
Classification: Core
Component: D-Bus
0.35.x
Other All
: Normal normal
: ---
Assigned To: Vala maintainers
Vala maintainers
Depends on:
Blocks:
 
 
Reported: 2017-02-21 01:59 UTC by mrmacete
Modified: 2017-02-21 15:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix memleak in GDBusClientModule generated code (1.41 KB, patch)
2017-02-21 01:59 UTC, mrmacete
none Details | Review
Updated patch with comment (1.41 KB, patch)
2017-02-21 14:12 UTC, mrmacete
none Details | Review
Updated patch with comment (again) (1.47 KB, patch)
2017-02-21 14:27 UTC, mrmacete
none Details | Review
Patch generated with git format-patch (1.80 KB, application/mbox)
2017-02-21 14:38 UTC, mrmacete
  Details
Patch generated with git format-patch (1.80 KB, patch)
2017-02-21 14:40 UTC, mrmacete
committed Details | Review

Description mrmacete 2017-02-21 01:59:20 UTC
Created attachment 346294 [details] [review]
Fix memleak in GDBusClientModule generated code

In the code generated by GDBusClientModule,when calling g_dbus_connection_send_message_with_reply_finish, the result of g_task_propagate_pointer is leaked, since the called function doesn't take ownership of it.

The proposed solution is to keep a reference to the g_task_propagate_pointer return value, and call g_object_unref after passing it down to the function which uses it.
Comment 1 Carlos Garnacho 2017-02-21 11:44:27 UTC
Review of attachment 346294 [details] [review]:

Good catch :). g_task_propagate_pointer() indeed transfers ownership of the returned pointer to the caller, so it must be freed here.

::: codegen/valagdbusclientmodule.vala
@@ +742,3 @@
 				ccode.add_assignment (new CCodeIdentifier ("_reply_message"), ccall);
+
+				var unref_inner_res = new CCodeFunctionCall (new CCodeIdentifier ("g_object_unref"));

It took me a while to convince myself that the returned object can't be NULL here. given g_task_propagate_pointer() can possibly return NULL according to docs, I think it'd still be clearer here if this were NULL-safe.
Comment 2 mrmacete 2017-02-21 14:12:40 UTC
Created attachment 346358 [details] [review]
Updated patch with comment

Updated the patch with a comment on NULL-safety
Comment 3 mrmacete 2017-02-21 14:27:07 UTC
Created attachment 346359 [details] [review]
Updated patch with comment (again)

This one is correct (earlier i accidentally re-uploaded the old one)
Comment 4 Rico Tzschichholz 2017-02-21 14:30:22 UTC
Please attach patches created with "git format-patch" for proper credits.
Comment 5 mrmacete 2017-02-21 14:38:07 UTC
Created attachment 346363 [details]
Patch generated with git format-patch
Comment 6 mrmacete 2017-02-21 14:40:13 UTC
Created attachment 346364 [details] [review]
Patch generated with git format-patch
Comment 7 Rico Tzschichholz 2017-02-21 15:24:32 UTC
commit fc8a6ac1de1e162bf817fee704f15e38383b755e
Author: mrmacete <mrmacete@protonmail.ch>
Date:   Tue Feb 21 15:34:43 2017 +0100

    gdbus: Fix memleak using g_task_propagate_pointer