Created attachment 16126 [details] [review]
Patch to correct the problem

do the connection actually go away?  I guess I see no harm.  Can you send the patch in this format please:

http://lists.freedesktop.org/archives/dbus/2008-April/009642.html

Thanks

A Disconnected signal handler is the perfect place to unref a connection, no?  If you do that, in most cases, the last reference will be gone by the time that call finishes -- so it'll access freed memory.

I'm afraid I don't know how to drive git to get the patch format you want?

I can step you through it.  How are you hacking on D-Bus right now?  I'm going to assume you are using the tarball or distro package.  Here is how you do it otherwise:

# the reason for the 2 dbus directories is we also have dbus/python, dbus/glib etc.
git clone git://git.freedesktop.org/git/dbus/dbus
cd dbus

apply your patch

# you only need to do the git-config calls once for each machine you use
git-config --global user.name "Scott James Remnant"
git-config --global user.email "scott@netsplit.com"

# commit the patch -a means commit all changes or you can use git-add to add the 
# files you wish to commit and then just do a git commit.  The nice thing
# about git-add is it will only commit the changes when the git-add was called
# you you can add your changes, code some debug printf's and then commit without # the printf's being committed.
git-commit -a

# git-commit will bring up the ChangeLog screen just like cvs or svn.  Please
# follow the commit format in the message I pointed to

# repeat the above steps for every patch you have that has been approved

# this will output all commit you have made that are not upstream
git-format-patch origin

Attach those patches to the bugs.

Created attachment 16158 [details] [review]
Patch in preferred format

Ok, patch attached.

Thanks.  The format for the ChangeLog is a bit off but I can make those changes for now.  Just make sure to follow this format in the future (notice the < 80 character summary):

Hold a local reference to connection object while we are using it

* dbus/dbus-connection.c (_dbus_connection_read_write_dispatch):
  Reference the D-Bus connection during the function call since we
  call other functions that may free the last reference and we
  still expect to be able to check the connection after they return
  to decide our own return value.

Hadn't realised there was going to be a first-line summary ;-)  That's not usual in ChangeLog entries <g>

This patch should be safe, but I'm not entirely sure that the relevant user code shouldn't be holding onto a ref somehow outside of the dispatch, and then be unreffing it later.

For example in the test-privserver.c code, we just _dbus_loop_quit (loop), and then after the mainloop has exited we unref the connection.

Maybe this model wouldn't be right though if you had a number of client connections from a DBusServer.  

Adding it for now, if you can come up with a test case to put in test/name-test/ it would be appreciated.

commit ef41cd31100097636523088ec7f115e432366956
Author: Colin Walters <walters@verbum.org>
Date:   Fri May 30 20:41:49 2008 -0400

    Bug 15635: Hold a reference during read/write dispatch (Scott James Remnant)
    
    	* dbus/dbus-connection.c (_dbus_connection_read_write_dispatch):
     	Reference the D-Bus connection during the function call since we
     	call other functions that may free the last reference and we
     	still expect to be able to check the connection after they return
     	to decide our own return value.

That's pretty much exactly what I have, and also remember that Upstart doesn't exit() on disconnection from the system bus and carries on, so has to deal with disconnection in ways that other services generally don't.

Private connections to the internal server (rare) and the system bus connection itself may be disconnected, and since we're holding on to a reference to them in the list of connections, we have to unref them.  The logical place to do that is in the Disconnected signal handler.

Since that's called by read_write_dispatch, which is called in the main loop, there's no higher place to unreference -- the alternative would be to set a timer so that the connection was unreferenced "later", but that's definitely working around a bug.

Since read_write_dispatch attempts to use the connection after the dispatch function has returned, it should be holding a reference to ensure that it _can_ use it.