When we set a property we usually tend to freeze the queue notification
and thaw it at the end. This always requires a per-object allocation
that is necessary to track the freeze count and frozen properties.
But there are cases cases, where we freeze only a single time and never
add a property to unfreeze. In such cases, we can avoid allocating a new
GObjectNotifyQueue instance.
Optimize for that case by initially adding a global, immutable sentinel
pointer "notify_queue_empty". Only when requiring a per-object queue,
allocate one.
This can be useful before calling dispose(). While there are probably
dispose functions that still try to set properties on the object (which
is the main reason to freeze the notification), most probably don't. In
this case, we can avoid allocating the memory during g_object_unref().
Another such case is object construction. If the object has no construct
properties and the user didn't specify any properties during
g_object_new(), we may well freeze the object but never add properties
to it. In that case too, we can get away without ever allocating the
GObjectNotifyQueue.
During object initialization, we may want to freeze the notifications,
but only do so once (and once unfreeze at the end).
Rework how that was done. We can avoid an additional GData lookup.
By now, GObjectNotifyQueue gets reallocated. So quite possibly if we
keep the queue, it is a dangling pointer.
That is error prone, but it's also unnecessary. All we need to know is
whether we bumped the freeze count and need to unfreeze. The queue
itself was not useful, because we anyway must take a lock (via
g_datalist_id_update_atomic()) to do anything with it.
Instead, use a nqueue_is_frozen boolean variable.
GSList is almost in all use cases a bad choice. It's bad for locality
and requires a heap allocation per entry.
Instead, use an array, and grow the buffer exponentially via realloc().
Now, that we use g_datalist_id_update_atomic(), it is also easy to
update the pointer. Hence, the GObjectNotifyQueue struct does not point
to an array of pspecs. Instead the entire GObjectNotifyQueue itself gets
reallocated, thus saving one heap allocation for the separate head
structure.
Now all accesses to quark_notify_queue are guarded by the GData lock.
Several non-trivial operations are implemented via
g_datalist_id_update_atomic().
The OPTIONAL_BIT_LOCK_NOTIFY lock is thus unnecessary and can be dropped.
Note that with the move to g_datalist_id_update_atomic(), we now
potentially do more work while holding the GData lock (e.g. some code
paths allocation additional memory). But note that
g_datalist_id_set_data() already has code paths where it must allocate
memory to track the GDataElt. Also, most objects are not used in
parallel, so holding the per-object (per-GData) lock longer does not
affect them. Also, many operations also require a object_bit_lock(), so
it seems very unlikely that you really could achieve higher parallelism
by taking more locks (and minimizing the time to hold the GData lock).
On the contrary, taking one lock less and doing all the work there is
beneficial.
A common pattern is to look whether a GData entry exists, and if it
doesn't, add it.
For that, we currently always must take a OPTIONAL_BIT_LOCK_NOTIFY lock.
This can be avoided, because GData already uses an internal mutex. By
using g_datalist_id_update_atomic(), we can perform all relevant
operations while holding that mutex.
Move functionality from g_object_notify_queue_freeze() inside
g_datalist_id_update_atomic().
The goal will be to drop the OPTIONAL_BIT_LOCK_NOTIFY lock in a later
commit.
We had code to avoid that we could call a toggle "up" notification
callback in locked state, but this was not covering the case in which
the cancellable second to last reference was removed in its cancellation
callback.
In fact, in such case we end up going from 2 -> 1 references during the
signal callback call and this leads to calling the toggle notify
callback in locked state.
To prevent this, add an even further reference before calling the
callback (in locked state, but there's no risk that a toggle-up
notification happens now), and drop it once unlocked again.
If when calling g_cancellable_connect() the cancellable was already
cancelled we could have ended up in calling the data cleanup
function while the cancellable lock was held.
This is likely not an issue, but it's still better not to do it,
so protect the code against it
We had wrong comments, in particular:
@callback is called exactly once each time @cancellable is cancelled,
either directly at the time of the connect if @cancellable is already
cancelled, or when @cancellable is cancelled in some thread.
In case the cancellable is reset via [method@Gio.Cancellable.reset]
then the callback can be called again if the @cancellable is cancelled
This is not true if `@cancellable` is already cancelled because then the
callback will be thrown away, and will not be called for subsequent
cancellations.
Since GLib 2.40, the lock protecting @cancellable is not held when
@callback is invoked. This lifts a restriction in place for
earlier GLib versions which now makes it easier to write cleanup
code that unconditionally invokes e.g. g_cancellable_cancel().
As per commit 3a07b2abd4006 this section needs some amending since
it's not true anymore, as the GCancellable's mutex is held when `@callback` is
invoked. However this happens in a way that it's still safe to call most
of the GCancellable API.
Closes: #3642
When a non-cancelled cancellable ::cancelled signal callback is called
the cancellable has enough references so that it can be unreferenced on
the callback itself. However this doesn't happen if the cancellable has
been already cancelled at the moment we connect to it.
To prevent this, add a temporary reference before calling the signal
callback.
Note that we do this also if the callback has not been already cancelled
to prevent that we may end up calling a toggle-notify callback while we
are locked.
Add tests
Closes: #3643
In function ‘memmem_with_end_pointer’,
inlined from ‘domain_found’ at ../glib/gmessages.c:2695:16,
inlined from ‘should_drop_message’ at ../glib/gmessages.c:2802:35:
../glib/gmessages.c:2674:19: warning: ‘log_domain_length’ may be used
uninitialized [-Wmaybe-uninitialized]
2674 | #define my_memmem memmem
| ^
../glib/gmessages.c:2683:10: note: in expansion of macro ‘my_memmem’
2683 | return my_memmem (haystack, (const char *) haystack_end - (const char *) haystack, needle, needle_len);
| ^~~~~~~~~
../glib/gmessages.c: In function ‘should_drop_message’:
../glib/gmessages.c:2760:13: note: ‘log_domain_length’ was declared here
2760 | gsize log_domain_length;
| ^~~~~~~~~~~~~~~~~
If a value has no size, we'd end up to use g_variant_store() with a NULL
value, and this is not allowed by the API, leading to errors:
stderr:
../glib/gvariant-core.c:1429:9: runtime error: null pointer passed as argument
1, which is declared to never be null
#0 0x7f9c139ce559 in g_variant_store ../glib/gvariant-core.c:732
#1 0x7f9c13c60b01 in g_variant_byteswap ../glib/gvariant.c:6211
#2 0x564ae412e9b9 in test_byteswap ../glib/tests/gvariant.c:2321
#3 0x564ae412e9b9 in test_byteswaps ../glib/tests/gvariant.c:2374
#4 0x7f9c13bc1000 in test_case_run ../glib/gtestutils.c:3115
#5 0x7f9c13bc1000 in g_test_run_suite_internal ../glib/gtestutils.c:3210
#6 0x7f9c13bc0d7b in g_test_run_suite_internal ../glib/gtestutils.c:3229
#7 0x7f9c13bc0d7b in g_test_run_suite_internal ../glib/gtestutils.c:3229
#8 0x7f9c13bc2019 in g_test_run_suite ../glib/gtestutils.c:3310
#9 0x7f9c13bc216f in g_test_run ../glib/gtestutils.c:2379
#10 0x564ae410f326 in main ../glib/tests/gvariant.c:6045
In case data is NULL we ended up to call memcpy with NULL parameter
which is undefined behavior (see the trace below).
So instead of having multiple null checks to do just the same, simplify
the NULL or 0-sized cases.
../glib/gbytes.c:140:7: runtime error: null pointer passed as argument 2,
which is declared to never be null
#0 0x7f56ea7c667e in g_bytes_new ../glib/gbytes.c:140
#1 0x5557c3659f06 in test_null ../glib/tests/bytes.c:453
#2 0x7f56ea9c0f70 in test_case_run ../glib/gtestutils.c:3115
#3 0x7f56ea9c0f70 in g_test_run_suite_internal ../glib/gtestutils.c:3210
#4 0x7f56ea9c0ceb in g_test_run_suite_internal ../glib/gtestutils.c:3229
#5 0x7f56ea9c1f89 in g_test_run_suite ../glib/gtestutils.c:3310
#6 0x7f56ea9c20df in g_test_run ../glib/gtestutils.c:2379
#7 0x5557c36599d2 in main ../glib/tests/bytes.c:536
Ensure we don't do an user-after-free access, as reported by ASAN:
==3704==ERROR: AddressSanitizer: stack-use-after-return on address
0x70a58f8631c0 at pc 0x000000405144 bp 0x7fffff62c7a0 sp 0x7fffff62c798
READ of size 4 at 0x70a58f8631c0 thread T0
#0 0x405143 in on_object_unregistered ../../GNOME/glib/gio/tests/gdbus-export.c:597
#1 0x70a592e858d8 in call_destroy_notify_data_in_idle ../../GNOME/glib/gio/gdbusconnection.c:244
#2 0x70a5940016a4 in g_idle_dispatch ../../GNOME/glib/glib/gmain.c:6221
#3 0x70a59401095b in g_main_dispatch ../../GNOME/glib/glib/gmain.c:3348
#4 0x70a59401095b in g_main_context_dispatch_unlocked ../../GNOME/glib/glib/gmain.c:4197
#5 0x70a59401ba17 in g_main_context_iterate_unlocked ../../GNOME/glib/glib/gmain.c:4262
#6 0x70a59401cc73 in g_main_context_iteration ../../GNOME/glib/glib/gmain.c:4327
#7 0x405658 in test_threaded_unregistration_iteration ../../GNOME/glib/gio/tests/gdbus-export.c:1878
#8 0x405658 in test_threaded_unregistration ../../GNOME/glib/gio/tests/gdbus-export.c:1952
#9 0x70a5940dfb04 in test_case_run ../../GNOME/glib/glib/gtestutils.c:2988
#10 0x70a5940dfb04 in g_test_run_suite_internal ../../GNOME/glib/glib/gtestutils.c:3090
#11 0x70a5940df893 in g_test_run_suite_internal ../../GNOME/glib/glib/gtestutils.c:3109
#12 0x70a5940df893 in g_test_run_suite_internal ../../GNOME/glib/glib/gtestutils.c:3109
#13 0x70a5940e0bc9 in g_test_run_suite ../../GNOME/glib/glib/gtestutils.c:3189
#14 0x70a5940e0d1f in g_test_run ../../GNOME/glib/glib/gtestutils.c:2275
#15 0x40eb72 in session_bus_run ../../GNOME/glib/gio/tests/gdbus-sessionbus.c:69
#16 0x403a2c in main ../../GNOME/glib/gio/tests/gdbus-export.c:1990
#17 0x70a591d9f149 in __libc_start_call_main (/lib64/libc.so.6+0x28149) (BuildId: 0d710e9d9dc10c500b8119c85da75004183618e2)
#18 0x70a591d9f20a in __libc_start_main_impl (/lib64/libc.so.6+0x2820a) (BuildId: 0d710e9d9dc10c500b8119c85da75004183618e2)
#19 0x403b44 in _start (/tmp/_build/gio/tests/gdbus-export+0x403b44) (BuildId: f6312e919c3d94e4c49270b0dfc5c870e1ba550b)
Address 0x70a58f8631c0 is located in stack of thread T0 at offset 192 in frame
#0 0x40525f in test_threaded_unregistration ../../GNOME/glib/gio/tests/gdbus-export.c:1936
This frame has 7 object(s):
[32, 40) 'local_error' (line 1835)
[64, 72) 'unregister_thread' (line 1836)
[96, 104) 'value' (line 1838)
[128, 136) 'value_str' (line 1839)
[160, 168) 'call_result' (line 1840)
[192, 204) 'object_registration_data' (line 1834) <== Memory access at offset 192 is inside this variable
[224, 240) 'data' (line 1833)
This is what we already ignored with valgrind and it's something that
we should always ignore since it's leaked by design.
So do explicitly mark it as such.
See the previous commit. Clarify these variable names so it’s more
obvious they contain a size in bytes rather than a length in wide-chars.
This introduces no functional changes.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3649
It can be confusing otherwise when getting string values: is the size in
bytes or wide-characters?
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3649
`value_size` is in bytes, whereas `ms_resource_prefix_len` is in wide
characters, so they cannot be compared directly. This meant that if
12 ≤ `value_size` < 24 then the call to `memcmp()` would read off the
end of `value`.
Fix it by using a wide-character and nul-aware comparison function and
operating only on wide-lengths. This is safe because
`g_win32_registry_key_get_value_w()` guarantees that string-typed return
values are nul-terminated.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Fixes: #3649
This can happen if a user passes a ludicrously long string to argv.
Spotted by chamalsl as #YWH-PGM9867-48.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
