+++ This bug was initially created as a clone of Bug #30171 +++ Similar to Bug #30171, if the call to _dbus_gvalue_marshal in get_all_object_properties (dbus-glib's implementation of GetAll) fails, we'll g_error ("out of memory"), even if the real cause is programming error (e.g. a property value that can't be serialized, like a non-UTF-8 string). The same theoretically applies to the dbus_message_iter_close_container calls, which should probably abandon_container during any error unwinding. I think it's OK if GetAll just doesn't reply at all on such programming errors, but we could consider raising Failed so the caller knows something is wrong.
Created attachment 45284 [details] [review] [PATCH 01/15] dbus-gobject: factor out reply_or_die, error_or_die and add error-checking
Created attachment 45285 [details] [review] [PATCH 02/15] dbus-gobject: check for NULL when producing messages
Created attachment 45286 [details] [review] [PATCH 03/15] check_property_access: centralize error handling (and check for OOM)
Created attachment 45287 [details] [review] [PATCH 04/15] get_object_property: unwind on errors, and avoid returning NULL Also treat all errors here as programming errors (because this method should never fail), upgrading them from warning to critical; return a D-Bus error reply anyway, to be nicer to our callers.
Created attachment 45288 [details] [review] [PATCH 05/15] get_object_property: add brief documentation
Created attachment 45289 [details] [review] [PATCH 06/15] object_registration_message: handle remote caller errors better If the remote process sends us a wrong message, that's its fault, not ours; we should send back a comprehensible D-Bus error, and not spam to stderr. Previously, in each of these cases libdbus would have sent back NoReply, because we declined to handle the method call. One case that's still wrong is passing extra arguments to Get, GetAll or Set, like so: Get("com.example.Iface", "MyProperty", "extra") Set("com.example.Iface", "MyProperty", Variant("foo"), "extra") GetAll("com.example.Iface", "extra") dbus-glib has historically warned, ignored the extra argument(s) and sent back a reply as if they hadn't been there, whereas a stricter implementation (like telepathy-glib's TpDBusPropertiesMixin) would have sent back an error reply and done nothing.
Created attachment 45290 [details] [review] [PATCH 07/15] object_registration_message: make logic/assertions slightly clearer
Created attachment 45291 [details] [review] [PATCH 08/15] object_registration_message: check for OOM when a property is absent
Created attachment 45292 [details] [review] [PATCH 09/15] export_signals: check interface, signal names for validity
Created attachment 45293 [details] [review] [PATCH 10/15] emit_signal_for_registration: assert that signal headers are all valid This is an assertion, not a return_if_fail, because we already checked the object path in dbus_g_connection_register_g_object and the others in export_signals. After these checks, dbus_message_new_signal can really only fail on OOM.
Created attachment 45294 [details] [review] [PATCH 11/15] dbus-gobject: add and use connection_send_or_die to check for OOM
Created attachment 45295 [details] [review] [PATCH 12/15] get_all_object_properties: skip invalidly-named properties, with a critical
Created attachment 45296 [details] [review] [PATCH 13/15] get_all_object_properties: note which operations can only fail via OOM
Created attachment 45297 [details] [review] [PATCH 14/15] get_all_object_properties: if _dbus_gvalue_marshal fails, bail out This isn't necessarily OOM: _dbus_gvalue_marshal can fail due to programming errors. If so, raise a critical warning, then (if that wasn't fatal) return a D-Bus error to be sent to the caller.
Created attachment 45298 [details] [review] [PATCH 15/15] test-dbus-glib: make lose() abort, and make lose_gerror() more informative Not really part of this bug, but I spotted it while fixing this and it's trivial.
Review of attachment 45284 [details] [review]: Good for me, also you already went through all the occurrences of dbus_message_new_* to handle failures consistently, which makes my comment on Bug #35767 pointless now :) It just keeps out _new_signal() which is used just once.
OK, went through patches 1-15 and they look good, not a reviewer though, so just a virtual-review+
(In reply to comment #16) > It just keeps out _new_signal() which is used just once. Patch 10/15 checks all arguments to _new_signal(), so there's no way it can fail except genuine OOM. I think. If you think I'm wrong, we can fix that too. As with Bug #35767, I'm going to merge these as Reviewed-by: you, because none of the approved reviewers have had anything bad to say about them in 4½ months :-)
Fixed in git for 0.96. (In reply to comment #18) > Patch 10/15 checks all arguments to _new_signal(), so there's no way it can > fail except genuine OOM. I think. If you think I'm wrong, we can fix that too. Please reopen or something if you think this deserves a comment (or is untrue).
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.