Using the same justification as in
https://gitlab.gnome.org/GNOME/dconf/-/merge_requests/81#note_2083220:
it’s hard to get this right, with error handling, in a way which is
understandable to people reading it, and which both bash and shellcheck
will be happy with.
On the assumption that none of the completions generated by any of these
utilities will include ‘problematic’ characters (ones which would cause
word splitting or globbing in bash), just ignore the shellcheck
warnings. Note that I have not actually closely verified that these
utilities can’t return ‘problematic’ characters.
This means we can enable shellcheck, with fatal warnings, for these
scripts, and hence catch future regressions.
If someone wants to improve the handling of globbing/word splitting in
some/all of these array assignments in future, the shellcheck disables
can be removed.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
This means that backslashes in the input (which is unlikely, but I guess
possible) won’t affect line splitting. Spotted by shellcheck.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Having them on the same line masks failure of the subcommand generating
the value being assigned. Spotted by shellcheck.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Because completion scripts are not executed directly, they don’t have a
shebang line, so shellcheck can’t be sure which shell syntax to use for
them. Help it out.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
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.
Several of the assertions in GLib (particularly on hot paths in
`gobject.c`) are protected behind `#if G_ENABLE_DEBUG`. In order for
scan-build to see them, the scan-build CI job needs to make sure that
a debug build is definitely enabled — not just rely on it being
implicitly enabled via the combination of other build options.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
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
All of the indications in the surrounding code are that `node` should
never be `NULL`, but the error handling for it did actually allow it to
be `NULL` iff its `parent` was also `NULL`.
That made scan-build (kind of legitimately) warn about `NULL` pointer
dereferences of `node`.
Avoid that by unambiguously using an assertion to prevent `NULL` nodes.
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>
Rather than iterating over the list twice: once to find the resource,
and once to re-find its link and delete it, just use
`g_list_delete_link()` to delete what was found.
This has the lovely side-effect of squashing a false positive from
scan-build, which thought there was a use-after-free of `resource` in
the caller, due to `g_resource_unref()` being called on it here.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
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
This enables `NULL` pointer dereference checking in the compiler. This
isn’t as good as static analysis, but it should hopefully catch some
simple errors without too high a false positive rate.
If the false positive rate is too high to be useful, we can always
disable it again.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
Eventually, we do want to include them in static analysis (their code is
run in the same process as GLib, after all). But for now, that’s too
much work to get started.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
It’s not highlighting severe bugs for us, and currently generates 132
out of 172 of the scan-build reports, so let’s disable it for now.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
They cause too much noise at the moment. I want to make scan-build
messages fatal, and with 66 of 238 reports coming from the tests,
that’s not currently feasible.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
It might not actually be needed (I haven’t checked if the default is
correct), but it certainly does no harm and makes things explicit.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
This reverts commit 280c8d41fb.
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
