Agreed.

But couldn't we call dbus_shutdown from an __attribute__((destructor)) function?

oh noes. Please don't! 

The order in which destructors are called is not defined (although very recent gcc versions have a priority arg for that now, but the numbers are not really established). That means if an app installs a destructor and libdbus does too and the app wants to shut down some dbus stuff in the destructor, and happens to be called after the libdbus destructor things go BOOM.

And things are even worse if you think about threads. While those destructors are executed all other threads stay around as normal. That means that if you have a thread that dispatches dbus calls while in another thread the destructor is called because we want to exit then things go MEGABOOM.

In short, it must be up to the application programmer to call dbus_shutdown() at the right place, when he can specify the order and can make sure all other threads are already shut down. Everything else is disaster.

Especially since dbus_shutdown() is mostly just cosmetics to make things valgrind clean.

I don't think your reasoning is entirely correct.

We'd do:
1) make dbus_shutdown() be empty and do nothing, so if any app or library is calling it, it will be a no-op

2) move its code to a private destructor function

Yes, the order in which destructor functions are run is undefined, within the same library. However, across libraries, the order is very clear: all destructors from one library are run before the next. So the library loading/unloading order dictates what happens first.

Also, by definition, there are no threads using the library when it's being unloaded. By the library load/unload order, anything that links to the library has already been destroyed (and possibly unloaded) by the time libdbus-1's destructors are run.

And note that any runtime-registered destructor functions (atexit, destructors in C++ local static variables) will be run before any library destructors.

destructors are not only run when a library is unloaded but also when a process exits (which is the more common case). And when a process exits the destructors are executed but the libraries not unloaded.

No. When the process exits, the libraries are cleanly unloaded in the reverse order of loading.

A C++ program to illustrate (pasting only relevant information):
$ LD_DEBUG=files bin/qdbusxml2cpp -h                              
      2860:     file=libQtDBus.so.4 [0];  needed by bin/qdbusxml2cpp [0]                                  
      2860:     file=libQtXml.so.4 [0];  needed by bin/qdbusxml2cpp [0]                                   
      2860:     file=libQtCore.so.4 [0];  needed by bin/qdbusxml2cpp [0]                                  
      2860:     file=libpthread.so.0 [0];  needed by bin/qdbusxml2cpp [0]                                 
      2860:     file=libstdc++.so.6 [0];  needed by bin/qdbusxml2cpp [0]                                  
      2860:     file=libm.so.6 [0];  needed by bin/qdbusxml2cpp [0]                                       
      2860:     file=libgcc_s.so.1 [0];  needed by bin/qdbusxml2cpp [0]                                   
      2860:     file=libc.so.6 [0];  needed by bin/qdbusxml2cpp [0]                                       
      2860:     file=libz.so.1 [0];  needed by /home/tmacieir/obj/troll/qt-4.6/lib/libQtCore.so.4 [0]     
      2860:     file=libdl.so.2 [0];  needed by /home/tmacieir/obj/troll/qt-4.6/lib/libQtCore.so.4 [0]    
      2860:     file=libgthread-2.0.so.0 [0];  needed by /home/tmacieir/obj/troll/qt-4.6/lib/libQtCore.so.4 [0]                                                                                                         
      2860:     file=librt.so.1 [0];  needed by /home/tmacieir/obj/troll/qt-4.6/lib/libQtCore.so.4 [0]      
      2860:     file=libglib-2.0.so.0 [0];  needed by /home/tmacieir/obj/troll/qt-4.6/lib/libQtCore.so.4 [0]
      2860:     file=libpcre.so.0 [0];  needed by /usr/lib/libglib-2.0.so.0 [0]                             

      2860:     calling init: /lib/i686/libpthread.so.0                                                     
      2860:     calling init: /lib/i686/libc.so.6                                                           
      2860:     calling init: /lib/libpcre.so.0                                                             
      2860:     calling init: /usr/lib/libglib-2.0.so.0                                                     
      2860:     calling init: /lib/libdl.so.2                                                               
      2860:     calling init: /lib/libz.so.1                                                                
      2860:     calling init: /lib/libgcc_s.so.1                                                            
      2860:     calling init: /lib/i686/libm.so.6                                                           
      2860:     calling init: /usr/lib/libstdc++.so.6                                                       
      2860:     calling init: /lib/i686/librt.so.1                                                          
      2860:     calling init: /usr/lib/libgthread-2.0.so.0                                                  
      2860:     calling init: /home/tmacieir/obj/troll/qt-4.6/lib/libQtCore.so.4                            
      2860:     calling init: /home/tmacieir/obj/troll/qt-4.6/lib/libQtXml.so.4                             
      2860:     calling init: /home/tmacieir/obj/troll/qt-4.6/lib/libQtDBus.so.4                            

      2860:     calling fini: bin/qdbusxml2cpp [0]                              
      2860:     calling fini: /home/tmacieir/obj/troll/qt-4.6/lib/libQtDBus.so.4 [0]
      2860:     calling fini: /home/tmacieir/obj/troll/qt-4.6/lib/libQtXml.so.4 [0] 
      2860:     calling fini: /home/tmacieir/obj/troll/qt-4.6/lib/libQtCore.so.4 [0]
      2860:     calling fini: /usr/lib/libgthread-2.0.so.0 [0]
      2860:     calling fini: /lib/i686/librt.so.1 [0]
      2860:     calling fini: /lib/i686/libpthread.so.0 [0]
      2860:     calling fini: /usr/lib/libstdc++.so.6 [0]
      2860:     calling fini: /lib/i686/libm.so.6 [0]
      2860:     calling fini: /lib/libgcc_s.so.1 [0]
      2860:     calling fini: /lib/libz.so.1 [0]
      2860:     calling fini: /lib/libdl.so.2 [0]
      2860:     calling fini: /usr/lib/libglib-2.0.so.0 [0]
      2860:     calling fini: /lib/libpcre.so.0 [0]
      2860:     calling fini: /usr/lib/gconv/UTF-16.so [0]
      2860:     calling fini: /lib/i686/libc.so.6 [0]

Note how the order is well-defined.

See _dl_sort_fini in elf/dl-fini.c in glibc.

Don't dlopen libdbus?  I'd like to WONTFIX this one.

Ok for WONTFIX.

Unloading has always been problematic for most libraries.

Uh, it's not that easy.

It's not that anybody would ever dlopen() libdbus directly. It's that it might get pulled in indirectly, because some other module uses it. Just think of libpam_ck_connector which is temporarily loaded when a PAM operation is done. This module will do a couple of D-Bus calls inside an app that might not use D-Bus itself (for example, because it is called written in the 80's like most PAM clients) and then be unloaded. And each time that happens a new set of data is leaked.

So, I would definitely say that this should NOT be closed WONTFIX.

Adding -z nodelete is not particularly hard, is it?

Using destructor functions isn't hard either.

(In reply to comment #9)
> Using destructor functions isn't hard either.

As mentioned this means big failure in threaded code. 

Also, what you mentioned in #5 boils down to that the destruction order is still arbitrary: to fix the problem properly we'd have to make sure that D-Bus is always the last library to be unloaded or unloaded before and after certain other libraries. However, if the destruction order is the reverse of the load order then on systems which uses loadable modules triggered by user input (i.e. due to NSS, yadda yadda) the order is practically random.

So, really, destructors are not a solution, they add two new problems.

-z nodelete is the way to go.

(In reply to comment #8)
> It's not that anybody would ever dlopen() libdbus directly. It's that it might
> get pulled in indirectly, because some other module uses it. Just think of
> libpam_ck_connector which is temporarily loaded when a PAM operation is done.

Would linking libpam_ck_connector with -z nodelete fix this?

I can't help thinking that instead of applying special runes to every library that has globals and happens to have been used by a plugin, it seems more appropriate to apply the special runes to the plugin itself; plugins are already special in various other ways.

(In reply to comment #11)
> Would linking libpam_ck_connector with -z nodelete fix this?

Anyone?

My opinion has not changed in the last 3.5 years. I still think libdbus-1 should shut itself down automatically in a library fini-style function.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/23.