GNOME Bugzilla – Bug 778540
fd passing on dbus messages may trigger glib warnings
Last modified: 2018-03-21 16:16:10 UTC
In tracker there's this test that basically attempts a DOS over tracker-store: https://git.gnome.org/browse/tracker/tree/tests/libtracker-sparql/tracker-gb-737023.c It does trigger many asynchronous queries that require fd passing, reaching to a point where tracker-store has more queries open than fds available. The test was introduced as this behavior used to crash tracker-store, but even if this was fixed, the running tracker-store still triggers a bunch of: (tracker-store:14796): GLib-GIO-CRITICAL **: g_unix_fd_list_get: assertion 'G_IS_UNIX_FD_LIST (list)' failed (tracker-store:14796): GLib-GIO-CRITICAL **: g_unix_output_stream_new: assertion 'fd != -1' failed This happens in the vala dbus wrappers, and tracker-store can do nothing to prevent those. I'm attaching some patches adding proper error handling on the dbus wrapper paths involved in fd passing.
Created attachment 345589 [details] [review] gdbus: Initialize intermediate variables Those may be left uninitialized after error handling paths are introduced, so invalid data might be attempted to be freed later on.
Created attachment 345590 [details] [review] gdbus: Ensure extracted data from the GVariant is freed on error This will make extracted variables properly freed when extracting those fails at some point.
Created attachment 345591 [details] [review] gdbus: Handle errors when extracting fds from dbus messages On the right situations (eg. fd exhaustion), both g_dbus_message_get_unix_fd_list() and g_unix_fd_list_get() should be considered failable here. Add proper error handling to avoid triggering glib warnings.
Tracker fails to build with these patches http://paldo.org:8010/builders/vala-staging/builds/199/steps/tracker/logs/stdio
Created attachment 345607 [details] [review] gdbus: Handle errors when extracting fds from dbus messages On the right situations (eg. fd exhaustion), both g_dbus_message_get_unix_fd_list() and g_unix_fd_list_get() should be considered failable here. Add proper error handling to avoid triggering glib warnings.
I apparently didn't delete the vala timestamp files before rebuilding tracker after my last cleanup. This last patch fixes the build error.
Thanks! Could you add a small test-case as well? At least to test the changed code-paths.
Created attachment 346130 [details] [review] tests: Add test for dbus fd passing failures The server will exhaust all fds before the fd list in the dbus request is opened. We do expect it to fail in the client.
Created attachment 346131 [details] [review] dbus: Account for _reply_message being NULL in error paths Commit 09e6818d01 changed dbus server paths so requests fell back to freeing allocated memory on errors. However certain kinds of dbus replies issue a final g_dbus_connection_send_message() there. In case of errors, _reply_message will be NULL, and the error paths would have already issued g_dbus_method_invocation_return_gerror(), so the g_dbus_connection_send_message() call is both unnecessary and warns on the NULL argument. So add a NULL check for these cases.
Created attachment 346135 [details] [review] dbus: Move send_message() call before _error label Commit 09e6818d01 changed dbus server paths so requests fell back to freeing allocated memory on errors. However certain kinds of dbus replies issue a final g_dbus_connection_send_message() there. In case of errors, _reply_message will be NULL, and the error paths would have already issued g_dbus_method_invocation_return_gerror(), so the g_dbus_connection_send_message() call is both unnecessary and warns on the NULL argument. So move the DBus reply before the _error label. --- Alternative fix (and IMHO simpler) to attachment 346131 [details] [review].
Created attachment 347303 [details] [review] tests: Add tests around DBus server argument handling Commit 09e6818d01e introduced the possibility of inserting _error labels at the end of functions, which make C compilers angry. This "test" (no actual tests are run, just compile correctness is checked here) triggers some of these situations. the "test3" abstract method is trickier though, it doesn't warn but produces code with early returns, which in practice means possible leaks.
Created attachment 347304 [details] [review] codegen: Only add _error path if there's parameters that require unref Fixes the _error label from being set at the end of a compound statement if there are no values to unref.
Created attachment 347305 [details] [review] codegen: Avoid early return on sync dbus methods returning an error If there are arguments that need freeing, we need to fall through the _error label, if there is any.
Created attachment 347308 [details] [review] tests: Add tests around DBus server argument handling Commit 09e6818d01e introduced the possibility of inserting _error labels at the end of functions, which make C compilers angry. This "test" (no actual tests are run, just compile correctness is checked here) triggers some of these situations. the "test3" abstract method is trickier though, it doesn't warn but produces code with early returns, which in practice means possible leaks.
Hopefully for good now.