altdesktop / python-dbus-next

šŸšŒ The next great DBus library for Python with asyncio support
https://python-dbus-next.readthedocs.io/en/latest/
MIT License
187 stars 58 forks source link

GLib.Source has awkward behaviour, leading to crashes in dbus-next #113

Closed whot closed 1 year ago

whot commented 2 years ago

Upstream bug is here: https://gitlab.gnome.org/GNOME/pygobject/-/issues/525

The summary is: for a custom source that returns GLib.SOURCE_REMOVE we need to explicitly call source.destroy() (and not source.__del__). This doesn't seem to have an effect on a "normal" program but I have a pytest test suite that ends up calling this code multiple times and semi-reliably triggers a segfault this way.

The simplest dbus-next reproducer is this:

from dbus_next.glib import MessageBus
from dbus_next import BusType

from gi.repository import GLib
import sys

bus = MessageBus(bus_type=BusType.SESSION).connect_sync()
loop = GLib.MainLoop()

def timeout():                                                                                                                
    loop.quit()

GLib.timeout_add(2000, timeout)    
loop.run()

Which yields these errors:

$ valgrind --tool=memcheck /usr/bin/python3 test.py
...
==75874== Invalid read of size 8
==75874==    at 0x49FC57E: PyObject_GetAttrString (in /usr/lib64/libpython3.10.so.1.0)
==75874==    by 0x15673851: ??? (in /usr/lib64/python3.10/site-packages/gi/_gi.cpython-310-x86_64-linux-gnu.so)
==75874==    by 0x1570A506: g_source_unref_internal (gmain.c:2334)

Seems to be that this diff here fixes the issue:

diff --git dbus_next/glib/message_bus.py dbus_next/glib/message_bus.py
index 668e9ec..b1190be 100644
--- dbus_next/glib/message_bus.py
+++ dbus_next/glib/message_bus.py
@@ -457,7 +457,7 @@ class MessageBus(BaseMessageBus):
                 self._stream.write(Authenticator._format_line(resp))
                 self._stream.flush()
                 if resp == 'BEGIN':
-                    self._readline_source = None
+                    self._readline_source.destroy()
                     authenticate_notify(None)
                     return True
             except Exception as e: