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
Back in the far-off twentieth century, it was normal on unix
workstations for U+0060 GRAVE ACCENT to be drawn as "‛" and for U+0027
APOSTROPHE to be drawn as "’". This led to the convention of using
them as poor-man's ‛smart quotes’ in ASCII-only text.
However, "'" is now universally drawn as a vertical line, and "`" at a
45-degree angle, making them an `odd couple' when used together.
Unfortunately, there are lots of very old strings in glib, and also
lots of new strings in which people have kept up the old tradition,
perhaps entirely unaware that it used to not look stupid.
Fix this by just using 'dumb quotes' everywhere.
https://bugzilla.gnome.org/show_bug.cgi?id=700746
g_dbus_connection_call_with_unix_fd_list_sync () and
g_dbus_connection_call_sync () should allow None for the
bus_name parameter.
https://bugzilla.gnome.org/show_bug.cgi?id=683771
Signed-off-by: Richard Hughes <richard@hughsie.com>
Now that we're using g_simple_async_result_set_check_cancellable() we
no longer need this terrible hack of carrying the GCancellable on the
GSimpleAsyncResult using qdata. See bug 672013 for more details.
https://bugzilla.gnome.org/show_bug.cgi?id=672013
Signed-off-by: David Zeuthen <davidz@redhat.com>
Call g_simple_async_result_set_check_cancellable() after all
GSimpleAsyncResult creation in order to take advantage of the new
reliable cancellation feature.
The guarantee of reliable cancellation fixes a bug in dbusmenu (which
was already assuming that cancellation was reliable). See this bug:
https://bugs.launchpad.net/ubuntu/+source/libdbusmenu/+bug/953562https://bugzilla.gnome.org/show_bug.cgi?id=672013
Signed-off-by: David Zeuthen <davidz@redhat.com>
g_bus_get_finish() and g_bus_get_sync() both document that the returned
object will usually have exit-on-close set to TRUE, but the property's
documentation specified that its default is FALSE. While that's
technically true from a GObject perspective, it's not accurate from the
API user's perspective.
https://bugzilla.gnome.org/show_bug.cgi?id=668163
Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <zeuthen@gmail.com>
Previously, this would fail the assertion
"connection->initialization_error != NULL" after the label "out".
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=665067
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
This happens to work at the moment (because GDBusWorker.frozen is a
gboolean and not just a 1-bit bitfield), but isn't right: the gboolean
ends up with values 0 or G_DBUS_CONNECTION_FLAGS_DELAY_MESSAGE_PROCESSING
(which is more than 1).
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=664558
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
Strictly speaking, neither of the two uses that aren't under the lock
*needs* to be atomic, but it seems better to be obviously correct (and
we save another 4 bytes of struct).
One of these uses is in g_dbus_connection_is_closed(), any use of which
is inherently a race condition anyway.
The other is g_dbus_connection_flush_sync, which as far as I can tell
just needs a best-effort check, to not waste effort on a connection that
has been closed for a while (but I could be wrong).
I removed the check for the closed flag altogether in
g_dbus_connection_send_message_with_reply_unlocked, because it turns out
to be redundant with one in g_dbus_connection_send_message_unlocked,
which is called immediately after.
g_dbus_connection_close_sync held the lock to check the closed flag,
which is no longer needed.
As far as I can tell, the only reason why the lock is still desirable
when setting the closed flag is so that remove_match_rule can't fail
by racing with close notification from the worker thread - but
on_worker_closed needs to hold the lock anyway, to deal with other
data structures, so there's no point in trying to eliminate the
requirement to hold the lock.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661992
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
Also, a few that don't need to be.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661992
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
This isn't strictly necessary, because in every location where it's
checked, if the reading thread misses an update from another thread,
it's indistinguishable from the reading thread having been scheduled
before the writing thread, which is an unavoidable race condition that
callers need to cope with anyway. On the other hand, merging exit_on_close
into atomic_flags gives the least astonishing semantics to library users
and saves 4 bytes of struct, and if you're accessing exit-on-close often
enough for it to be a performance concern, you're probably doing it wrong.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661992
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
The thread shared between all GDBusWorker instances was variously called
the "worker thread" or "message handler thread", which I mostly changed to
"the GDBusWorker thread" to avoid ambiguity.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661992
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
The only exceptions are those of the trivial getters/setters that don't
already need the initialization check for its secondary role as a memory
barrier (this is consistent with GSocket, where trivial getters/setters
don't check):
* g_dbus_connection_set_exit_on_close
* g_dbus_connection_get_exit_on_close
* g_dbus_connection_is_closed
g_dbus_connection_set_exit_on_close needs to be safe for
use before initialization anyway, so it can be set at construct-time.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661689
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
Also document which fields require such a check in order to have correct
threading semantics.
This usage doesn't matches the GInitable documentation, which suggests
use of a GError - but using an uninitialized GDBusConnection is
programming error, and not usefully recoverable. (The GInitable
documentation may have been a mistake - GNOME#662208.) Also, not all of
the places where we need it can raise a GError.
The check serves a dual purpose: it turns a non-deterministic crash into
a deterministic critical warning, and is also a memory barrier for
thread-safety. All of these functions dereference or return fields that
are meant to be protected by FLAG_INITIALIZED, so they could crash or
return an undefined value to their caller without this, if called from a
thread that isn't the one that called initable_init() (although I can't
think of any way to do that without encountering a memory barrier,
undefined behaviour, or a race condition that leads to undefined
behaviour if the non-initializing thread wins the race).
One exception is that initable_init() itself makes a synchronous call.
We deal with that by passing new internal flags up the call stack, to
reassure g_dbus_connection_send_message_unlocked() that it can go ahead.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661689
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661992
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
The comment implied that even failed initialization would set
is_initialized = TRUE, but this wasn't the case - failed initialization
would only set initialization_error, and it was necessary to check both.
It turns out the documented semantics are nicer than the implemented
semantics, since this lets us use atomic operations, which are also
memory barriers, to avoid needing separate memory barriers or locks
for initialization_error (and other members that are read-only after
construction).
I expect to need more than one atomically-accessed flag to fix thread
safety, so instead of a minimal implementation I've turned is_initialized
into a flags word.
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661689
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661992
Reviewed-by: David Zeuthen <davidz@redhat.com>
If the connection to the bus is lost while a method call is ongoing,
the method call does not get cancelled. Instead it just sits around
until it times out.
This is visible here on XO laptops when stopping the display manager
during shutdown. imsettings starts sending a sync message to give up
its bus name (via g_bus_unown_name()), then systemd terminates the
session bus at approximately the same time. imsettings then hangs for
about 20 seconds before timing out the message.
http://lists.freedesktop.org/archives/dbus/2011-September/014717.html
imsettings behaviour could be improved as described in that thread,
but I think this is a glib bug. I've also come up with the attached
patch which fixes it.
Credits for the bug-fix goes to Daniel Drake <dsd@laptop.org>. The test
case was written by David Zeuthen <zeuthen@gmail.com>.
https://bugzilla.gnome.org/show_bug.cgi?id=660637
Signed-off-by: David Zeuthen <davidz@redhat.com>
Add g_main_context_ref_thread_default(), which always returns a
reffed GMainContext, rather than sometimes returning a (non-reffed)
GMainContext, and sometimes returning NULL. This simplifies the
bookkeeping in any code that needs to keep a reference to the
thread-default context for a while.
https://bugzilla.gnome.org/show_bug.cgi?id=660994
We can't safely close the output part of the I/O stream until any
pending write or flush has been completed. In the worst case, this could
lead to an assertion failure in the worker (when the close wins the
race) or not closing the stream at all (when the write wins the race).
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=651268
Bug-NB: NB#271520
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Signed-off-by: David Zeuthen <davidz@redhat.com>
