gamemode icon indicating copy to clipboard operation
gamemode copied to clipboard

Should check return value of `dbus_connection_send_with_reply` in `lib/client_impl.c`

Open gicmo opened this issue 4 years ago • 2 comments

In make_request we call dbus_connection_send_with_reply but don't actually check the if the call succeeded: https://github.com/FeralInteractive/gamemode/blob/4000a32584a9eb3964813b8fdbb279be7beb24cc/lib/client_impl.c#L259

This could result in call being NULL and then as a result dbus_pending_call_block trigger an abort via the check _dbus_return_if_fail (pending != NULL);.

gicmo avatar Jan 04 '22 11:01 gicmo

According to the documentation call can also be NULL "if the connection is disconnected or you try to send Unix file descriptors on a connection that does not support them, the DBusPendingCall will be set to NULL, so be careful with this." So we need to check call independently of the return value, it seems.

gicmo avatar Jan 04 '22 11:01 gicmo

This could result in call being NULL and then as a result dbus_pending_call_block trigger an abort via the check _dbus_return_if_fail (pending != NULL);.

This is not just a theoretical concern, it does genuinely happen.

One symptom is that gamemoderun steam mis-detects that creating a container is not possible, and pops up a message "Steam on Linux now requires the ability to create new user namespaces" (#467, https://github.com/ValveSoftware/steam-runtime/issues/658). A crude workaround for this is to delete ~/.steam/root/ubuntu12_32/steam-runtime/*/usr/bin/steam-runtime-check-requirements.

After that is worked around, Steam starts successfully, but logs these warnings repeatedly:

dbus[14985]: arguments to dbus_pending_call_block() were incorrect, assertion "pending != NULL" failed in file ../../../dbus/dbus-pending-call.c line 765.
This is normally a bug in some application using the D-Bus library.

This appears to be the standard lsof(8) tool crashing, in code injected by gamemode's LD_PRELOAD module that runs during exit(). Backtrace:

#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=127824908428352) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=127824908428352) at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=127824908428352, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x000074418e242476 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x000074418e2287f3 in __GI_abort () at ./stdlib/abort.c:79
#5  0x000074418e008ed6 in ?? () from /lib/x86_64-linux-gnu/libdbus-1.so.3
#6  0x000074418e02f690 in _dbus_warn_check_failed () from /lib/x86_64-linux-gnu/libdbus-1.so.3
#7  0x000074418e04dd21 in make_request (error=0x7fff7ff63000, npids=<optimized out>, pids=0x7fff7ff63088, 
    method=0x74418e04f128 "UnregisterGame", use_pidfds=<optimized out>, native=<optimized out>, bus=0x5e3fd65acf30)
    at ../lib/client_impl.c:264
#8  gamemode_request (method=0x74418e04f128 "UnregisterGame", for_pid=0) at ../lib/client_impl.c:335
#9  0x000074418e4f2165 in gamemode_request_end () at ../lib/gamemode_client.h:283
#10 0x000074418e4ff24e in _dl_fini () at ./elf/dl-fini.c:142
#11 0x000074418e245495 in __run_exit_handlers (status=status@entry=0, listp=0x74418e41a838 <__exit_funcs>, 
    run_list_atexit=run_list_atexit@entry=true, run_dtors=run_dtors@entry=true) at ./stdlib/exit.c:113
#12 0x000074418e245610 in __GI_exit (status=status@entry=0) at ./stdlib/exit.c:143
#13 0x00005e3fd6092576 in Exit (xv=0) at /build/lsof-uBiEgR/lsof-4.93.2+dfsg/misc.c:784
#14 0x00005e3fd609eb9b in main (argc=<optimized out>, argv=<optimized out>) at /build/lsof-uBiEgR/lsof-4.93.2+dfsg/main.c:1761

It would be much safer if gamemode did not do anything with D-Bus during process shutdown. Instead, if it doesn't do so already (maybe it already does?), the daemon could track the D-Bus unique name (which looks something like :1.42) of every process that has registered with it, and when it sees a NameOwnerChanged signal indicating that the name has disconnected from the bus, treat that as though the process had unregistered.

smcv avatar Mar 21 '24 16:03 smcv