This fixes commit 057f0fcbfb. 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
It’s very obviously a false positive, as `str` has been added to on the
previous line, so can’t be `(void *) 0`. Not sure what scan-build is
thinking.
I’d rather not have this assertion (it doesn’t help the programmer’s
understanding of the code), but I would also rather have scan-build
running with no warnings so that it can helpfully catch newly-introduced
errors in future.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
See the code comment. scan-build can’t handle analysis over the
refcounts, so consistently complains about potential use-after-free
errors in the code, essentially because:
* It understands `name_unref()`, but completely ignores `name_ref()`
* The code often calls `name_unref()` on the ‘wrong’ pointer, in the
sense that it knows that if another struct exists, that struct holds
a ref on a `Name`, but without actually having a pointer to the
`Name`. So the code calls `name_unref (name); name_unref (name)`.
That’s valid, but quite understandably looks like a recipe for a
use-after-free.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build thinks that there can be a `NULL` pointer dereference of
`error` here because it doesn’t understand that the function return
value and `GError` are related: when a valid return value is returned,
the error is `NULL` and vice-versa.
Try and make that clearer to the static analyser by checking whether the
error is `NULL`, rather than the return value.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build thinks there could be a `NULL` pointer dereference of
`t->data` here. It’s wrong, so add an assertion to try and help it
understand the control flow.
The loop is exited as soon as a target is found whose weight is greater
than or equal to a random value between 0 and the sum of all the weights
in the set of remaining targets in the loop. By definition, the last
target in the loop always satisfies this condition, so a target will
always be chosen, and hence `t` will never be `NULL` within the loop.
`t->data` will never be `NULL` by construction of the target list.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build thinks that `data` could be leaked. It’s not, though; it’s
passed as the `user_data` to `g_dbus_connection_register_object()` along
with its free function.
Try and persuade scan-build that there’s no leak by annotating the
transfer.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
This just makes it a bit clearer that they’re atomic/for thread safety,
and not just NIHed bit operations with shouty names.
This introduces no functional changes.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
This avoids the need to ref/unref the closure while invalidating it in
the `closure->ref_count == 1` path in `g_closure_unref()`.
scan-build gets very confused about the ref count here, and ends up
assuming it’s possible for the `g_closure_unref()` call in
`g_closure_invalidate()` to finalise the closure when the latter is
called from `g_closure_unref()`. There was an existing assertion in
`g_closure_invalidate()` which hinted that this wasn’t possible, but
scan-build doesn’t seem to be able to propagate assumptions about
refcounts between function contexts.
So, introduce an internal variant of `g_closure_invalidate()` which can
skip modifying the closure’s refcount. It’s safe to invalidate the
closure without adding a ref when doing so from `g_closure_unref()` with
`closure->ref_count == 1` because at that point `g_closure_unref()`
holds the only remaining ref to the closure. So none of the invalidation
callbacks are allowed to unref it further.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build was complaining that `dest_hostname` and `dest_protocol` were
used after being freed, which could potentially happen if the code is
built with `G_DISABLE_CHECKS`. This is a false positive, because the
state of types in the program should be the same regardless of whether
`G_DISABLE_CHECKS` is used.
However, the code did smell. If we are trying to free things and return
gracefully if the underlying socket address enumerator returns something
of the wrong type, why not free the rest of the function’s state, or
skip the invalid address and move on to the next one? Or if we are trying
to make an assertion, why bother freeing some temporary data at all?
This halfway house doesn’t make sense.
So turn the `g_return_val_if_fail()` into a full assertion.
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 is worried that `node->data->common.value_table->value_init`
will be a `NULL` pointer dereference in the assignment to
`node->mutatable_check_cache`.
There’s already an assertion immediately below to check against this, so
let’s move it up a line to help the static analyser out.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
Avoid scan-build thinking that `new_wrdata` could be `NULL` on this
control path. It can’t be `NULL` if `new_object` is set.
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
After a lot of loop unwinding, during which I think it might have lost
its knowledge that `cache->buffer != NULL` (from a prior check on line
765), scan-build seems to think that there can be a `NULL` pointer
dereference of `cache->buffer` within `cache_magic_compare_to_data()`.
There can’t be. Add an assertion to try and help the analyser.
Upstreamed as
https://gitlab.freedesktop.org/xdg/xdgmime/-/merge_requests/38.
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
scan-build thinks that it’s possible for `read_netlink_messages()` to
return `FALSE` and an unset error (or `TRUE` and a set error), and this
belief causes it to emit warnings for code which calls
`read_netlink_messages()`.
That’s not possible, but the function is written in such a way that
following the control flow would be hard for a static analyser. It would
have to work out that `retval` and `local_error == NULL` are identical
on all control flow branches.
Avoid the need for such complex analysis by eliminating `retval` and
just using `local_error` throughout.
This introduces no functional changes to the code.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build thinks that `term_arg` could be used uninitialised. I think
there isn’t a bug here because that use is protected by the
`found_terminal == NULL` check and early return. But perhaps that logic
is a bit too complex for static analysis, so add a default value for the
variable.
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>
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>
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>
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 previous approach was to return a length as a `gssize`, with
negative values indicating failure. That works fine, but causes a lot of
signed/unsigned comparisons or assignments.
Tidy the code up by splitting success from length, returning success as
a boolean, and length as a `size_t*` out argument. This introduces no
functional changes, but does tidy the code up and fix some compiler
integer warnings.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
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>
Looks like the original author mixed up where the link label and the
link URL goes. :p
Previously the link would point to "https://docs.gtk.org/gio/file
attributes", with a space and no file extension.
This will become confusing when we start tracking the owner of a
well-known-name sender, and it's redundant anyway. Instead, track the
1 bit of data that we actually need: whether it's a well-known name.
Strictly speaking this too is redundant, because it's syntactically
derivable from the sender, but only via extra string operations.
A subsequent commit will add a data structure to keep track of the
owner of a well-known-name sender, at which point this boolean will
be replaced by the presence or absence of that data structure.
Signed-off-by: Simon McVittie <smcv@collabora.com>
No functional change, just removing some nesting. The check for whether
signal_data->subscribers is empty changes from a conditional that tests
whether it is into an early-return if it isn't.
A subsequent commit will add additional conditions that make us consider
a SignalData to be still in use and therefore not eligible to be removed.
Signed-off-by: Simon McVittie <smcv@collabora.com>
No functional changes, except that the implicit ownership-transfer
for the rule field becomes explicit (the local variable is set to NULL
afterwards).
Signed-off-by: Simon McVittie <smcv@collabora.com>
Subsequent changes will need to access these data structures from
on_worker_message_received(). No functional change here, only moving
code around.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Using these is a bit more clearly correct than repeating them everywhere.
To avoid excessive diffstat in a branch for a bug fix, I'm not
immediately replacing all existing occurrences of the same literals with
these names.
The names of these constants are chosen to be consistent with libdbus,
despite using somewhat outdated terminology (D-Bus now uses the term
"well-known bus name" for what used to be called a service name,
reserving the word "service" to mean specifically the programs that
have .service files and participate in service activation).
Signed-off-by: Simon McVittie <smcv@collabora.com>
On GNOME/glib#3268 there was some concern about whether this would
allow an attacker to send signals and have them be matched to a
GDBusProxy in this situation, but it seems that was a false alarm.
Signed-off-by: Simon McVittie <smcv@collabora.com>
This somewhat duplicates test_connection_signals(), but is easier to
extend to cover different scenarios.
Each scenario is tested three times: once with lower-level
GDBusConnection APIs, once with the higher-level GDBusProxy (which
cannot implement all of the subscription scenarios, so some message
counts are lower), and once with both (to check that delivery of the
same message to multiple destinations is handled appropriately).
Signed-off-by: Simon McVittie <smcv@collabora.com>
There is a meson option (gir_dir_prefix), but without being passed in here
the files would always get installed into the default location (datadir).
Signed-off-by: Alexander Kanavin <alex@linutronix.de>
