|
Description
Simon McVittie
2011-01-14 09:53:49 UTC
Created attachment 42042 [details] [review] [1/7] dbus_bus_set_unique_name, dbus_bus_get_unique_name: remember to unlock on OOM On some runs I also got an assertion failure (deadlock detected) in the fake lock implementation, which I tracked down to this. Created attachment 42043 [details] [review] [2/2] bus-test: add support for only running one test This is much quicker when valgrinding. Created attachment 42044 [details] [review] [2/7 update_desktop_file_entry: use _dbus_strdup for something we'll dbus_free (not thoroughly tested) Created attachment 42045 [details] [review] update_desktop_file_entry: free exec_tmp in the successful case (not thoroughly tested) Created attachment 42046 [details] [review] [1/2] bus/test: assert that the client isn't listed, when appropriate (Not thoroughly tested yet) Created attachment 42168 [details] [review] [3/7] update_desktop_file_entry: make scope of exec_tmp as short as possible Attachment #42042 [details] is now patch 1/7 of my leak-fixes branch. Attachment #42044 [details] is now patch 2/7; this one is to be applied after it. Created attachment 42169 [details] [review] [4/7] update_desktop_file_entry: free @exec on error Created attachment 42170 [details] [review] [5/7] update_desktop_file_entry: don't double-free strings if added to entry before failure Created attachment 42171 [details] [review] [6/7] update_desktop_file_entry: unify cleanup code for success and failure cases Created attachment 42172 [details] [review] [7/7] _dbus_transport_get_is_authenticated: don't leak if copying GUID fails This would fail the "SHA1 connection test" if _dbus_iterate is modified to allocate and free one extra pointer per iteration. This took me *days* to track down :-/ I'm not sure whether the transport needs disconnecting here too, but this version at least doesn't leak. Comment on attachment 42046 [details] [review] [1/2] bus/test: assert that the client isn't listed, when appropriate Attachment #42046 [details] and Attachment #42043 [details] are not strictly necessary, so I've put them on another branch, 'bus-testing'. I think the patches marked 1/7 to 7/7 are unintrusive enough for 1.4. 1/2 and 2/2 are optional for 1.4, but I'd like them in 1.5. Comment on attachment 42042 [details] [review] [1/7] dbus_bus_set_unique_name, dbus_bus_get_unique_name: remember to unlock on OOM >+ goto finally; There are a lot of hits in the current dbus code for "out:", and none for "finally:". Other than that style nit, looks fine. Comment on attachment 42043 [details] [review] [2/2] bus-test: add support for only running one test Looks fine. Comment on attachment 42044 [details] [review] [2/7 update_desktop_file_entry: use _dbus_strdup for something we'll dbus_free Looks obviously correct. Comment on attachment 42170 [details] [review] [5/7] update_desktop_file_entry: don't double-free strings if added to entry before failure >+ /* ownership has been transferred to entry, do not free separately */ >+ name = NULL; >+ exec = NULL; >+ user = NULL; >+ systemd_service = NULL; style: I think this might look cleaner if it was alternating; i.e.: entry->name = name; name = NULL; entry->exec = exec; exec = NULL; Comment on attachment 42172 [details] [review] [7/7] _dbus_transport_get_is_authenticated: don't leak if copying GUID fails >From 1b170a4385eec374386e4e3b273b72b9ee0b773c Mon Sep 17 00:00:00 2001 >From: Simon McVittie <simon.mcvittie@collabora.co.uk> >Date: Tue, 18 Jan 2011 15:32:36 +0000 >Subject: [PATCH 7/7] _dbus_transport_get_is_authenticated: don't leak if copying GUID fails > >This would fail the "SHA1 connection test" if _dbus_iterate is modified >to allocate and free one extra pointer per iteration. We clearly need the _unref. As for backing out...well, grep for the callers. "check_write_watch" looks like it may not be ready to handle the connection being closed here. I think the cleanest thing is actually to change the API to be: dbus_bool_t _dbus_transport_get_is_authenticated (DBusTransport *transport dbus_bool_t *is_authenticated_out); I.e. the is authenticated boolean is a separate out parameter from the standard error return bool. But then e.g. check_read_watch isn't itself set up for error handling =/ We may need to suck it up and do _dbus_wait_for_memory() with a FIXME. (In reply to comment #13) > There are a lot of hits in the current dbus code for "out:", and none for > "finally:". Edited, and pushed to dbus-1.4 and master. (In reply to comment #14) > (From update of attachment 42043 [details] [review]) > Looks fine. Also in 1.4 and master. (In reply to comment #15) > (From update of attachment 42044 [details] [review]) > Looks obviously correct. Can't be merged without the other changes to update_desktop_file_entry: as it stands at the moment, it's leaked on some error paths (so the tests fail). This was masked by it being a plain strdup(), which isn't covered by our leak checks! Created attachment 42807 [details] [review] [7/7 v2] _dbus_transport_get_is_authenticated: don't leak if copying GUID fails Tracing through the code, I believe my first version of 7/7 was right to return FALSE without disconnecting the transport. The distinction between failure to copy, and a GUID mismatch, is that failure to copy is a temporary failure (if someone calls _dbus_transport_get_is_authenticated again, we might succeed), whereas a GUID mismatch is a permanent failure. This version of the patch hopefully clarifies why it's right, by falling through to the same code that'd run if the DBusAuth hadn't finished processing. (The name of this method is bizarre - it should be _dbus_transport_try_to_authenticate or something - but that's not a job for a minimal bugfix branch.) I'll also attach an alternative patch which solves it differently. Created attachment 42808 [details] [review] [7/7 alternative] DBusTransport: don't copy DBusAuth's GUID to expected_guid Rather than adding OOM handling everywhere, if we avoid strdup'ing the GUID, and just re-fetch the const string from the DBusAuth object on demand instead, we go back to a situation where _dbus_transport_get_is_authenticated can't fail. (Please say which solution you prefer; they both seem to work.) Created attachment 42809 [details] [review] update_desktop_file_entry: stylistic fixes based on Colin's review Any chance someone could look at this again? I'd like to get it into 1.4.4, and the stuff in update_desktop_file_entry is a real-world memory leak (I've seen it when valgrinding dbus-daemon). (In reply to comment #20) > Created an attachment (id=42808) [details] > [7/7 alternative] DBusTransport: don't copy DBusAuth's GUID to expected_guid > > Rather than adding OOM handling everywhere, if we avoid strdup'ing the > GUID, and just re-fetch the const string from the DBusAuth object on demand > instead, we go back to a situation where _dbus_transport_get_is_authenticated > can't fail. > > (Please say which solution you prefer; they both seem to work.) Re-using the const string we already have around seems a lot nicer to me. (In reply to comment #21) > Created an attachment (id=42809) [details] > update_desktop_file_entry: stylistic fixes based on Colin's review Looks OK, but I personally prefer rebased commits; for us, Bugzilla has all the history. Merged for 1.4.4/1.5.0, thanks. |
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.