| Summary: | invoke_object_method leaks, reports OOM if an "out" arg can't be marshalled | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Simon McVittie <smcv> |
| Component: | GLib | Assignee: | Simon McVittie <smcv> |
| Status: | RESOLVED FIXED | QA Contact: | John (J5) Palmieri <johnp> |
| Severity: | normal | ||
| Priority: | medium | CC: | cosimo.alfarano, emezeske, rob.taylor, smcv, walters, will |
| Version: | unspecified | Keywords: | patch |
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Bug Depends on: | 30171 | ||
| Bug Blocks: | 35766 | ||
| Attachments: |
dbus-binding-tool: forbid ReturnVal annotation after the first OUT <arg>
arg_iterate: document invoke_object_method: if dbus_message_new_method_return fails, fail hard gerror_to_dbus_error_message: guarantee to return non-NULL invoke_object_method, dbus_g_method_return_error: handle sending failure invoke_object_method: if marshalling an out argument fails, discard message dbus-gobject: centralize death-by-OOM and use oom() instead of goto [PATCH 2/2] invoke_object_method: simplify how we return |
||
|
Description
Simon McVittie
2011-03-29 06:31:30 UTC
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.