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
When piecewise validating the offset table for a variable sized array,
it’s possible that the offset table (`offsets.array`) won’t actually
have been set by `gvs_variable_sized_array_get_frame_offsets()` iff the
serialised `GVariant` is not in normal form.
Add an additional check to guard against this. This will result in an
empty child variant being returned, as with other error handling paths
in `gvs_variable_sized_array_get_child()`.
This is a true positive spotted by scan-build. Thanks, scan-build.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
I’m not sure exactly how this code is supposed to work, so this might
not be the right fix. But there’s definitely a problem here, and it was
spotted by scan-build.
If `param_value_array_validate()` is entered with
`value->data[0].v_pointer == NULL && aspec->fixed_n_elements`, that `NULL`
will be stored in `value_array` too. `value->data[0].v_pointer` will
then be set to a new non-`NULL` array.
A few lines down, `value_array_ensure_size()` is called on
`value_array` – which is still `NULL` – and this results in a `NULL`
pointer dereference.
It looks like `value->data[0].v_pointer` and `value_array` are used
interchangeably throughout the whole of the function, so assign the new
value of `value->data[0].v_pointer` to `value_array` too.
My guess is that `value_array` is just a convenience alias for
`value->data[0].v_pointer`, because the latter is a real mouthful to
type or read.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
Spotted by scan-build, an actual true positive result from it, and a
fiendish one too.
If any of the calls to `dupfd_cloexec()` (except the final one) fail,
the remainder of the `duped_source_fds` array would have been left
uninitialised.
The code in `out_close_fds` would have then called `g_clear_fd()` on an
uninitialised FD, with unpredictable results.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
Rather than `strdup()`ing strings when passing them into
`_xdg_glob_list_append()`, `strdup()` them *inside* the function
instead.
This avoids a leak in the case that the list entry (tuple of `data` and
`mime_type`) already exists in the list.
This has been upstreamed as
https://gitlab.freedesktop.org/xdg/xdgmime/-/merge_requests/36.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
There were a couple of functions in `GDBusConnection` which take a
`user_data` argument, but which then leak it if they error out early.
A true positive spotted by scan-build!
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build thinks that the `atypes` array is leaked, but it’s not.
Ownership is transferred into the `ffi_cif` structure, and it’s
eventually freed in `gi_callable_info_destroy_closure()`.
Try and help the static analysis by adding an explicit ownership
transfer annotation. It probably won’t help.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
There were some error paths where it wasn’t set, returning an
uninitialised value to the caller.
Spotted by scan-build.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
The recently added libmount-based unix mount monitoring may fail when the
device exceeds inotify limits. Let's fallback to the older implementation
in case of the `mnt_monitor_get_fd` function failure. This among others
fixes tracker-miners failures caused by seccomp rules.
Fixes: https://gitlab.gnome.org/GNOME/tracker-miners/-/issues/315
With the shell in nounset mode, an error is emitted on referencing
`schemadir` as it is not initialized in all code paths.
Initialize to an empty string to fix.
Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>
This fixes an issue with the number getting very big due to
CPU_ISSET not returning exactly 0 or 1.
This also fixes scenarios where there are holes in the CPU
set. E.g. for a simple run like `taskset --cpu-list 1,2,4 ...`
the old code would return 2 instead of 3, due to iterating
until `ncores` (which is 3) and therefore not accounting for
CPUs further in the set.
Ref https://gitlab.gnome.org/GNOME/glib/-/merge_requests/3784
(cherry picked from commit cc25486b23)
This was erroring on recent GCC because `struct heap_dict` is smaller than
the publicly provided size (guintptr[16]) in the header for GVariantDict.
Port to use `g_malloc()` directly, and use a static assertion to ensure
we’re allocating the larger of the two struct sizes.
These consistently fail on scheduled CI runs, which is not helping our
ability to catch Hurd regressions.
For example, https://gitlab.gnome.org/GNOME/glib/-/jobs/3709402
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
See: #3148
Bumping the reference count from 1 to 2 (and back) is more expensive,
due to the check for toggle notifications.
We have a performance test already that hits that code path. Avoid that
for he "property-{get,set}" tests, so we avoid the known overhead and
test more relevant parts.
Despite all the efforts, there still seems to be a lot of noise in the
performance measurement. Especially, the first iterations seem to run
faster. Maybe that is because the kernel didn't yet determine that the
process is CPU bound and is less likely to schedule it out Or maybe it's
because burning the cycles heats up the CPU and it gets throttled after
a while. It's unclear why, and it's even unclear whether this really
happens. But from my observations, it seems to do.
Hence, more warm up.
- the first time we enter the test, ensure that we keep the CPU busy for
at 2 seconds. This additional warm up (WARM_UP_ALWAYS_SEC) is
global, and not per test.
- for each test, ignore the first 5% of the runs. It seems those tend to
run faster, thus skewing the results.
- if the user specifies a "--factor", the warm up operations are the
same and independent from external factors (such as time
measurements).
Note that this matters the most, when you want to run the executable
twice in a row and compare the results.
By default, the test estimates a run factor for each test. This means,
if you run performance under `perf`, the results are not comparable,
as the run time depends on the estimated factor.
Add an option, to set a fixed factor.
Of course, there is only one factor argument for all tests. Quite
possibly, you would want to run each test individually with a factor
appropriate for the test. On the other hand, all tests should be tuned
so that the same factor gives a similar test duration. So this may not
be a concern, or the tests should be adjusted. In any case, the option
is most useful when running only one test explicitly.
You can get a suitable factor by running the test once with "--verbose".
Another use case is if you run the benchmark under valgrind. Valgrind
slows down the run so much, that the estimated factor would be quite
off. As a result, the chosen code paths are different from the real run.
By setting the factor, the timing measurements don't affect the executed
code.
The default output is annoyingly verbose. You see
Running test simple-construction
simple-construction: Millions of constructed objects per second: 33.498
Running test simple-construction1
simple-construction1: Millions of constructed objects per second: 142.493
Running test complex-construction
complex-construction: Millions of constructed objects per second: 14.304
Running test complex-construction1
...
where the "Running test" lines just clutter the output. In fact so much
so, that my terminal fills up and I don't see the output of all tests in
one page. The "Running test" line is not so useful, because I mostly
care about the test result, and that line already contains the test
name.
Add an option to silence this.
Previously, the result lines are not unique, for example
Running test simple-construction
Millions of constructed objects per second: 27.629
Running test simple-construction1
Millions of constructed objects per second: 151.879
...
That is undesirable, because we might want to parse the test results
with a script, and that's easier when the line is unique.
Change to:
Running test simple-construction
simple-construction: Millions of constructed objects per second: 27.629
Running test simple-construction1
simple-construction1: Millions of constructed objects per second: 151.879
...
It may not be obvious, but the moment unlock is called, the locker
instance may be destroyed.
See g_object_unref(), which calls toggle_refs_check_and_ref_or_deref().
It will check for toggle references while dropping the ref count from 2
to 1. It must decrement the ref count while holding the lock, but it
also must still unlock afterwards.
Note that the locker instance is on the object itself. Once we decrement
the ref count we give up our reference and another thread may race
against destroying the object. We thus must not touch object anymore.
How can we then still unlock?
This works correctly because:
- unlock operations must not touch the locker instance after unlocking.
- assume that another thread races g_object_unref() to destroy the
object, while we are about to call object_bit_unlock() in
toggle_refs_check_and_ref_or_deref(). Then that other thread will also
need to acquire the same lock (during g_object_notify_queue_freeze()).
It thus is blocked to destroy the object.
Add code comments about that.
We can only assert for having one toggle reference, after we confirmed
(under lock) that the ref count was in the toggle case.
Otherwise, if another thread refs/unrefs the object, we can hit a wrong
g_critical() assertion about
if (tstackptr->n_toggle_refs != 1)
{
g_critical ("Unexpected number of toggle-refs. g_object_add_toggle_ref() must be paired with g_object_remove_toggle_ref()");
Fixes: 9ae43169cf ('gobject: fix race in toggle ref during g_object_ref()')
