Unlike the various functions to call D-Bus methods, these sort their
arguments in a non-obvious order (bus name, interface, signal, path),
presumably aiming to sort the most-likely-to-be-used arguments first.
Signed-off-by: Simon McVittie <smcv@collabora.com>
These function arguments are arranged in the obvious order from
conceptually largest to smallest: (bus name, path, interface, method).
Signed-off-by: Simon McVittie <smcv@collabora.com>
This test was subscribing to the NameOwnerChanged signal with an
incorrect object path, so the callback would never be called. In this
particular case it doesn't actually matter, because the callback does
nothing anyway (the purpose of this particular test was to test that
the user-data is freed on unsubscription).
Signed-off-by: Simon McVittie <smcv@collabora.com>
Almost identically to the previous commit, fix a similar latent bug in
`g_dbus_connection_export_action_group()`, which was not ready to handle
the fledgling `GActionGroupExporter` being freed early on an error
handling path.
See the previous commit message for details of the approach.
This includes a unit test.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Fixes: #3366
This latent bug wasn’t triggered until commit 3f30ec86c (or its
cherry-pick onto `glib-2-80`, 747e3af99, which was first released in
2.80.1).
That change means that `g_menu_exporter_free()` is now called on the
registration failure path by `g_dbus_connection_register_object()`
before it returns. The caller then tries to call `g_slice_free()` on the
exporter again. The call to `g_menu_exporter_free()` tries to
dereference/free members of the exporter which it expects to be
initialised — but because this is happening in an error handling path,
they are not initialised.
If it were to get any further, the `g_slice_free()` would then be a
double-free on the exporter allocation.
Fix that by making `g_menu_exporter_free()` robust to some of the
exporter members being `NULL`, and moving some of the initialisation
code higher in `g_dbus_connection_export_menu_model()`, and removing the
duplicate free code on the error handling path.
This includes a unit test.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Fixes: #3366
gio/glocalfileinfo.c has a struct 'ThumbMD5Context'
that's been unused since
commit d013d46b98 ("Replace the copy-and-paste MD5 digest generation
with GChecksum.")
Remove it.
Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
It's an array containing the list of sanitizers in use, normally it
contains a value, but in some cases may have more than one (e.g.
'address' and 'undefined').
And so use it to avoid repeated checks
In glocalfile we're allocating some temporary strings but we don't free
them on early returns, so free them once done and unset the variables
to prevent them being used incorrectly.
It looks like that finally also valgrind notices the same leaks as
address sanitizer does. It does it more randomly but it still happens,
so better to inform about until #2309 is resolved.
We're now caching arg0 but such value is not cleared when a new body is
set as it's in the connection filter test cases where we've a leak as
highlighted by both valgrind and leak sanitizer
In a D-Bus-Specification-compliant message bus, the owner of a well-known
name is a unique name. However, ibus has its own small implementation
of a message bus (src/ibusbus.c) in which org.freedesktop.IBus is
special-cased to also have itself as its owner (like org.freedesktop.DBus
on a standard message bus), and connects to that bus with the
G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION flag. The ability to do
this regressed when CVE-2024-34397 was fixed.
Relax the checks to allow the owner of a well-known name to be any valid
D-Bus name, even if it is not syntactically a unique name.
Fixes: 683b14b9 "gdbus: Track name owners for signal subscriptions"
Resolves: https://gitlab.gnome.org/GNOME/glib/-/issues/3353
Bug-Debian: https://bugs.debian.org/1070730
Bug-Debian: https://bugs.debian.org/1070736
Bug-Debian: https://bugs.debian.org/1070743
Bug-Debian: https://bugs.debian.org/1070745
Signed-off-by: Simon McVittie <smcv@debian.org>
The test case assumes signals will dispatched in a different order than
they're subscribed. In fact, signals can be dispatched in any order,
and are often dispatched in order.
This commit reorders the subscriptions so they're in order, which is
more logical, and also changes the code to only exit the event loops
when there are no pending handlers ready to dispatch.
This was highlighted (but not introduced) by
0144feb41f. Previously the test coverage
didn’t cover this branch, I think.
`iter` was leaked, and at this point `parameter` had never been set, so
clearing it was unnecessary.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3349
GDBusConnection sends each signal to recipients in a separate idle
callback, and there's no particular guarantee about the order in which
they're scheduled or dispatched. For the NameOwnerChanged signal that
reports the name becoming unowned, it's possible that g_bus_watch_name()
gets its idle callback called before the GDBusProxy:g-name-owner
machinery has updated the name owner, in which case the assertion
will fail.
Fixing GNOME/glib#3268 introduced a new subscription to NameOwnerChanged
which can alter the order of delivery, particularly in the case where
G_DBUS_PROXY_FLAGS_NO_MATCH_RULE was used (as tested in
/gdbus/proxy/no-match-rule). The resulting test failure is intermittent,
but reliably appears within 100 repetitions of that test.
Fixes: 511c5f5b "tests: Wait for gdbus-testserver to die when killing it"
Signed-off-by: Simon McVittie <smcv@debian.org>
If the file to be added is on a read-only filesystem, opening read/write
will fail with EROFS. In this case we should fall back to opening it
read-only, the same way we already do if write access is forbidden by
DAC or MAC.
An easy way to reproduce this test failure is to build and test GLib
in a podman container, with its source code read-only and its build
directory read/write:
podman run --rm -it \
-v $(pwd):$(pwd):ro \
-v $(pwd)/_build:$(pwd)/_build:rw \
-w $(pwd) ...
Before this commit, the dbus-appinfo test would fail, because opening
${srcdir}/gio/tests/org.gtk.test.dbusappinfo.flatpak.desktop read/write
would fail with EROFS.
For completeness, give similar handling to the other error codes
documented in Linux open(2) that might succeed if re-attempted using
read-only access: according to that documentation, we could get EPERM
if opening read/write is prevented by fcntl F_ADD_SEALS, or ETXTBSY
if the file is an executable that is currently being run.
Signed-off-by: Simon McVittie <smcv@collabora.com>
This was a bug that existed during development of this branch; make sure
it doesn't come back.
This test fails with a use-after-free and crash if we comment out the
part of name_watcher_unref_watched_name() that removes the name watcher
from `map_method_serial_to_name_watcher`.
It would also fail with an assertion failure if we asserted in
name_watcher_unref_watched_name() that get_name_owner_serial == 0
(i.e. that GetNameOwner is not in-flight at destruction).
Signed-off-by: Simon McVittie <smcv@collabora.com>
The vulnerability reported as GNOME/glib#3268 can be characterized
as: these signals from an attacker should not be delivered to either
the GDBusConnection or the GDBusProxy, but in fact they are (in at
least some scenarios).
Reproduces: https://gitlab.gnome.org/GNOME/glib/-/issues/3268
Signed-off-by: Simon McVittie <smcv@collabora.com>
The expected result is that because TEST_CONN_SERVICE owns
ALREADY_OWNED_NAME but not (yet) OWNED_LATER_NAME, the signal will be
delivered to the subscriber for the former but not the latter.
Before #3268 was fixed, it was incorrectly delivered to both.
Reproduces: https://gitlab.gnome.org/GNOME/glib/-/issues/3268 (partially)
Signed-off-by: Simon McVittie <smcv@collabora.com>
Otherwise a malicious connection on a shared bus, especially the system
bus, could trick GDBus clients into processing signals sent by the
malicious connection as though they had come from the real owner of a
well-known service name.
Resolves: https://gitlab.gnome.org/GNOME/glib/-/issues/3268
Signed-off-by: Simon McVittie <smcv@collabora.com>
We will use this in a subsequent commit to prevent signals from an
impostor from being delivered to a subscriber.
To avoid message reordering leading to misleading situations, this does
not use the existing mechanism for watching bus name ownership, which
delivers the ownership changes to other main-contexts. Instead, it all
happens on the single thread used by the GDBusWorker, so the order in
which messages are received is the order in which they are processed.
Signed-off-by: Simon McVittie <smcv@collabora.com>
If a connection has two signal subscriptions active for the same signal,
one with arg0 matching and one without, a signal which doesn’t contain
an arg0 value (i.e. `g_dbus_message_get_arg0()` returns `NULL`) will
cause `NULL` to be passed to `strcmp()` when checking for a match
against the signal subscription which *has* arg0 matching, causing a
crash.
Fix that by adding the obvious `NULL` check, and add a unit test.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Fixes: #3342
D-Bus Activation allows passing an array of parameters. Allow apps to
export actions that accept tuples to match the number of elements in the
parameters so the full potential of the D-Bus interface can be used.
Closes: https://gitlab.gnome.org/GNOME/glib/-/issues/3333
See the code comment. scan-build can’t handle analysis over the
refcounts, so consistently complains about potential use-after-free
errors in the code, essentially because:
* It understands `name_unref()`, but completely ignores `name_ref()`
* The code often calls `name_unref()` on the ‘wrong’ pointer, in the
sense that it knows that if another struct exists, that struct holds
a ref on a `Name`, but without actually having a pointer to the
`Name`. So the code calls `name_unref (name); name_unref (name)`.
That’s valid, but quite understandably looks like a recipe for a
use-after-free.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build thinks that there can be a `NULL` pointer dereference of
`error` here because it doesn’t understand that the function return
value and `GError` are related: when a valid return value is returned,
the error is `NULL` and vice-versa.
Try and make that clearer to the static analyser by checking whether the
error is `NULL`, rather than the return value.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build thinks there could be a `NULL` pointer dereference of
`t->data` here. It’s wrong, so add an assertion to try and help it
understand the control flow.
The loop is exited as soon as a target is found whose weight is greater
than or equal to a random value between 0 and the sum of all the weights
in the set of remaining targets in the loop. By definition, the last
target in the loop always satisfies this condition, so a target will
always be chosen, and hence `t` will never be `NULL` within the loop.
`t->data` will never be `NULL` by construction of the target list.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build thinks that `data` could be leaked. It’s not, though; it’s
passed as the `user_data` to `g_dbus_connection_register_object()` along
with its free function.
Try and persuade scan-build that there’s no leak by annotating the
transfer.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build was complaining that `dest_hostname` and `dest_protocol` were
used after being freed, which could potentially happen if the code is
built with `G_DISABLE_CHECKS`. This is a false positive, because the
state of types in the program should be the same regardless of whether
`G_DISABLE_CHECKS` is used.
However, the code did smell. If we are trying to free things and return
gracefully if the underlying socket address enumerator returns something
of the wrong type, why not free the rest of the function’s state, or
skip the invalid address and move on to the next one? Or if we are trying
to make an assertion, why bother freeing some temporary data at all?
This halfway house doesn’t make sense.
So turn the `g_return_val_if_fail()` into a full assertion.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
After a lot of loop unwinding, during which I think it might have lost
its knowledge that `cache->buffer != NULL` (from a prior check on line
765), scan-build seems to think that there can be a `NULL` pointer
dereference of `cache->buffer` within `cache_magic_compare_to_data()`.
There can’t be. Add an assertion to try and help the analyser.
Upstreamed as
https://gitlab.freedesktop.org/xdg/xdgmime/-/merge_requests/38.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build thinks that it’s possible for `read_netlink_messages()` to
return `FALSE` and an unset error (or `TRUE` and a set error), and this
belief causes it to emit warnings for code which calls
`read_netlink_messages()`.
That’s not possible, but the function is written in such a way that
following the control flow would be hard for a static analyser. It would
have to work out that `retval` and `local_error == NULL` are identical
on all control flow branches.
Avoid the need for such complex analysis by eliminating `retval` and
just using `local_error` throughout.
This introduces no functional changes to the code.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build thinks that `term_arg` could be used uninitialised. I think
there isn’t a bug here because that use is protected by the
`found_terminal == NULL` check and early return. But perhaps that logic
is a bit too complex for static analysis, so add a default value for the
variable.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
The previous approach was to return a length as a `gssize`, with
negative values indicating failure. That works fine, but causes a lot of
signed/unsigned comparisons or assignments.
Tidy the code up by splitting success from length, returning success as
a boolean, and length as a `size_t*` out argument. This introduces no
functional changes, but does tidy the code up and fix some compiler
integer warnings.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Basically various trivial instances of the following MSVC compiler
warning:
```
../gio/gio-tool-set.c(50): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data
```
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Looks like the original author mixed up where the link label and the
link URL goes. :p
Previously the link would point to "https://docs.gtk.org/gio/file
attributes", with a space and no file extension.
This will become confusing when we start tracking the owner of a
well-known-name sender, and it's redundant anyway. Instead, track the
1 bit of data that we actually need: whether it's a well-known name.
Strictly speaking this too is redundant, because it's syntactically
derivable from the sender, but only via extra string operations.
A subsequent commit will add a data structure to keep track of the
owner of a well-known-name sender, at which point this boolean will
be replaced by the presence or absence of that data structure.
Signed-off-by: Simon McVittie <smcv@collabora.com>
No functional change, just removing some nesting. The check for whether
signal_data->subscribers is empty changes from a conditional that tests
whether it is into an early-return if it isn't.
A subsequent commit will add additional conditions that make us consider
a SignalData to be still in use and therefore not eligible to be removed.
Signed-off-by: Simon McVittie <smcv@collabora.com>
