We should never return unknown errors to the application. This would be
a glib bug.
I don't think it's currently possible to hit these cases, so asserts
should be OK. For this to happen, either (a) a GSocketAddressEnumerator
would have to return NULL on its first enumeration, without returning an
error, or (b) there would have to be a bug in our GSocketClient logic.
Either way, if such a bug were to exist, it would be better to surface
it rather than hide it.
These changes are actually going to be effectively undone in a
subsequent commit, as I'm refactoring the error handling, but the commit
history is a bit nicer with two separate commits, so let's go with two.
GSocketAddressEnumerator encapsulates the details of how DNS happens, so
we don't have to think about it. But we may have taken encapsulation a
bit too far, here. Usually, we resolve a domain name to a list of IPv4
and IPv6 addresses. Then we go through each address in the list and try
to connect to it. Name resolution happens exactly once, at the start.
It doesn't happen each time we enumerate the enumerator. In theory, it
*could*, because we've designed these APIs to be agnostic of underlying
implementation details like DNS and network protocols. But in practice,
we know that's not really what's happening. It's weird to say that we
are RESOLVING what we know to be the same name multiple times. Behind
the scenes, we're not doing that.
This also fixes#1994, where enumeration can end with a RESOLVING event,
even though this is supposed to be the first event rather than the last.
I thought this would be hard to fix, even requiring new public API in
GSocketAddressEnumerator to peek ahead to see if the next enumeration is
going to return NULL. Then I decided we should just fake it: always emit
both RESOLVING and RESOLVED at the same time right after each
enumeration. Finally, I realized we can emit them at the correct time if
we simply assume resolving only happens the first time. This seems like
the most elegant of the possible solutions.
Now, this is a behavior change, and arguably an API break, but it should
align better with reasonable expectations of how GSocketClientEvent
ought to work. I don't expect it to break anything besides tests that
check which order GSocketClientEvent events are emitted in. (Currently,
libsoup has such tests, which will need to be updated.) Ideally we would
have GLib-level tests as well, but in a concession to pragmatism, it's a
lot easier to keep network tests in libsoup.
This isn't an API guarantee, but it's a potentially-surprising
behavior difference between the sync and async functions that is good
to know about, especially because our sync and async functions are
normally identical.
The linux kernel does not know that the socket will be used
for connect or listen and if you bind() to a local address it must
reserve a random port (if port == 0) at bind() time, making very easy
to exhaust the ~32k port range, setting IP_BIND_ADDRESS_NO_PORT tells
the kernel to choose random port at connect() time instead, when the
full 4-tuple is known.
`g_local_file_fstatat()` needs to fall back to returning an error if
`fstatat()` isn’t defined, which is the case on older versions of macOS
(as well as Windows, which was already handled). Callers shouldn’t call
`g_local_file_fstatat()` in these cases. (That’s already the case.)
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #2203
Expose a function that prepares an attribute query string to be passed
to g_file_query_info() to get a list of attributes normally copied with
the file. This function is used by the implementation of
g_file_copy_attributes, and it's useful if one needs to split
g_file_copy_attributes into two stages, for example, when nautilus does
a recursive move of a directory. When files are moved from the source
directory, its modification time changes. To preserve the mtime on the
destination directory, it has to be queried before moving files and set
after doing it, hence these two stages.
Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
The GSubprocessLauncher class lacks a dispose() method, and frees
all their resources in the finalize() method.
This is a problem with Javascript because the sockets passed to a
child process using g_subprocess_launcher_take_fd() aren't closed
in the parent space until the object is fully freed. This means
that if the child closes a socket, it won't be detected until the
GSubprocessLauncher object has been freed by the garbage
collector.
Just closing the socket externally is not a valid solution,
because the finalize() method will close it again, and since
another file/pipe/socket could have been opened in the meantime
and use the same FD number, the finalize() method would close
an incorrect FD.
An example is launching a child process that uses its own
socket for Wayland: the parent creates two sockets with
socketpair(), passes one to the Wayland API (wl_client_create()),
and the other is passed to the child process using
g_subprocess_launcher_take_fd(). But now there are two instances
of that second socket: one in the parent, and another one in the
child process. That means that, if the child closes its socket (or
dies), the Wayland server will not detect that until the
GSubprocessLauncher object is fully destroyed. That means that a
GSubprocessLauncher created in Javascript will last for several
seconds after the child dies, and every window or graphical element
will remain in the screen until the Garbage Collector destroys the
GSubprocessLauncher object.
This patch fixes this by moving the resource free code into a
dispose() method, which can be called from Javascript. This allows
to ensure that any socket passed to the child with
g_subprocess_launcher_take_fd() can be closed even from Javascript
just by calling the method run_dispose().
Fix https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1670
This combines a massive code re-folding with functionlity expansion
that allows us to track multiple verbs per handler or per application.
Also fixes a few issues and removes a function that made no sense.
Like G_SOURCE_REMOVE and G_SOURCE_CONTINUE, these make it clearer what
it means to return TRUE or FALSE.
In particular, in GDBus methods that fail, the failure case still needs
to return TRUE (unlike the typical GError pattern), leading to comments
like this:
g_dbus_method_invocation_return_error (invocation, ...);
return TRUE; /* handled */
which can now be replaced by:
g_dbus_method_invocation_return_error (invocation, ...);
return G_DUS_METHOD_INVOCATION_HANDLED;
G_DBUS_METHOD_INVOCATION_UNHANDLED is added for symmetry, but is very
rarely (perhaps never?) useful in practice.
Signed-off-by: Simon McVittie <smcv@collabora.com>
The previous parsing code could read off the end of a URI if it had an
incorrect %-escaped character in.
Fix that, and more closely implement parsing for the syntax defined in
RFC 6874, which is the amendment to RFC 3986 which specifies zone ID
syntax.
This requires reworking some network-address tests, which were
previously treating zone IDs incorrectly.
oss-fuzz#23816
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Emptying trash over `gio trash` is a bit slow in comparison to plain
`rm -r`. On my system, it took about 3 min to empty the trash with a
folder containing 600 000 files, which is not ideal as `rm -r` call
took just a few seconds. I found that `g_file_delete` is implemented
differently for locations provided by the trash backend. The trash
backend prevents modifications of trashed content thus the delete
operation is allowed only for the top-level files and folders. So it
is not necessary to recursive delete all files as the permission
denied error is returned anyway. Let's call `g_file_delete` only for
top-level items, which reduces the time necessary for emptying trash
from minutes to seconds...
See: https://gitlab.gnome.org/GNOME/nautilus/-/issues/1589
Some filesystems don't have meaningful access times under at least some
circumstances (see #2189, #2205). In this situation the traditional stat()
and related kernel interfaces have to put something meaningless in the
st_atime field, and have no way to signal that it is meaningless.
However, statx() does have a way to signal that the atime is meaningless:
if the filesystem doesn't provide a useful access time, it will unset
the STATX_ATIME bit (as well as filling in the same meaningless value
for the stx_atime field that stat() would have used, for compatibility).
We don't actually *need* the atime, so never include it in the required
mask. This was already done for one code path in commit 6fc143bb
"gio: Allow no atime from statx" to fix#2189, but other callers were
left unchanged in that commit, and receive the same change here.
It is not actually guaranteed that *any* of the flags in the
returned stx_mask will be set (the only guarantee is that items in
STATX_BASIC_STATS have at least a harmless compatibility value, even if
their corresponding flag is cleared), so it might be better to follow
this up by removing the concept of the required mask entirely. However,
as of Linux 5.8 it looks as though STATX_ATIME is the only flag in
STATX_BASIC_STATS that might be cleared in practice, so this simpler
change fixes the immediate regression.
Resolves: https://gitlab.gnome.org/GNOME/glib/-/issues/2205
Signed-off-by: Simon McVittie <smcv@collabora.com>
It is not allowed to be `NULL` or unset if requested by the file
attribute matcher. Derive it from the basename. This doesn’t handle the
situation of a failed UTF-16 to UTF-8 conversion very well, but will at
least return something.
Note that the `g_filename_display_basename()` function can’t be used as
`GWinHttpFile` provides its URI in UTF-16 rather than in the file system
encoding.
This fixes a crash when using GIMP on Windows. Thanks to lillolollo for
in-depth debugging assistance.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #2194
For interoperability with libdbus, we want to use compatible timeouts.
In particular, this fixes a spurious failure of the `gdbus-server-auth`
test caused by libdbus and gdbus choosing to expire the key (cookie) at
different times, as diagnosed by Thiago Macieira. Previously, the libdbus
client would decline to use keys older than 7 minutes, but the GDBus
server would not generate a new key until the old key was 10 minutes old.
For completeness, also adjust the other arbitrary timeouts in the
DBUS_COOKIE_SHA1 mechanism to be the same as in libdbus. To make it
easier to align with libdbus, create internal macros with the same names
and values used in dbus-keyring.c.
* maximum time a key can be in the future due to clock skew between
systems sharing a home directory
- spec says "a reasonable time in the future"
- was 1 day
- now 5 minutes
- MAX_TIME_TRAVEL_SECONDS
* time to generate a new key if the newest is older
- spec says "If no recent keys remain, the server may generate a new
key", but that isn't practical, because in reality we need a grace
period during which an old key will not be used for new authentication
attempts but old authentication attempts can continue (in practice both
libdbus and GDBus implemented this logic)
- was 10 minutes
- now 5 minutes
- NEW_KEY_TIMEOUT_SECONDS
* time to discard old keys
- spec says "the timeout can be fairly short"
- was 15 minutes
- now 7 minutes
- EXPIRE_KEYS_TIMEOUT_SECONDS
* time allowed for a client using an old key to authenticate, before
that key gets deleted
- was at least 5 minutes
- now at least 2 minutes
- at least (EXPIRE_KEYS_TIMEOUT_SECONDS - NEW_KEY_TIMEOUT_SECONDS)
Based on a merge request by Philip Withnall.
Fixes: #2164
Thanks: Philip Withnall
Thanks: Thiago Macieira
Signed-off-by: Simon McVittie <smcv@collabora.com>
This doesn't trigger the cancellation assertion issue when run locally
(the task didn't return yet, so the error is simply overwritten), but
perhaps it ever does in CI. Anyhow, it's good to have a cancellation
test.
After a splice operation is finished, it attempts to 1) close input/output
streams, as per the given flags, and 2) return the operation result (maybe
an error, too).
However, if the operation gets cancelled early and the streams indirectly
closed, the splice operation will try to close both descriptors and return
on the task when both are already closed. The catch here is that getting the
streams closed under its feet is possible, so the completion callback would
find both streams closed after returning on the first close operation and
return the error, but then the second operation could be able to trigger
a second error which would be returned as well.
What happens here is up to further race conditions, if the task didn't
return yet, the returned error will be simply replaced (but the old one not
freed...), if it did already return, it'll result in:
GLib-GIO-FATAL-CRITICAL: g_task_return_error: assertion '!task->ever_returned' failed
Fix this by flagging the close_async() callbacks, and checking that both
close operations did return, instead of checking that both streams are
closed by who knows.
This error triggers a semi-frequent CI failure in tracker, see the summary at
https://gitlab.gnome.org/GNOME/tracker/-/issues/240
statx does not provide stx_atime when querying a file in a read-only
mounted file system. So call to statx should not expect it to be in
the mask. Otherwise we would fail with ERANGE for querying any file in
a read-only file system.
Fixes#2189.
The `make_pollfd()` call can’t fail because it only does so if
`cancellable == NULL`, and we’ve already checked that. Assert that’s the
case, to shut Coverity up and to catch behavioural changes in future.
Coverity CID: #1159433
Signed-off-by: Philip Withnall <withnall@endlessm.com>
`g_strsplit()` never returns `NULL`, although it can return an empty
strv (i.e. with its first element being `NULL`).
Drop a redundant `NULL` check.
Coverity CID: #1430976
Signed-off-by: Philip Withnall <withnall@endlessm.com>
If `statx()` is supported, query it for the file creation time and use
that if returned.
Incorporating some minor code rearrangement by Philip Withnall
<withnall@endlessm.com>.
Fixes: #1970
This currently just implements the same functionality as the existing
`stat()`/`fstat()`/`fstatat()`/`lstat()` calls, although where a reduced
field set is requested it may return faster.
Helps: #1970
It turns out that our async write operation implementation is broken
on non-O_NONBLOCK pipes, because the default async write
implementation calls write() after poll() said there were some
space. However, the semantics of pipes is that unless O_NONBLOCK is set
then the write *will* block if the passed in write count is larger than
the available space.
This caused a deadlock in https://gitlab.gnome.org/GNOME/glib/-/issues/2182
due to the loop-back of the app stdout to the parent, but even without such
a deadlock it is a problem that we may block the mainloop at all.
In the particular case of g_subprocess_communicate() we have full
control of the pipes after starting the app, so it is safe to enable
O_NONBLOCK (i.e. we can ensure all the code using the fd after this can handle
non-blocking mode).
This fixes https://gitlab.gnome.org/GNOME/glib/-/issues/2182
This has almost the same semantics as WSAECONNRESET and for all
practical purposes is handled the same. The main difference is about
*who* reset the connection: the peer or something in the network.
For UDP sockets this happens when receiving packets and previously sent
packets returned an ICMP "Time(-to-live) expired" message. This is
similar to WSAECONNRESET, which on UDP sockets happens when receiving
packets and previously sent packets returned an ICMP "Port Unreachable"
message.
This is a step towards supporting `statx()`, which allows the set of
fields it returns to be specified by the caller. Currently, the existing
`stat()` and `fstat()` calls continue to be made, and there are no
behavioural changes — but the new wrapper functions will be extended in
future.
Helps: #1970
Don’t call `g_file_query_exists()` followed by `g_file_delete()`. Just
call `g_file_delete()` and check the error.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Make `G_URI_FLAGS_PARSE_RELAXED` available instead, for the
implementations which need to handle user-provided or incorrect URIs.
The default should nudge people towards being compliant with RFC 3986.
This required also adding a new `G_URI_PARAMS_PARSE_RELAXED` flag, as
previously parsing param strings *always* used relaxed mode and there
was no way to control it. Now it defaults to using strict mode, and the
new flag allows for relaxed mode to be enabled if needed.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #2149
Add support for x-gvfs-notrash mount option, which allows to disable
trash functionality for certain mounts. This might be especially useful
e.g. to prevent trash folder creation on enterprise shares, which are
also accessed from Windows...
https://bugzilla.redhat.com/show_bug.cgi?id=1096200
There is already g_unix_mount_at function which allows to find certain
unix mount for given mount path. It would be useful to have similar
function for mount points, which will allow to replace custom codes in
gvfs. Let's add g_unix_mount_point_at.
_g_uri_parse_authority() can be replaced with g_uri_split_network() &
PARSE_STRICT. Keep the original error code, for compatibility reasons.
Notice that GUri uses gint for the port, and value -1 if the port value
is missing. However, GNetworkAddress::port is a guint.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
_g_uri_parse_authority() without argument is actually checking that the
URI is valid, by checking it parses successfully
We keep the existing error domain / code for compatibility reasons,
instead of raising the underlying G_URI_ERROR.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
_g_uri_from_authority() is doing the same work as g_uri_join(): taking
URI components and merging them in a legit URI string, with encoding.
It turns out g_uri_from_authority was unnecessarily complex, since no
caller used the userinfo field.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
It may be defined by the environment (we document that as being allowed)
— if so, individual files should not try to redefine it, as that causes
a preprocessor warning.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Where applicable. Where the current use of `g_file_set_contents()` seems
the most appropriate, leave that in place.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #1302
Give GAppInfo a bunch of readonly properties, and
support them in GDesktopAppInfo. This makes app infos
more convenient to work with in GTK4, and in general.
g_task_set_name() was added in GLib 2.60, so only use it in the
overridden definition of g_task_set_source_tag() if the user has said
that they require GLib ≥ 2.60.
This is a follow up to commit b08bd04abe.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
There is no guarantee that this function would not be called
concurrently. Particularly since flatpak_info_read was set to TRUE
before /.flatpak-info is actually read from disk, there is a potential
race where a second thread would return default values for the various
flags set from that file.
Fixes#2159
Correct an off-by-one error in hex_unescape_string()'s computation of
the output string length.
(Turned into a git-format patch by Philip Withnall. Original patch
submitted on the Debian bug tracker, bug#962912.)
It's safe to assume an escaped string doesn't contain embedded null bytes,
but raw memory buffers (as returned by getxattr()) require more care.
If the length of the data to be escaped is known, use that knowledge instead
of invoking strlen().
(Turned into a git-format patch by Philip Withnall. One minor formatting
tweak. Original patch submitted on the Debian bug tracker, bug#962912.)
Fixes: #422
And improve them externally, where not otherwise set, by setting them
from the function name passed to `g_task_set_source_tag()`, if called by
third party code.
This should make profiling and debug output from GLib more useful.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
`complete_in_idle_cb()` shows up in a lot of sysprof traces, so it’s
quite useful to include the most specific contextual information we can
in it.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
When an app is spawned using g_desktop_app_info_launch_uris_with_spawn
it will expand the various token in the app's commandline with the
URIs of the files to open. The expand_macro() function that is used for
this advances the pointer to the URI list to show up to which entries
it used.
To not loose the pointer to the list head a duplicate of the URI list
was actually passed to expand_macro(). However, it's not necessary to
create a copy of the URI list for that as expand_macro() will only
change which element the pointer will point to.
This behaviour actually caused the duplicated list to be leaked as the
the list pointer is NULL once all URIs are used up by expand_macro()
and thus nothing was freed at the end of the function.
In ostree based systems, such as flatpak and fedora silverblue, the
time of modification of every system file is epoch 0, including
giomodule.cache, which means that every module is loaded and unloaded
every time.
The solution is to use the change time of the file as well. In a typical
system, it is equal to the mtime, and in an ostree based system, since
the directory is mounted as read-only, the user cannot add a module and
we must assume that the cache file corresponds to the modules.
* Add g_tls_connection_get_channel_binding_data API call
* Add g_dtls_connection_get_channel_binding_data API call
* Add get_binding_data method to GTlsConnection class
* Add get_binding_data method to GDtlsConnection interface
* Add GTlsChannelBindingType enum with tls-unique and
tls-server-end-point types
* Add GTlsChannelBindingError enum and G_TLS_CHANNEL_BINDING_ERROR
quark
* Add new API calls to documentation reference gio-sections-common
This speeds up the `cancellable` test a little by stopping waiting for
the threads to start up as soon as they have started, rather than after
an arbitrary timeout.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #1764
This should fix some sporadic test failures in this test, although I
can’t be sure as I was unable to reproduce the original failure.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1764
It seems that allowing the GCancellable to be finalised in either the
main thread or the worker thread sometimes leads to crashes when running
on CI.
I cannot reproduce these crashes locally, and various analyses with
memcheck, drd and helgrind have failed to give any clues.
Fix this for this particular test case by deferring destruction of the
`GCancellable` instances until after the worker thread has joined.
That’s OK because this test is specifically checking a race between
`g_cancellable_cancel()` and disposal of a `GCancellableSource`.
The underlying bug remains unfixed, though, and I can only hope that we
eventually find a reliable way of reproducing it so it can be analysed
and fixed.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
The G_FILE_ATTRIBUTE_FILESYSTEM_REMOTE is set to TRUE only for NFS
filesystem types currently. Let's add also SMB filesystem types. This
also changes g_local_file_is_nfs_home function logic to handle only
NFS filesystems.
The g_local_file_is_remote function is misleading as it works only for
NFS filesystem types and only for locations in home directorly. Let's
rename it to g_local_file_is_nfs_home to make it obvious.
statfs/statvfs is called several times when querying filesystem info.
This is because the G_FILE_ATTRIBUTE_FILESYSTEM_REMOTE attribute is set
over is_remote_fs function, which calls statfs/statvfs again. Let's use
the already known fstype instead of redundant statfs/statvfs calls.
This also changes g_local_file_is_remote implementation to use
g_local_file_query_filesystem_info to obtain fstype, which allows to
remove duplicated code from is_remote_fs function.
The G_FILE_ATTRIBUTE_FILESYSTEM_REMOTE currently works only for locations
in the home directory. Let's make it work also for files outside the home
directory.
There are glocalfile.h and glocalfileprivate.h header files currently.
None of those header files is public, so it doesn't make sense to have
two private headers for glocalfile.c. Let's remove glocalfileprivate.h.
This was mostly machine generated with the following command:
```
codespell \
--builtin clear,rare,usage \
--skip './po/*' --skip './.git/*' --skip './NEWS*' \
--write-changes .
```
using the latest git version of `codespell` as per [these
instructions](https://github.com/codespell-project/codespell#user-content-updating).
Then I manually checked each change using `git add -p`, made a few
manual fixups and dropped a load of incorrect changes.
There are still some outdated or loaded terms used in GLib, mostly to do
with git branch terminology. They will need to be changed later as part
of a wider migration of git terminology.
If I’ve missed anything, please file an issue!
Signed-off-by: Philip Withnall <withnall@endlessm.com>
In glib-networking#127, it was reported that we don't properly implement
the documented behavior of these properties. However, we cannot fix it
because libsoup relies on the implemented behavior, and it's hard to
change that without cascading breakage. The practical solution is to
adjust our documentation to match reality. There should be no downsides
to this, and compat risk of changing the documentation is much smaller
than risk of changing the implementation, so I think this is the best we
can make of an unfortunate situation. See glib-networking#127 for full
discussion and glib-networking#129 for the regression when we attempted
to match the documented behavior.
Some editors automatically remove trailing blank lines, or
automatically add a trailing newline to avoid having a trailing
non-blank line that is not terminated by a newline. To avoid unrelated
whitespace changes when users of such editors contribute to GLib,
let's pre-emptively normalize all files.
Unlike more intrusive whitespace normalization like removing trailing
whitespace from each line, this seems unlikely to cause significant
issues with cherry-picking changes to stable branches.
Implemented by:
find . -name '*.[ch]' -print0 | \
xargs -0 perl -0777 -p -i -e 's/\n+\z//g; s/\z/\n/g'
Signed-off-by: Simon McVittie <smcv@collabora.com>
While these assertions look right at the first glance,
they actually crash the program. That's because GObject
insists on initializing all construct-only properties
to their default values, which results in
g_win32_registry_key_set_property() being called multiple
times with NULL string, once for each unset property.
If "path" is actually set by the caller, a subsequent
call to set "path-utf16" to NULL will fail an assertion,
since absolute_path is already non-NULL.
With assertions moved the set-to-NULL calls bail out before
an assertion is made.
This ensures that we do really export the symbols for Visual
Studio-style builds, by using _GLIB_EXTERN to decorate the generated
prototypes and including config.h so that we are sure the symbols are
actually exported.
This adds three options to gdbus-codegen so that we may be able to
use a self-defined symbol decorator, such as _GLIB_EXTERN, to decorate
the generated prototypes, to be used possibly to export the symbols, if
needed.
The other two options allows including headers that are required for the
specified symbol decorator to be usable and preprocessor macros that are
required for the symbol decorator to be defined appropriately, also when
needed.
Have the generated .c code decorate the prototypes with "G_MODULE_EXPORT"
instead of "extern" when --internal is not being used, so that we also
export the symbols from the generated code on Visual Studio-style
compilers. If --internal is used, we decorate the prototypes with
"G_GNUC_INTERNAL", as we did before.
Note that since the generated .c code does not attempt to include the
generated headers (if one is also generated), the gnerated headers are
still generated as they were before.
Sometimes this test was timing out due to the file monitor notifications
taking longer than the arbitrary 2s delay before ending the test and
checking its results at the end of `iclosed_cb()`.
Avoid that timing-dependence by ending the test when the expected file
monitor notifications are seen, or after a 10s timeout (if so, the test
is failed).
This makes the test run 4× faster in the normal case, as it’s no longer
waiting for a timeout to elapse if the file monitor notifications come
in sooner.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
The test added for #1841 spawned 100000 threads. That was fine on a
desktop machine, but on a heavily loaded CI machine, it could result in
large (and unpredictable) slowdowns, resulting in the test taking over
120s in about 1 in 5 runs, and hence failing that CI pipeline due to a
timeout. When passing normally on CI, the test would take around 90s.
Here’s a histogram of time per iteration on a failing (timed out) test
run. Each iteration is one thread spawn:
Iteration duration (µs) | Frequency
------------------------+----------
≤100 | 0
100–200 | 30257
200–400 | 13696
400–800 | 1046
800–1000 | 123
1000–2000 | 583
2000–4000 | 3779
4000–8000 | 4972
8000–10000 | 1027
10000–20000 | 2610
20000–40000 | 650
40000–80000 | 86
80000–100000 | 10
100000–200000 | 2
>200000 | 0
There’s no actual need for the test to spawn 100000 threads, so rewrite
it to reuse a single thread, and pass new data to that thread.
Reverting the original commit (e4a690f5dd) reproduces the failure on
100 out of 100 test runs with this commit applied, so the test still
works.
The test now takes 3s, rather than 11s, to run on my computer, and has
passed when run with `meson test --repeat 1000 cancellable`.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
An extra argument to g_win32_registry_key_get_value_w() and
g_win32_registry_key_get_value() indicates that RegLoadMUIStringW()
should be used instead of RegQueryValueExW(). It only works on
strings, and automatically resolves resource strings (the ones
that start with "@").
The extra argument is needed to find resource DLLs that are only
specified by their relative name.
It is critical to mention how the identity parameter is expected to be
handled. In particular, if identity is not passed, then the identity of
the server certificate will not be checked at all. This is in contrast
to the connection-level APIs, which are supposed to be fail-safe. The
database and certificate-level APIs are more manual.
There’s no need to call `access()` and then `stat()` on the keyring
directory to check that it exists, is a directory, and has the right
permissions. Just call `stat()`.
This eliminates one potential TOCTTOU race in this code.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #1954
There was a time-of-check-to-time-of-use (TOCTTOU) race in the keyring
lock code, where it would check the existence of the lock file using
`access()`, then proceed to call `open(O_CREAT | O_EXCL)` to try and
create the lock file once `access()` showed that it didn’t exist.
The problem is that, because this is happening in a shared directory
(`~/.dbus-keyrings`), another process could quite legitimately create
the lock file in the meantime.
Instead, unconditionally call `open()` and ignore errors from it (which
will be returned if the lock file already exists) until it succeeds (or
the code times out).
This eliminates the TOCTTOU race, and simplifies the timeout behaviour
so there aren’t two loops (check for existence, try to create)
happening. It brings this code in line with what dbus.git does (see
`_dbus_keyring_lock()`).
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #1954
When multiple tests were run in parallel, this would race on its access
to `~/.dbus-keyrings` to authenticate with the D-Bus server, since the
keyring directory was not appropriately sandboxed to the unit test.
Use `G_TEST_OPTION_ISOLATE_DIRS` to automatically isolate each unit
test’s directory usage.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #1954
Commit 721e385 left one remaining race in the filter test, with a
comment associated with it. Unfortunately, the (seemingly unrelated)
changes in #1841 to `GCancellable` seem to have made this remaining race
a lot more likely to fail on FreeBSD than before.
What’s likely to have happened (although I was unable to reproduce the
failure, due to not having a FreeBSD system; I was only able to
reproduce the problem as a 3/1000 failure on Linux, which is still worth
fixing) is that the atomic write of the `FilterData.serial` to be
expected by the filter function sometimes happened after the filter
function had executed, so the expected message was dropped and didn’t
result in an update to the `FilterData` state.
Rework the test so that instead of setting some expectations (on
`FilterData`) in one thread and then checking them in another thread,
the worker thread just unconditionally returns messages from the filter
function to the main thread, and then the main thread checks whether the
expected one has been filtered.
With this change applied, the `gdbus-connection` test passes 5000 times
in a row for me, on Linux; and doesn’t seem to fail any more on the
FreeBSD CI machines over a few runs. (Previously it failed on 4/5 runs.)
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #2092Fixes: #1957
Mention in the documentation that (presumably for performance reasons)
the search results from `g_desktop_app_info_search()` are not filtered
by executable presence or hidden attribute.
Perhaps they should be in future, but for now we should at least
document it.
Spotted by Will Thompson.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
By default, meson builds glib with -Werror=format=2, which
implies -Werror=format-nonliteral. With these flags, clang errors
out on e.g. the g_message_win32_error function, due to "format
string is not a string literal". This function takes a format
string, and passes the va_list of the arguments onwards to
g_strdup_vprintf, which is annotated with printf attributes.
When passing a string+va_list to another function, GCC doesn't warn
with -Wformat-nonliteral. Clang however does warn, unless the
functions themselves (g_message_win32_error and set_error) are decorated
with similar printf attributes (to force the same checks upon the
caller) - see
https://clang.llvm.org/docs/AttributeReference.html#format
for reference.
Adding these attributes revealed one existing mismatched format string
(fixed in the preceding commit).
The GIO tests memory-monitor-dbus and memory-monitor-portal use a number
of third party Python modules that may not be present when running the
test case.
Instead of failing due to missing imports, catch the ImportError and
mock a test case that skips. This can't use the usual unittest.skip
logic because the test case class itself uses a 3rd party module.
Closes#2083.
There are two memory monitor tests that use Python's unittest module directly,
but GLib tests should be outputting TAP. Use the embedded TAPTestRunner to
ensure that TAP is output for these tests too.
The G_FILE_ATTRIBUTE_STANDARD_CONTENT_TYPE attribute doesn't have to be
always set. See https://gitlab.gnome.org/GNOME/gvfs/-/merge_requests/68
for more details. In that case, the g_file_query_default_handler function
fails with the "No application is registered as handling this file" error.
Let's fallback to the "standard::fast-content-type" attribute instead to
fix issues when opening such files.
https://gitlab.gnome.org/GNOME/nautilus/-/issues/1425
Meson 0.54.0 added a new method meson.override_dependency() that must be
used to ensure dependency consistency. This patch ensures a project that
depends on glib will never link to a mix of system and subproject
libraries. It would happen in such cases:
The system has glib 2.40 installed, and a project does:
dependency('glib-2.0', version: '>=2.60',
fallback: ['glib', 'glib_dep'])
dependency('gobject-2.0')
The first call will configure glib subproject because the system libglib
is too old, but the 2nd call will return system libgobject.
By overriding 'gobject-2.0' dependency while configuring glib subproject
during the first call, meson knows that on the 2nd call it must return
the subproject dependency instead of system dependency.
This also has the nice side effect that with Meson >0.54.0 an
application depending on glib can declare the fallback without knowing
the dependency variable name: dependency('glib-2.0', fallback: 'glib').
Slightly unexpectedly, `g_icon_serialize()` doesn’t produce a floating
`GVariant`, it produces one with full ownership and returns that. That’s
not the convention for `GVariant` return values from functions which
build variants, but there’s nothing we can do to change this now as that
would be an API break.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
D-Bus filter functions run in a worker thread. The `gdbus-connection`
test was sharing a `FilterData` struct between the main thread and the
filter function, which was occasionally (on the order of 0.01% of test
runs) causing spurious test failures due to racing on reads/writes of
`num_handled`.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #480
g_assert() can be compiled out with G_DISABLE_ASSERT, which renders the
test rather useless.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #480
If a username and password are specified by the caller, `GSocks5Proxy`
tells the server that it supports anonymous *and* username/password
authentication, and the server can choose which it prefers.
Otherwise, `GSocks5Proxy` only says that it supports anonymous
authentication. If that’s not acceptable to the server, the code was
previously returning `G_IO_ERROR_PROXY_AUTH_FAILED`. That error code
doesn’t indicate to the caller that authentication might succeed were
they to provide a username and password.
Change the error handling to make that clearer. A fuller solution would
be to expose more of the method negotiation in the `GSocks5Proxy` API,
so that the caller can specify ahead of time which authentication
methods they want to use. That can follow in issue #2059 though.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #1988
They were not actually asynchronous, and hence caused blocking in the
main thread. Deleting them means the default implementation of those
vfuncs is used, which runs the sync implementation in a thread — which
is what is wanted here.
Spotted by Benjamin Otte.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #2051
There’s a minor race condition between cancellation of a `GCancellable`,
and disposal/finalisation of a `GCancellableSource` in another thread.
Thread A Thread B
g_cancellable_cancel(C)
→cancellable_source_cancelled(C, S)
g_source_unref(S)
cancellable_source_dispose(S)
→→g_source_ref(S)
→→# S is invalid at this point; crash
Thankfully, the `GCancellable` sets `cancelled_running` while it’s
emitting the `cancelled` signal, so if `cancellable_source_dispose()` is
called while that’s high, we know that the thread which is doing the
cancellation has already started (or is committed to starting) calling
`cancellable_source_cancelled()`.
Fix the race by resurrecting the `GCancellableSource` in
`cancellable_source_dispose()`, and signalling this using
`GCancellableSource.resurrected_during_cancellation`. Check for that
flag in `cancellable_source_cancelled()` and ignore cancellation if it’s
set.
The modifications to `resurrected_during_cancellation` and the
cancellable source’s refcount have to be done with `cancellable_mutex`
held so that they are seen atomically by each thread. This should not
affect performance too much, as it only happens during cancellation or
disposal of a `GCancellableSource`.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #1841
`g_assert()` is compiled out if `G_DISABLE_ASSERT` is defined, and
`g_assert_*()` gives more detailed failure messages.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Guard against NULL type being passed to
g_content_type_get_generic_icon_name() just as we protect
g_content_type_get_description(), otherwise it will cause a crash.
See https://gitlab.gnome.org/GNOME/gtk/issues/2482
Distributions will likely want to update GLib before
GObject-Introspection, to avoid circular dependencies.
Signed-off-by: Simon McVittie <smcv@debian.org>
It was checking for the main SOCKS5 version number, rather than the
subnegotiation version number. The username/password authentication
protocol is described in https://tools.ietf.org/html/rfc1929.
Spotted and diagnosed by lovetox.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #1986
Clang warns about string+int not appending to the string (to try and
catch newbie mistakes). While this test didn’t expect that to happen, it
was substituting the same constant string in multiple places for no good
reason. Switch to a single static const string, which should also fix
the compiler warning.
We have to define the string length since it’s used in various
stack-allocated array lengths. This is the easiest fix without more
major refactoring of the test to be less 90s.
Also make things a bit more static.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
When running under CI, each iteration takes so long that the total test
time is around 200s. If the CI runner is highly loaded, this can tip it
over the timeout of 360s.
Reduce the iteration counts unless running the test thoroughly.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #1515
Currently the test waits for 1s before deciding that a refcount has been
leaked. But slow test machines might take longer than that between
scheduling different threads to sort out the refcount, so increase the
timeout.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #1515
Previously, if the `--address` option was passed to `gdbus-tool`, it
would treat the connection as peer to peer. However, almost all the
commands `gdbus-tool` supports require a message bus (introspection,
calling a method with a destination, etc.). Only the `signal` command
would ever work on a peer-to-peer connection (if no `--dest` was
specified).
So change the `--address` option to generally create message bus
connections.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #938
bindfs is part of the setup process, so if it fails (as can happen if
the `fuse` kernel module has not been loaded — not much we can do about
that) then skip the test.
Signed-off-by: Philip Withnall <withnall@endlessm.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>
When testing that signals are delivered to the correct thread, and are
delivered the correct number of times, call `EmitSignal()` on the
`gdbus-testserver` to trigger a signal emission, and listen for that.
Previously, the code listened for `NameOwnerChanged` and connected to
the bus again to trigger emission of that. The problem with that is that
other things happening on the bus (for example, an old
`gdbus-testserver` instance disconnecting) can cause `NameOwnerChanged`
signal emissions. Sometimes, the `gdbus-threading` test was failing the
`signal_count == 1` assertion due to receiving more than one
`NameOwnerChanged` emission.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #1515
This is equivalent, but makes the loop exit conditions a little clearer,
since they’re actually in a `while` statement, rather than being a
`g_main_loop_quit()` call in a callback somewhere else in the file.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #1515
As with the previous commit, don’t stop iterating the `context` in
`test_delivery_in_thread_func()` until the unsubscription from a signal
is complete, and hence there’s a guarantee that no callbacks are pending
in the `thread_context`.
This commit uses the `GDestroyNotify` for
`g_dbus_connection_signal_subscribe()` as a synchronisation message from
the D-Bus worker thread to the `test_delivery_in_thread_func()` thread
to notify of signal unsubscription.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #1515
Previously, the code in `ensure_gdbus_testserver_up()` created a proxy
object and watched its `name-owner` to see when the
`com.example.TestService` name appeared.
This ended up subscribing to three signals (one of them for name
ownership, and two unused for properties of the proxy), and was racy. In
particular, the `name-owner` property could be set before all D-Bus
messages had been processed — it could have been derived from getting
the owner of the name, for example.
This left unprocessed messages hanging around in the `context`, but that
context was never iterated again, which essentially leaked the
references held by those messages. That included a reference to the
`GDBusConnection`.
The first part of the fix is to simplify the code to use
`g_bus_watch_name_on_connection()`, so there’s only one signal
subscription to worry about.
The second part of the fix is to use the `GDestroyNotify` callback for
the watch data to be notified of when all D-Bus traffic has been
processed and the signal unsubscription is complete. At this point, it’s
guaranteed that there are no idle callbacks pending in the
`GMainContext`, since the `GDestroyNotify` callback is the last one
invoked on the `GMainContext`.
Essentially, this commit uses the `GDestroyNotify` callback as a
synchronisation message between the D-Bus worker thread and the thread
calling `ensure_gdbus_testserver_up()`.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #1515
Iterate the given `context` while waiting, rather than sleeping. This
ensures that if the errant `GDBusConnection` ref is held by some pending
callback in the given `context`, it will actually be released.
Typically `context` is going to be the global default main context.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #1515
This introduces no functional changes, but makes the code a little more
explicit about which connection and main context it’s operating on.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #1515
`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
The fix for bgo#651133 (commit 7e0f890e38) introduced a kind of weak
ref, which had to be thread-safe due to the fact that `GDBusProxy`
operates in one thread but can emit signals in another.
Since that commit, `GWeakRef` was added, which does the same thing. Drop
the custom code in favour of it; this should be functionally equivalent,
but using an RW lock rather than a basic mutex, which should reduce
contention.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
These checks used to be a precondition on test_threaded_singleton(); but
the earlier tests could leave the refcount of the shared connection in a
bad state, and this wouldn’t be caught until later.
Factor out the check, increase the iteration count to 1000 (so the check
blocks for up to 1s rather than 100ms), and call it in more places.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
https://gitlab.gnome.org/GNOME/glib/issues/1515
g_assert() can be compiled out with G_DISABLE_ASSERT, which renders the
test rather useless.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
https://gitlab.gnome.org/GNOME/glib/issues/1515
mtab_file_changed_id is not currently removed when finalizing, which
could potentially lead to segfaults. Let's remove the source when
finalizing to avoid this.
mtab_file_changed_id might be set on thread default context, but it is
always cleared on the global context because of usage of g_idle_add. This
can cause the emission of redundant "mounts-change" signals. This should
not cause any issues to the client application, but let's attach the idle
source to the thread-default context instead to avoid those races for sure.
The `get_mounts_timestamp()` function uses `mount_poller_time` when
`proc_mounts_watch_source` is set, but the `mount_poller_time` is not
initialized in the same time as `proc_mounts_watch_source`. This may
cause that zero, or some outdated value is returned. Let's initialize
`mount_poller_time` to prevent invalid values to be returned.
The Nautilus test suite often crashes with "GLib-FATAL-CRITICAL:
g_source_is_destroyed: assertion 'g_atomic_int_get (&source->ref_count)
> 0' failed" if it is started with "GIO_USE_VOLUME_MONITOR=unix". This
is because GUnixMountMonitor is simultaneously used from multiple
threads over GLocalFile and GVolumeMonitor APIs. Let's add guards for
proc_mounts_watch_source and mount_poller_time variables to prevent
those crashes.
Fixes: https://gitlab.gnome.org/GNOME/glib/issues/2030
There was a slight race in name ownership: a gap between calling
`RequestName` (or receiving its reply) and subscribing to `NameLost`. In
that gap, another process could request and receive the name, and this
one wouldn’t know about it.
Fix that by subscribing to `NameAcquired` and `NameLost` before calling
`RequestName`, and then unsubscribing again if the subscriptions turn
out not to be necessary (if the process can’t own the requested name).
Spotted and diagnosed by Miika Karanki.
One of the tests needs an additional iteration of the main loop in order
to free all the signal closures before it can complete its checks.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #1517
This is a fairly large refactoring. The highlights are:
- Removing in-progress connections/addresses from GSocketClientAsyncConnectData:
This caused issues where multiple ConnectionAttempt's would step over eachother
and modify shared state causing bugs like accidentally bypassing a set proxy.
Fixes#1871Fixes#1989Fixes#1902
- Cancelling address enumeration on error/completion
- Queuing successful TCP connections and doing application layer work serially:
This is more in the spirit of Happy Eyeballs but it also greatly simplifies
the flow of connection handling so fewer tasks are happening in parallel
when they don't need to be.
The behavior also should more closely match that of g_socket_client_connect().
- Better track the state of address enumeration:
Previously we were over eager to treat enumeration finishing as an error.
Fixes#1872
See also #1982
- Add more detailed documentation and logging.
Closes#1995
There were some problems about where to install `gio-launch-desktop` to
support multiarch systems without circular dependencies. Simon McVittie
suggested that, actually, given the current set of platforms supported
by `GDesktopAppInfo` (they’re all POSIX), we could just use `sh`.
That simplifies things nicely. `gio-launch-desktop` can always be
resurrected (and the multiarch debate continued and resolved) if needed
in future.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #1633
Some CI platforms invoke these tests with euid != 0 but with
capabilities. Detect whether we have Linux CAP_DAC_OVERRIDE or other
OSs' equivalents, and skip tests that rely on DAC permissions being
denied if we do have that privilege.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Fixes: https://gitlab.gnome.org/GNOME/glib/issues/2027
Fixes: https://gitlab.gnome.org/GNOME/glib/issues/2028
There were a couple of custom paths which could end up being relative,
rather than absolute, due to not properly prefixing them with
`get_option('prefix')`.
The use of `join_paths()` here correctly drops all path components
before the final absolute path in the list of arguments. So if someone
configures GLib with an absolute path for `gio_module_dir`, that will be
used unprefixed; but if someone configures with a relative path, it will
be prefixed by `get_option('prefix)`.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #1919
The loops should continue iterating if the timeout is non-zero and we're
still waiting for the updated value. Otherwise, if things break, we'll
be waiting until we receive a value that never arrives.
"gio info" output doesn't contain any information about mount points, but
that information can be useful when debugging issues in facilities that
depend on knowing about mount points, such as the trash API.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Co-authored-by: Ondrej Holy <oholy@redhat.com>
