Fix another variant of the previous commit, this time specific to the
idle callback of a method call on a subtree object, racing with
unregistration of that subtree.
In this case, the `process_subtree_vtable_message_in_idle_cb()` idle
callback already has a pointer to the right `ExportedSubtree` struct,
but again doesn’t have a strong reference to it.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2400
If `g_dbus_connection_unregister_object()` (or `unregister_subtree()`)
was called from one thread, while an idle callback for a method call (or
a property get or set) was being invoked in another, it was possible for
the two to race after the idle callback had checked that the
object/subtree was registered, but before it had finished dereferencing
all the data related to that object/subtree.
Unregistering the object/subtree would immediately free the data,
leading the idle callback to cause a use-after-free error.
Fix that by giving the idle callback a strong reference to the data from
inside the locked section where it checks whether the object/subtree is
still registered.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #2400
This is needed for an upcoming change which decouples their lifecycle
from their presence in the `map_id_to_ei` and `map_id_to_es` hash
tables.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2400
Move it further up the file, but make no changes to it. This will help
with a subsequent commit.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2400
Clarify that the terms ‘GUID’ and ‘UUID’ are used interchangeably in the
context of D-Bus, and that neither of them are an RFC 4122 UUID.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Update several links to allow the remote to use its configured default
branch name, rather than specifying `master` as the default branch name.
This will help avoid breakage if any of these projects rename their
default branch in the future.
Fix a few of the links where they were hitting redirects or had moved.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2348
Since commit ab285899a6 ('gdbusconnection: Document main context
iteration for unsubscript'), we document when the user is guaranteed
that all resources are gone after g_dbus_connection_signal_unsubscribe().
This is not merely an implementation detail, it's something that the
user needs to be able to rely on. It is good that this is documented.
However, libnm does something different ([1]). It registers to several D-Bus
signals without providing a GDestroyNotify. After unsubscription, it schedules
another idle action with lower priority and uses that to know when
cleanup is complete. I think this is a useful alternative and should
also be guaranteed and documented to work.
Also note that this isn't just some implementation detail that currently
happens to work. GDBusConnection tightly integrates with GMainContext and it
works by scheduling idle sources with G_PRIORITY_DEFAULT priority. It needs to
schedule all events with this same priority, otherwise the ordering is not
preserved. At this point, with GDBusConnection working this way, this is no longer
something that can reasonably be any different. It's how GDBusConnection fundamentally
works, and a user must be able to rely on that. As such, this new promise isn't
something that we would want to break in the future.
Thus document it.
[1] a55c10c6cb/src/libnm-client-impl/nm-client.c (L7918)
This eliminates a common use case for the
`GDBusAuthObserver::authorize-authenticated-peer` signal, which is often
implemented incorrectly by people.
Suggested by Simon McVittie.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #1804
The GDBusConnectionFlags and GDBusServerFlags can affect how we carry
out authentication and authorization, either making it more or less
restrictive, so it's desirable to "fail closed" if a program is compiled
against a new version of GLib but run against an old version.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Convert all the call sites which use `g_memdup()`’s length argument
trivially (for example, by passing a `sizeof()`), so that they use
`g_memdup2()` instead.
In almost all of these cases the use of `g_memdup()` would not have
caused problems, but it will soon be deprecated, so best port away from
it.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
gio/gdbusconnection.c: In function ‘g_dbus_connection_register_object_with_closures’:
gio/gdbusconnection.c:5527:5: error: missing initializer for field ‘padding’ of ‘GDBusInterfaceVTable’ {aka ‘struct _GDBusInterfaceVTable’}
5527 | };
| ^
This commit only looks at the `Returns:` lines in the documentation, and
has examined all of them in the file. Function arguments have not been
checked.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2227
This should introduce no API changes; there are public functions
exported by `GDBusConnection` which still have some (incorrectly)
`volatile` arguments, but dropping those qualifiers would be an API
break.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #600
In the D-Bus wire protocol, the handle type (G_VARIANT_TYPE_HANDLE, h)
is intended to be an index/pointer into the implementation's closest
equivalent of GUnixFDList: its numeric value has no semantic meaning
(in the same way that the numeric values of pointers have no semantic
meaning), but a handle with value n acts as a reference to the nth fd
in the fd list.
GDBus provides a fairly direct mapping from the wire protocol to the
C API, which makes it technically possible to attach and use fds
without ever referring to them in the message body, and some
GLib-centric D-Bus APIs rely on this.
However, the other major implementations of D-Bus (libdbus and sd-bus)
transparently replace file descriptors with handles when building
messages, and transparently replace handles with file descriptors when
parsing messages. This means they cannot implement D-Bus APIs that do
not follow the conventional meaning of handles as indexes/pointers into
an equivalent of GUnixFDList.
For interoperability, we should encourage D-Bus API designers to follow
the convention, even though code written against GDBus doesn't strictly
need to do so.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Add a note to the documentation of
`g_dbus_connection_signal_unsubscribe()`, `g_bus_unwatch_name()` and
`g_bus_unown_name()` warning about the need to continue iterating the
caller’s thread-default `GMainContext` until the
unsubscribe/unwatch/unown operation is complete.
See the previous few commits and #1515 for an idea of the insidious bugs
that can be caused by not iterating the `GMainContext` until
everything’s synchronised.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
`CallDestroyNotifyData` never uses that `GMainContext`, and holding a
ref to it could cause reference count cycles if the `GMainContext` is no
longer being iterated.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #1515
This just removes a now-redundant intermediate array. This means that
the `SignalSubscriber` instances are now potentially freed a little
sooner, inside the locked segment, but they are already careful to only
call their `user_data_free_func` in the right thread. So that should not
deadlock.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #978
Instead of storing a copy of the `callback` and `user_data` from a
`SignalSubscriber` in a `SignalInstance` struct (which is the closure
for signal callback data as it’s sent from the D-Bus worker thread to
the thread which originally subscribed to a signal), store a strong
reference to the `SignalSubscriber` struct itself.
This keeps the `SignalSubscriber` alive until the emission is
complete, which ensures that the `user_data` is not freed prematurely.
It also slightly reduces the allocation size of `SignalInstance` (not
that it matters).
This is threadsafe because the fields in `SignalSubscriber` are all
immutable after construction.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #978
Tie the destruction of the `user_data` to the destruction of the
`SignalSubscriber` struct. This is tidier, and ensures that the fields
in `SignalSubscriber` are all immutable after being set, so the
structure can safely be used across threads without locking.
It doesn’t matter which thread we call `call_destroy_notify()` in, since
it always defers calling `user_data_free_func` to the user-provided
`GMainContext`.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #978
The `SignalSubscriber` structs contain the callback and `user_data` of each
subscriber to a signal, along with the `guint id` token held by that
subscriber to identify their subscription. There are one or more
`SignalSubscriber` structs for a given signal match rule, which is
represented as a `SignalData` struct.
Previously, the `SignalSubscriber` structs were stored in a `GArray` in
the `SignalData` struct, to reduce the number of allocations needed
when subscribing to a signal.
However, this means that a `SignalSubscriber` struct cannot have a
lifetime which exceeds the `SignalData` which contains it. In order to
fix the race in #978, one thread needs to be able to unsubscribe from a
signal (destroying the `SignalData` struct) while zero or more other
threads are in the process of calling the callbacks from a previous
emission of that signal (using the callback and `user_data` from zero or
more `SignalSubscriber` structs). Multiple threads could be calling
callbacks because callbacks are invoked in the `GMainContext` which
originally made a subscription, and GDBus supports subscribing to a
signal from multiple threads. In that case, the callbacks are dispatched
to multiple threads.
In order to allow the `SignalSubscriber` structs to outlive the
`SignalData` which contained their old match rule, store them in a
`GPtrArray` in the `SignalData` struct, and refcount them individually.
This commit in itself should make no functional changes to how GDBus
works, but will allow following commits to do so.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #978
With debug enabled, g_dbus_connection_call_done() will throw a
g_warning() if the call failed (on purpose or not) while trying to the
serial of a non-existant reply.
(/builds/GNOME/glib/_build/gio/tests/gdbus-connection:26921): GLib-GIO-CRITICAL **: 10:10:16.311: g_dbus_message_get_reply_serial: assertion 'G_IS_DBUS_MESSAGE (message)' failed
When compiling GLib with `-Wsign-conversion`, we get various warnings
about the atomic calls. A lot of these were fixed by
3ad375a629, but some remain. Fix them by
adding appropriate casts at the call sites.
Note that `g_atomic_int_{and,or,xor}()` actually all operate on `guint`s
rather than `gint`s (which is what the rest of the `g_atomic_int_*()`
functions operate on). I can’t find any written reasoning for this, but
assume that it’s because signedness is irrelevant when you’re using an
integer as a bit field. It’s unfortunate that they’re named a
`g_atomic_int_*()` rather than `g_atomic_uint_*()` functions.
Tested by compiling GLib as:
```
CFLAGS=-Wsign-conversion jhbuild make -ac |& grep atomic
```
I’m not going to add `-Wsign-conversion` to the set of default warnings
for building GLib, because it mostly produces false positives throughout
the rest of GLib.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #1565
This commit changes a comment in _g_dbus_worker_do_read_cb() to be
slightly more useful. At least in my experience debugging an
intermittent unit test failure in another project, this failure
condition occurred because although g_test_dbus_down() ensures that the
session GDBusConnection has exit-on-close set to FALSE before killing
its dbus-daemon, there was still a GDBusConnection on the system bus
which hit this failed read code path, because we had
DBUS_SYSTEM_BUS_ADDRESS set to the address of the #GTestDBus daemon, to
appease libudisks.
Also, make a few other minor improvements to the docs.
Using the generic marshaller has drawbacks beyond performance. One such
drawback is that it breaks the stack unwinding from the Linux kernel due
to having unsufficient data to walk past ffi_call_unixt64. That means that
performance profiling by application developers looks grouped among
seemingly unrelated code paths.
While we can't fix the kernel unwinding here, we can provide proper
c_marshallers and va_marshallers for objects within Gio so that
performance profiling of applications is more reliable.
Related to GNOME/Initiatives#10
Right now this can only be set at construction but not read back.
That seems unnecessarily restrictive, and we'll need to read these
flags from outside of gdbusconnection.c in the next commit, so let's
just make it public.
https://gitlab.gnome.org/GNOME/glib/issues/1620
There's a race condition somewhere in GTestDBus that can result in
the next test being started at a time when g_bus_get() would still
return the connection that is in the process of closing. This can
be reproduced reasonably reliably by running the gapplication test
10K times in a loop.
Instead of relying on waiting for the weak reference to be released,
we can force the issue by clearing it.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=768996
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=894677
It incorrectly said that an error could only be returned if the GVariant
was incorrect for the D-Bus API, but that’s not true: an error will also
be returned if you call it on a closed GDBusConnection.
Clarify that, and mention the actual error codes which are returned.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Reviewed-by: nobody
The g_dbus_connection_call() documentation doesn’t make it clear that
the reply type is always a tuple.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Reviewed-by: nobody
Where we were already treating GHashTables as sets, modify them to use
the set-specific APIs g_hash_table_add() and g_hash_table_contains(), to
make that usage more obvious and less prone to being broken.
Heavily based on patches by Garrett Regier <garrettregier@gmail.com>.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
https://bugzilla.gnome.org/show_bug.cgi?id=749371
g_dbus_connection_call_internal() accesses the user data it passes to
g_dbus_connection_send_message_with_reply() after the call. That data
might be freed already in the case that the callback is called
immediately.
Fix this by removing the 'serial' field from the user data altogether
and fetch the serial from the message in the callback.
https://bugzilla.gnome.org/show_bug.cgi?id=748263
Some annotations I made while trying to debug bug #781847. They
introduce no behavioural changes.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Actually, Unicode changes to this file got reverted in
2d56c49b10. Also, there is
"No such interface '%s'" string already, so we avoid
breaking the string freeze.
This highlighted a bug in GDBusConnection, where an interface name was
not included in a message referring to it.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
https://bugzilla.gnome.org/show_bug.cgi?id=780032
If we have an input parameter (or return value) we need to use (nullable).
However, if it is an (inout) or (out) parameter, (optional) is sufficient.
It looks like (nullable) could be used for everything according to the
Annotation documentation, but (optional) is more specific.
This commit broke some tests, and I don't have the time
to fix up all the expected output, so I'll revert the changes
to the affected files for now.
This needs to be redone with the necessary test fixes.
If you called g_dbus_connection_remove_filter() on a filter while it
was running (or about to be run) in another thread, its GDestroyNotify
would be run immediately, potentially causing the filter thread to
crash.
Fix this by refcounting the filters, and using the existing mechanism
for running a GDestroyNotify in another thread in the case where the
the gdbus thread is the one that frees it.
Also, add a bit of documentation explaining this (and add a related
clarification to g_dbus_connection_signal_subscribe()).
https://bugzilla.gnome.org/show_bug.cgi?id=704568
This is a binding-friendly version of g_dbus_connection_register_object.
Based on a patch by Martin Pitt and the code of g_bus_watch_name_with_closures.
https://bugzilla.gnome.org/show_bug.cgi?id=656325
exit-on-close for a DBus connection is a completely normal thing. On
a regular GNOME login, gdm retains the X server, but terminates the
session login bus and associated helpers like gnome-settings-dameon,
the a11y tools, etc.
I've seen several downstream reports of confusion as to what these
apparent error messages mean in the system log. It doesn't help
that they're so obtuse.
We're also printing them to stderr, when this is not an error.
The reason this was introduced is presumably some people were confused
as to why their process exited when the system bus did. But the
solution for that I believe is documentation, not printing stuff to
everyone's system log in normal operation.
https://bugzilla.gnome.org/show_bug.cgi?id=742386
DBus has recently introduced new message flag
DBUS_HEADER_FLAG_ALLOW_INTERACTIVE_AUTHORIZATION, which tells that
caller is willing to wait for unspecified amount of time for the call
to return, as the service may perform interactive authorization (e.g.
using polkit).
https://bugzilla.gnome.org/show_bug.cgi?id=739616
In path_rule_matches(), the given paths may be of 0-length. Do not
access memory before the array in those case. This is for example
triggered by:
test_match_rule (con, G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_PATH, "/", "", FALSE);
in test_connection_signal_match_rules().
This bug was found thanks to GCC AddressSanitizer.
https://bugzilla.gnome.org/show_bug.cgi?id=745745
The gdbus GTask port introduced a deadlock because some code had been
using g_simple_async_result_complete_in_idle() to ensure that the
callback didn't run until after a mutex was unlocked, but in the gtask
version, the callback was being run immediately. Fix it to drop the
mutex before calling g_task_return*(). Also, tweak
tests/gdbus-connection to test this.
https://bugzilla.gnome.org/show_bug.cgi?id=747349
When losing the D-Bus connection, we would write to stdout about it just
before killing ourselves with SIGTERM. We're a library, so we should
probably use stderr instead.
https://bugzilla.gnome.org/show_bug.cgi?id=721324
In Windows development environments that have it, <unistd.h> is mostly
just a wrapper around several other native headers (in particular,
<io.h>, which contains read(), close(), etc, and <process.h>, which
contains getpid()). But given that some Windows dev environments don't
have <unistd.h>, everything that uses those functions on Windows
already needed to include the correct Windows header as well, and so
there is never any point to including <unistd.h> on Windows.
Also, remove some <unistd.h> includes (and a few others) that were
unnecessary even on unix.
https://bugzilla.gnome.org/show_bug.cgi?id=710519
It's possible to get a org.freedesktop.Properties.GetAll call even if we
have no readable properties in the introspection, in which case we
should return the empty list in the usual way.
We should certainly _not_ be dispatching to the method call handler of
an interface which has no properties (since it will not be expecting
this).
Add a check to make sure that there is at least one readable property
before assuming that a NULL get_property handler implies that we want to
handle properties asynchronously.
Add a testcase that was failing before the change and works after it.
https://bugzilla.gnome.org/show_bug.cgi?id=703437
The existing advice in the documentation to "simply" register the
"org.freedesktop.DBus.Properties" interface if you want to handle
properties asynchronously is pretty unreasonable. If you want to handle
this interface you have to deal with all properties for all interfaces
on the path, and you have to do all of the checking for yourself. You
also have to provide your own introspection data.
Introduce a new convention for dealing with properties asynchronously.
If the user provides NULL for their get_property() or set_property()
functions in the vtable and has properties registered then the
properties are sent to the method_call() handler. We get lucky here
that this function takes an "interface_name" parameter that we can set
to "org.freedesktop.DBus.Properties".
We also do the user the favour of setting the GDBusPropertyInfo on the
GDBusMethodInvocation for their convenience (for much the same reasons
as they might want the already-available GDBusMethodInfo).
Add a testcase as well as a bunch of documentation about this new
feature.
https://bugzilla.gnome.org/show_bug.cgi?id=698375
Separate the code for validating a method call from the code for
actually scheduling it for dispatch.
This will allow property Get/Set/GetAll calls to be dispatched to the
method_call handler without duplicating a lot of code.
https://bugzilla.gnome.org/show_bug.cgi?id=698375
We presently do a lot of checks on property sets (signature check,
correct interface, property exists, etc.) from the worker thread before
dispatching the call to the user's thread. The typecheck, however, is
saved until just before calling the user's vfunc, in their thread.
My best guess is that this was done to save having to unpack the value
from the tuple twice (since we don't unpack it until we're just about
the call the user).
This patch moves the check to the same place as all of the other checks.
The purpose of this change is to allow for sharing this check with the
(soon-to-be-introduced) case of handing property sets from
method_call().
This change has a minor side effect: error messages generated by sending
invalid values to property sets are no longer guaranteed to be correctly
ordered with respect to the void returns from successful property sets.
They will instead be correctly ordered with respect to the other error
messages.
https://bugzilla.gnome.org/show_bug.cgi?id=698375
