After commit 34b7992fd6 the overflow check
was only done when expanding the string, but we need to do it before
checking whether to expand the string, otherwise that calculation could
overflow and falsely decide that the string is big enough already.
As a concrete example, consider a `GString` which has:
* `.len = G_MAXSIZE / 2 + 1`
* `.allocated_len = G_MAXSIZE / 2 + 1`
and `g_string_append()` is called on it with an input string of length
`G_MAXSIZE / 2`.
This results in a call `g_string_maybe_expand (string, G_MAXSIZE / 2)`,
which calculates `string->len + len` as `(G_MAXSIZE / 2 + 1) +
(G_MAXSIZE / 2)` which evaluates to `1` as it overflows. This is not
greater than `string->allocated_len` (which is `G_MAXSIZE / 2 + 1`), so
`g_string_expand()` is *not* called, and `g_string_maybe_expand()`
returns successfully. The caller then assumes that there’s enough space
in the buffer, and happily continues to cause a buffer overflow.
It’s unlikely anyone could hit this in practice because it requires
ludicrously big strings and `GString` allocations, which likely would
have been blocked by other code, but if we’re going to have the overflow
checks in `GString` then they should be effective.
Spotted by code inspection.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
This works around a Meson bug
(https://github.com/mesonbuild/meson/issues/4668).
If we have a Python test which spawns a built native binary, that binary is
listed in the `depends` argument of the `test()`. On Linux, this results in
the directories containing the built libraries which the binary depends on
being added to the `LD_LIBRARY_PATH` of the test invocation. On Windows,
however, Meson currently doesn’t add those directories to `PATH` (which is
the equivalent of `LD_LIBRARY_PATH`), so we have to do it manually.
This takes the same approach as Christoph Reiter did in
gobject-introspection
(13e8c7ff80/tests/meson.build (L2)).
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Unlike the previous commit, there is no `g_fdopen()` wrapper where we
can add cross-platform support for this.
I’m not adding that now just for `O_CLOEXEC` support for these two
calls, so pass the flag locally for now.
If someone wanted to add a `g_fdopen()` wrapper in future, the
`GLIB_FD_CLOEXEC` here could be refactored easily.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
This adds cross-platform support for it: on glibc, musl and BSD’s libc,
the flag is natively supported. On Windows, convert it to the `N` flag,
which similarly indicates that an open file shouldn’t be inherited by
child processes.
This allows us to unconditionally pass `e` to `g_fopen()` so `O_CLOEXEC`
can easily be set on its FDs.
Also do the same for `g_freopen()`, since it shares the same underlying
mode handling code.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
While the test is structured so that each member of these arrays is only
ever accessed by one worker thread, there’s currently no barriers to
synchronise the final read (in the main thread) with the final write (in
a worker thread), which could lead to very occasional failures like this
one (https://gitlab.gnome.org/GNOME/glib/-/jobs/5119324):
```
GLib:ERROR:../glib/tests/asyncqueue.c:203:test_async_queue_threads: assertion failed (sums[i] > 0): (0 > 0)
not ok /asyncqueue/threads - GLib:ERROR:../glib/tests/asyncqueue.c:203:test_async_queue_threads: assertion failed (sums[i] > 0): (0 > 0)
```
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
While working on
https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4648
I realized there is no assertion checking the data argument in either
g_async_queue_push_sorted() or g_async_queue_push_sorted_unlocked().
All the other push functions assert that data is not null, and the
argument is even marked as not nullable in g_async_queue_push_sorted(),
so this is clearly an oversight.
This patch adds the assert in the _unlocked() version, so that it
applies to both.
I have consciously separated this change from the other merge request,
since that one only makes documentation changes, whereas adding an
assert is a subtle change in behavior that could potentially show as
a regression in buggy applications.
Fixes https://gitlab.gnome.org/GNOME/glib/-/issues/3698
This patch marks the data arguments of the push() methods and the return
values of the pop() methods as transfer full.
This patch also marks the return values of the non-timed pop() functions
as non-nullable, as they can only return NULL when assertions fail.
In the process I also found g_async_queue_push_sorted_unlocked() was
missing a non-nullable annotation, so I added it. Given that
g_async_queue_push_sorted() has a non-nullable annotation, it makes no
sense for the unlocked version to lack it.
This is needed to avoid situations where you cannot coordinate between
bindings and libraries which also need to interact with introspection
which could affect bindings.
For example, a Python application using libpeas to load plugins, also in
Python.
Fixes: #3664
During g_datalist_id_update_atomic(), we get "data" and "destroy_notify"
pointers. The callback can then create, update or steal the data. It
steals it by setting the `*data` to NULL. In that case, the
"destroy_notify" has no more significance, because the old data was
stolen (and won't be touched by g_datalist_id_update_atomic()) and the
new data that we return is NULL (which means there is no data at all,
and nothing to destroy).
Still, to be clearer about that, clear the "destroy_notify" to NULL at a
few places, where we steal the data.
Note that there are other g_datalist_id_update_atomic() places that also
steal the data but still don't reset the "destroy_notify". Those places
are about the quark_closure_array (CArray) and quark_weak_notifies
(WeakRefStack). For those keys, we never set any "destroy_notify". We
relay on the fact that we take care of the data explicitly and we never
set it. We also don't double check that it's really set to NULL, when we
rightly expect it to be NULL already. At those places, it feels wrong
to suddenly reset "destroy_notify" to NULL.
This is different from the places where this patch clears the
"destroy_notify". At those places, we (sometimes) do have a callback set,
and it might be clearer to explicitly clear it.
When we set a property, we usually freeze the queue notification and
thaw it at the end. This currently always requires a per-object
allocation. That is needed to track the freeze count and frozen
properties.
But there are cases, where we freeze only a single time and never track
a frozen property. In such cases, we can avoid allocating a separate
GObjectNotifyQueue instance.
Optimize for that case by initially tracking 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 we freeze the notification), most probably don't. In
this case, we can avoid allocating the memory during g_object_unref().
Another such case is during 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.
These functions are unused. The per-object bitlock OPTIONAL_FLAG_LOCK
was replaced by using GData's lock and g_datalist_id_update_atomic().
Note that object_bit_lock() was originally introduced to replace global
locks for GObject so that all locks were per-object. Now this lock is
also dropped, and we only use the (per-object) GData lock.
When we introduced object_bit_lock(), we also added optional-flags on
32bit (see HAVE_OPTIONAL_FLAGS_IN_GOBJECT). These flags are still
useful, to have also on x86, so we keep them even if the original user
is gone now.
It was replaced by the GData lock and g_datalist_id_update_atomic().
Note that we introduced object_bit_lock() to replace global mutexes.
Now, object_bit_lock() is also replaced, by using the GData lock via
g_datalist_id_update_atomic().
This means, all mutex-like locks on the GObject now go through the GData
lock on the GObject's qdata.
For the moment, the object_bit_lock() API is still here and unused. It
will be dropped in a separate commit.
This is the first step to drop OPTIONAL_BIT_LOCK_TOGGLE_REFS lock. That
will happen soon after.
Note that toggle_refs_check_and_ref_or_deref() is called from
g_object_ref()/g_object_unref(), when the ref count toggles between 1
and 2 and called frequently. Also consider that most objects have no
toggle references and would rather avoid the overhead.
Note that we expect that the object has no toggle references. So the
fast path only takes a g_datalist_lock() first, updates the ref-count
and checks OBJECT_HAS_TOGGLE_REF(). If we have no toggle reference, we
avoid the call to g_datalist_id_update_atomic() -- which first needs to
search the datalist for the quark_toggle_refs key.
Only if OBJECT_HAS_TOGGLE_REF(), we call g_datalist_id_update_atomic().
At that point, we pass "already_locked" to indicate that we hold the
lock, and avoid the overhead of taking the lock a second time.
In this commit, the fast-path actually gets worse. Because previously we
already had the OBJECT_HAS_TOGGLE_REF() optimization and only needed the
object_bit_lock(). Now we additionally take the g_datalist_lock(). Note
that the object_bit_lock() will go away next, which brings the fast path
back to take only one bit lock. You might think, that the fast-path then
is still worse, because previously we took a distinct lock
object_bit_lock(), while now even more places go through the GData lock.
Which theoretically could allow for higher parallelism, by taking different
locks. However, note that in both cases these are per-object locks. So
it would be very hard to find a usage where previously higher
parallelism was achieved due to that (e.g. a concurrent g_weak_ref_get()
vs toggling the last reference). And that is only the fast-path in
toggle_refs_check_and_ref_or_deref(). At all other places, we soon will
take one lock less.
This also fixes a regression of commit abdb58007a ('gobject: drop
OPTIONAL_BIT_LOCK_NOTIFY lock'). Note the code comment in
toggle_refs_check_and_ref_or_deref() how it relies to hold the same lock
that is also taken while destroying the object. This was no longer the
case since OPTIONAL_BIT_LOCK_NOTIFY lock was replaced by GData lock.
This is fixed by this commit, because again the same lock is taken.
Fixes: abdb58007a ('gobject: drop OPTIONAL_BIT_LOCK_NOTIFY lock')
