GCC 14 now emits this warning with the tests:
```
In file included from ../glib/gthread.h:34,
from ../glib/gasyncqueue.h:34,
from ../glib/glib.h:34,
from ../glib/tests/atomic.c:14:
../glib/tests/atomic.c: In function 'test_types':
../glib/gatomic.h:140:5: error: argument 2 of '__atomic_store' discards 'volatile' qualifier [-Werror=discarded-qualifiers]
140 | __atomic_store (gaps_temp_atomic, &gaps_temp_newval, __ATOMIC_SEQ_CST); \
| ^~~~~~~~~~~~~~
../glib/tests/atomic.c:139:3: note: in expansion of macro 'g_atomic_pointer_set'
139 | g_atomic_pointer_set (&vp_str_vol, NULL);
| ^~~~~~~~~~~~~~~~~~~~
cc1.exe: all warnings being treated as errors
```
I can’t think of a way to cast around this in the definition of
`g_atomic_pointer_set()` without making the behaviour worse (less type
safe) for modern non-volatile atomic variables.
We would like to strongly nudge users of GLib away from declaring atomic
variables as `volatile`, so letting another compiler warning be emitted
when they do is not the end of the world. As long as it doesn’t stop old
code compiling (without `-Werror`).
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
When running the alternate stack tests under valgrind the stack memory
gets corrupted that we've initialized gets somehow corrupted and this
causes a read-error while reading the stack memory area.
No matter if we use instead malloc-allocated or mmap'ed memory areas,
the result is always the same: a memory error while reading it.
Reading byte 2645
Reading byte 2646
Reading byte 2647
Reading byte 2648
==46100== Invalid read of size 1
Now this memory is definitely stack-allocated and unless the valgrind
stack gets corrupted, there's no way it could have been removed.
I quite trust that this is some valgrind problem only though since no
other memory analyzer I've tried (memory sanitizer mostly) has
highlighted any issue with this.
As per this, since the main point of the test was just checking if
signals are delivered properly even when using an alternate stack, I
think that we can just safely run a simpler version of the test when
running under valgrind. This implies assuming that sigaltstack()
does what is supposed to do, without us double-checking it, but I guess
we can trust that (especially because we're still testing it when not
using valgrind).
Closes: #3337
Some architecture such as sparc and some flavors of arm needs -latomic
to avoid the following build failure:
gthread-posix.c:(.text+0xda8): undefined reference to `__atomic_compare_exchange_4'
Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
This fixes commit 057f0fcbfba3b7c4e4b8730154bad9e5118a3ef8. I didn’t
notice that `tmp` is an array of strings, not an array of chars, and
somehow my compiler didn’t warn. Seems only the macOS CI job is spotting
the problem here.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Rather than returning through `G_VARIANT_TYPE`, which scan-build doesn’t
seem to fully understand ownership transfers through, just return `new`
directly, and do the `is_valid()` check separately.
The new code is equivalent to the old code, but squashes a scan-build
false positive around leaking `dest`. (See also: the previous commit.)
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
This introduces no functional changes. Switch from incrementing a
pointer to incrementing a counter and using array indexing.
This squashes a scan-build false positive, where it can’t choose which
of `dest` and `new` ‘own’ the newly allocated memory, so it kind of
assumes both do, and then warns there’s a potential leak of `dest` when
the function returns. In actual fact, ownership of the memory is
returned via `new`.
Partly this might be masked through use of the `G_VARIANT_TYPE` macro,
which the following commit will address.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
Otherwise scan-build thinks there could be `NULL` pointer dereference of
the `tz`. (There can’t be, it’s a false positive. 🤫)
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build thinks there’s a potential `NULL` pointer dereference of some
of the members of `msg->strings`, because it doesn’t know about the
implicit invariant that the length of `msg->strings` is
`msg->n_strings`.
Ideally we want an assertion like `g_assert (g_strv_length
(msg->strings) == msg->n_strings)`, but that’s not very performant, so
just settle for a non-`NULL` assertion on each loop iteration to give
scan-build the hint it needs.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
This helps out scan-build, which otherwise thinks there could be a
`NULL` pointer dereference.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build thinks that `tmp` can be dereferenced before it’s all been
assigned to. I don’t think that’s the case, because the number of
elements in it which have been assigned to is tracked as `i`. But static
analysers find that kind of state tracking hard to reason about, so
let’s just zero-initialise the array to simplify things.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build was complaining that the `wc_buffer[old_n_wc]` in `cc =
COMBINING_CLASS (wc_buffer[old_n_wc])` could dereference memory off the
end of the initialised `wc_buffer` array. It came to this conclusion by
assuming that the result of `find_decomposition()` for one of the
`gunichar`s was a non-`NULL` empty string, so that iteration of the
decomposition loop didn’t append anything to `wc_buffer`.
I don’t think it’s possible for an iteration of the loop to *not* append
anything to `wc_buffer`. Unicode characters don’t decompose to nothing.
Indeed, the current code coverage for GLib says that the `if (n_wc > 0)`
branch is always taken, and at that point in the control flow, `n_wc <=
0` is never true.
So, add an assertion to check that progress is made (i.e. `n_wc` is
incremented by at least 1), and remove the unnecessary condition.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build thinks that `gvs_variable_sized_array_is_normal()` can do a
`NULL` pointer dereference on `value.data` when `value.size == 0`. This
isn’t possible, because `offsets.length == 0` always when `value.size ==
0`, but that’s a bit of a complex relationship which the static analyser
can’t work out.
Give it some help by adding an assertion.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build thinks there can be a `NULL` pointer dereference in `while
((i = N_NODES (node->left)) != pos)`, if `node` is `NULL`.
`node` cannot be `NULL`, though, assuming the `n_nodes` member of each
node in the tree is an accurate count of the number of nodes beneath
that point. It controls the tree descent and avoids trying to descend
beneath a leaf.
A static analyser can’t know this though, so let’s add an assertion to
help.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
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>
The define for g_utf8_next_char(p) includes a not needed final cast to
(char *). In fact, this cast has the adverse effect of causing a warning
if p is a (const char *) with gcc/clang compiler option -Wcast-qual.
So lets remove the not needed cast and add option -Werror=cast-qual
to glib/tests/utf8-pointer.c which uses g_utf8_next_char().
Now utf8-pointer.c compiles also with compiler option -Werror=cast-qual
and passes all tests.
It's well known that memset may be optimized out by compilers and this
is one of these cases that freebsd CI highlighted.
To prevent this to happen we should use memset_explicit() but that's C23, so
till we don't support that, let's re-implement that ourself
making the compiler not to optimize our memset's.
In theory we could just rely on C11's memset_s, but that's not working
either in freebsd.
In other unix implementations other than linux, sigaltstack can't use a
NULL pointer for old_stack, so let's use SS_DISABLE instead to disable
the alternate stack.
Co-Authored-By: Marco Trevisan <mail@3v1n0.net>
Some applications, toolkits or languages may define an alternative stack
to use for traces. This is for example the case of go.
So, in case an application defines an alternate signal stack, GLib should
use that instead of the default one to receive signals otherwise it may
break the application expectations and write where it's not allowed to.
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
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
This reverts commit 280c8d41fbfd4d344dff677bae657f286617464b.
It breaks the unit tests on macOS (see #3314) and no fix has been
forthcoming.
The alternate stack changes can be resubmitted once they include a
working unit test on macOS, as evidently its treatment of alternate
stacks differs from that on Linux, and hence needs testing.
Helps: #3314
