The ref on the timeout source owned by `SendMessageData` was being
dropped just after attaching the source to the main context, leaving it
unowned in that struct. That meant the only ref on the source was held
by the `GMainContext` it was attached to.
This ref was dropped when returning `G_SOURCE_REMOVE` from
`send_message_with_reply_timeout_cb()`. Before that happens,
`send_message_data_deliver_error()` is called, which normally calls
`send_message_with_reply_cleanup()` and destroys the source.
However, if `send_message_data_deliver_error()` is called when the
message has already been delivered, calling
`send_message_with_reply_cleanup()` will be skipped. This leaves the
source pointer in `SendMessageData` dangling, which will cause problems
when `g_source_destroy()` is subsequently called on it.
I’m not sure if it’s possible in practice for this situation to occur,
but the code certainly does nothing to prevent it, and it’s easy enough
to avoid by keeping a strong ref on the source in `SendMessageData`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1264
It already implicitly held a strong ref on its `GTask` values, but
didn’t have a free function set so that they would be automatically
unreffed on removal from the map.
This meant that the functions handling removals from the map,
`on_worker_closed()` (via `cancel_method_on_close()`) and
`send_message_with_reply_cleanup()` had to call unref once more than
they would otherwise.
In `send_message_with_reply_cleanup()`, this behaviour depended on
whether it was called with `remove == TRUE`. If not, it was `(transfer
none)` not `(transfer full)`. This led to bugs in its callers.
For example, this led to a direct leak in `cancel_method_on_close()`, as
it needed to remove tasks from `map_method_serial_to_task`, but called
`send_message_with_reply_cleanup(remove = FALSE)` and erroneously didn’t
call unref an additional time.
Try and simplify it all by setting a `GDestroyNotify` on
`map_method_serial_to_task`’s values, and making the refcount handling
of `send_message_with_reply_cleanup()` not be conditional on its
arguments.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1264
Add a missing steal call in `schedule_method_call()`. This introduces no
functional changes, but documents the ownership transfer more clearly.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2924
Otherwise it’s possible for it to hang around in the `GMainContext`
after the “send message” operation has finished. In the best case, this
will cause the `GTask` and `GDBusMessage` to not be freed when the
calling code expects. In the worst case, it could cause use-after-free
problems if it derefs allocations which have since been freed.
I have not seen either of these problems in practice, but it would be
best for the code to eliminate the risk of them altogether by explicitly
destroying the source when the operation is finished.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2925
This introduces no functional changes, but makes it a little clearer how
the ownership of these `GDBusMessage` instances works. The free function
is changed to `g_clear_object()` to avoid the possibility of somehow
using the messages after freeing them.
Basically just some drive-by docs improvements while trying to debug
issue #1264.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1264
This didn’t actually cause any observable bugs, since the structures of
`PropertyData` and `PropertyGetAllData` were equivalent for the members
which the free function touches.
Definitely should be fixed though.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
This appears to fix an intermittent failure seen when sending a D-Bus
message with either of a cancellable or a timeout set.
In particular, I can reliably reproduce it with:
```
meson test gdbus-test-codegen-min-required-2-64 --repeat 10000
```
It can be caught easily with asan when reproduced. Tracking down the
location of the refcount mismatch was a little tricky, but was
simplified by replacing a load of `g_object_ref (message)` calls with
`g_dbus_message_copy (message, NULL)` to switch `GDBusMessage` handling
to using copy semantics. This allowed asan to home in on where the
refcount mismatch was happening.
The problem was that `send_message_data_deliver_error()` takes ownership
of the `GTask` passed to it, but the
`send_message_with_replace_cancelled_idle_cb()` and
`send_message_with_reply_timeout_cb()` functions which were calling it,
were not passing in a strong reference as they should have.
Another approach to fixing this would have been to change the transfer
semantics of `send_message_data_deliver_error()` so it was `(transfer
none)` on its `GTask`. That would probably have resulted in cleaner
code, but would have been a lot harder to verify/review the fix, and
easier to inadvertently introduce new bugs.
The fact that the bug was only triggered by the cancellation and timeout
callbacks explains why it was intermittent: these code paths are
typically never hit, but the timeout path may sometimes be hit on a very
slow test run.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #1264
We already set names on most sources, this
one was just forgotten. This lets us set
a static name, and prevents g_task_attach_source
from setting a non-static one.
The dominant implementations of the well-known session and system
message buses are the reference implementation from the dbus project
(dbus-daemon) and the sd-bus-based reimplementation dbus-broker, both
of which have correct implementations for EXTERNAL authentication with
an unspecified authorization identity.
This makes it reasonably safe to assume that the well-known message
buses can cope with the unspecified authorization identity, even if we
cannot make the same assumption for custom servers such as the ones
used in ibus and gvfs (which might have been started with an older
GLib version before upgrading GLib in-place).
Signed-off-by: Simon McVittie <smcv@collabora.com>
When using a GDBus client in a non-trivial user namespace, the result of
geteuid() can differ from the uid in the namespace where the server is
running. This would result in connection attempts being rejected, because
the identity that the client claims to have does not match the identity
that the server derives from its credentials.
RFC 4422 allows us to send an empty authorization identity, which means we
want to authenticate as whatever identity the server can derive from our
out-of-band credentials. In particular, this resolves the authentication
failure when crossing between different Linux user namespaces.
Because D-Bus does not have a way to represent an empty initial response
as distinct from the absence of an initial response, we cannot use the
initial-response optimization (RFC 4422 §4.3.a) in this case, and must
fall back to waiting for the server to send a challenge.
Unfortunately, GDBus versions older than glib!2826 did not implement
the server side of this protocol correctly, and would respond to the
missing initial response in a way that breaks the SASL state machine
(expecting a response without sending a challenge), causing client and
server to deadlock with each waiting for the other to respond. Until
fixed versions of GDBus are widespread, we can't rely on having a server
that can cope with this, so gate it behind a flag, which can be set for
connections that are known to cross non-trivial namespace boundaries.
Originally inspired by
<1ed4723d38>,
and based on earlier work by Giuseppe Scrivano (in which the
cross-namespace behaviour was unconditional, rather than gated by a
flag).
Co-authored-by: Giuseppe Scrivano <giuseppe@scrivano.org>
Signed-off-by: Simon McVittie <smcv@collabora.com>
Add SPDX license (but not copyright) headers to all files which follow a
certain pattern in their existing non-machine-readable header comment.
This commit was entirely generated using the command:
```
git ls-files gio/*.[ch] | xargs perl -0777 -pi -e 's/\n \*\n \* This library is free software; you can redistribute it and\/or\n \* modify it under the terms of the GNU Lesser General Public/\n \*\n \* SPDX-License-Identifier: LGPL-2.1-or-later\n \*\n \* This library is free software; you can redistribute it and\/or\n \* modify it under the terms of the GNU Lesser General Public/igs'
```
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1415
The public `g_strv_contains()` API didn’t exist at the time this code
was originally written. Now, happily, it does.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
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
