+++ This bug was initially created as a clone of Bug #30171 +++ Somewhat similar to Bug #30171, if one of the two calls to _dbus_gvalue_marshal in invoke_object_model fails, we'll return DBUS_HANDLER_RESULT_NEED_MEMORY to libdbus, while leaking the GValues for any subsequent arguments. The most likely cause for one of these calls to fail is programming error (e.g. a return value that can't be serialized, like a non-UTF-8 string).
Created attachment 45220 [details] [review] dbus-binding-tool: forbid ReturnVal annotation after the first OUT <arg> It has never actually worked correctly (invoke_object_method would always treat the ReturnVal as if it had been the first OUT argument), so let's only allow the situation that worked in practice.
Ugh, sorry, wrong bug; that patch is for Bug #35952.
Created attachment 45229 [details] [review] arg_iterate: document
Created attachment 45230 [details] [review] invoke_object_method: if dbus_message_new_method_return fails, fail hard There's no point in doing graceful unwinding here, since it really shouldn't happen.
Created attachment 45231 [details] [review] gerror_to_dbus_error_message: guarantee to return non-NULL
Created attachment 45232 [details] [review] invoke_object_method, dbus_g_method_return_error: handle sending failure I just made it fatal, since it's either programming error or OOM.
Created attachment 45233 [details] [review] invoke_object_method: if marshalling an out argument fails, discard message We shouldn't report this as OOM, because it probably wasn't (it's much more likely to be programming error); we should also continue through the arguments, so that we don't leak them. By abandoning the message as soon as we detect a programming error, we can use reply == NULL as an indicator of whether to keep appending.
Review of attachment 45232 [details] [review]: OK, pre-existing maybe-leak (review it please) apart. ::: dbus/dbus-gobject.c @@ +1875,2 @@ dbus_message_unref (reply); } that's OK, but since I just spotted what I think it's a leak and it's there: @reply leaks if the flow goes to "nomem" (and thus to "done"). My fix is split the connection_send() before "done" and the unref() within done. If it makes sense for you, please add it directly to your patch :)
Review of attachment 45230 [details] [review]: OK. Does not change the substance of the fix, but since I see the "OOM?" situation can happen quite a lot (as opposite to the certainty of a OOM), why not adding a maybe_oom(char *cause)? There are other places in this module that after a dbus_message_new_* actually return "out of memory" (as opposite to "out of memory?" or to oom()). Even if not related to this bug, it would be a good moment to replace them all with the right info. What do you think?
Review of attachment 45231 [details] [review]: OK, prev comment applies.
Review of attachment 45233 [details] [review]: OK for me ::: dbus/dbus-gobject.c @@ +1816,3 @@ + dbus_message_unref (reply); + reply = NULL; + G_VALUE_TYPE_NAME (&return_value), Oh, I see you already fixed the leak in another way, better so :) @@ +1905,2 @@ result = DBUS_HANDLER_RESULT_HANDLED; + Why not remove @result and return the value directly?
Not having any reviewer hat, I'm not updating the whiteboard with review+, but beside the comments which are up to Simon, it's a virtual-review+
I'm going to merge these as Reviewed-by: you, on the basis that none of the reviewers have vetoed them in the 4½ months they've been available. (In reply to comment #11) > Why not remove @result and return the value directly? Hysterical raisins; I assume there used to be a "finally" label just after the assignment to result, or something. I owe you a cleanup patch for that before closing this bug. (In reply to comment #9) > why not adding a maybe_oom(char *cause)? Fair enough, I'll look at that as a subsequent cleanup patch, if still relevant. > There are other places in this module that after a dbus_message_new_* actually > return "out of memory" (as opposite to "out of memory?" or to oom()). As you said on Bug #35766, these are fixed over there.
Created attachment 50325 [details] [review] dbus-gobject: centralize death-by-OOM and use oom() instead of goto You can't recover from OOM (in GLib-land) so there's no point in complicating the code for the reader. --- I've applied all of the older patches.
Created attachment 50328 [details] [review] [PATCH 2/2] invoke_object_method: simplify how we return Based on review feedback from Cosimo. --- This was correct, but more complicated than it needed to be; thanks for spotting it!
Both remaining patches Attachment #50325 [details] abd Attachment #50328 [details] are fine for me, thanks for the fixes.
These were applied in 0.98.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.