Fix static analysis errors - #462
Merged
Merged
Conversation
gcc -fanalyzer reports a NULL dereference in destroy_cb_info():
lua.c:111:19: warning: dereference of NULL 'cb_info' [CWE-476]
[-Wanalyzer-null-dereference]
111 | g_free(cb_info->path);
| ~~~~~~~^~~~~~
The reported caller is lua_apteryx_unprovide():
lua_callback_info *cb_info = remove_callback_info (L, ref, path, LUA_CALLBACK_PROVIDE);
if (!delete_callback (...))
...
destroy_cb_info(cb_info);
remove_callback_info() returns NULL when it finds no matching entry, so
a script calling unprovide() with a path or function that was never
registered - or unregistering twice - reaches destroy_cb_info() with
NULL. unindex(), unwatch(), unrefresh() and unvalidate() are all built
the same way.
Ignore NULL in destroy_cb_info() rather than repeating the check at all
five call sites.
Assisted-by: Claude: claude-opus-5
gcc -fanalyzer reports, twice, a NULL argument to strchr() when
db_delete_no_lock() trims a hanging parent:
database.c:397:25: warning: use of NULL where non-null expected
[CWE-476] [-Wanalyzer-null-argument]
397 | if (strchr (parent_path, '/'))
| ^~~~~~~~~~~~~~~~~~~~~~~~~
parent_path is g_strdup (path), which is NULL if path is, and every
other use of path in this function - db_timestamp_no_lock(),
hashtree_path_to_node() - would already have been reading through NULL
by then.
Check it once on entry instead. The function is recursive through the
hanging-parent case above, so this covers that path too.
Assisted-by: Claude: claude-opus-5
gcc -fanalyzer reports a NULL dereference reached through
APTERYX_VALUE() in _gather_values():
glib/gnode.h:305:70: warning: dereference of NULL 'node' [CWE-476]
[-Wanalyzer-null-dereference]
304 | #define g_node_first_child(node) ((node) ? \
305 | ((GNode*) (node))->children : NULL)
apteryxd.c:960:66: note: in expansion of macro 'APTERYX_VALUE'
960 | lists->values = g_list_prepend (lists->values, g_strdup (APTERYX_VALUE (node)));
APTERYX_HAS_VALUE() and APTERYX_VALUE() each expand to a
g_node_first_child() call, so the guard and the dereference are
separate lookups of the same child with an unrelated call in between,
and the analyser does not tie the second result to the first.
Fetch the child once and test and use that. Same behaviour, one
traversal instead of three, and the guard now demonstrably covers the
dereference.
Assisted-by: Claude: claude-opus-5
gcc -fanalyzer reports a NULL argument to strncmp() on entry to
remove_node():
apteryxd.c:1157:9: warning: use of NULL where non-null expected
[CWE-476] [-Wanalyzer-null-argument]
1157 | if (strncmp(APTERYX_NAME(root), path, strlen(APTERYX_NAME(root))) != 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Two of the three arguments can be NULL: APTERYX_NAME() is the node's
data pointer, which is NULL for a GNode carrying no name, and path is
g_strdup (_path), which is NULL if _path is.
Read the root's name once into a local, check both it and path, and
give up on the removal if either is missing.
That early exit also leaked "path" - it returned directly rather than
going through the exit label, which is where the g_free() lives - so
take the goto instead. Reuse the local for the two later
strlen (APTERYX_NAME (root)) calls, which are the same value.
Assisted-by: Claude: claude-opus-5
gcc -fanalyzer reports a NULL dereference of the value list in
handle_set():
apteryxd.c:1276:19: warning: dereference of NULL 'ivalue' [CWE-476]
[-Wanalyzer-null-dereference]
1276 | value = (const char *) ivalue->data;
| ~~~~~~~~~~~~~~~^~~~~~~~~~~~
The loop walks the path and value lists together but only tests one of
them:
for (ipath = ..., ivalue = lists.values; ipath; ipath=ipath->next, ivalue = ivalue->next)
so a value list shorter than the path list runs ivalue off the end
while ipath is still valid. _gather_values() appends to both for every
node, so they should match, but nothing here depends on that.
Test both. Also drop the two assignments immediately above the loop -
the for-initialiser overwrote both of them - and take
g_list_first (lists.values) in the initialiser, which is what the dead
assignment was doing.
Assisted-by: Claude: claude-opus-5
gcc -fanalyzer reports a double close in main():
apteryxd.c:2714:9: warning: double 'close' of file descriptor
'child_ready[0]' [CWE-1341]
[-Wanalyzer-fd-double-close]
2714 | close (child_ready[0]);
| ^~~~~~~~~~~~~~~~~~~~~~
After the fork the child closes the read end it does not use:
else if (child_pid == 0)
{
close (child_ready[0]);
}
and then closes both ends again on the way out at the exit label. The
second close is not merely redundant: by then the daemon has opened
sockets and files, so the number is very likely to have been reused and
the close lands on an unrelated descriptor.
Track which ends are still open. child_ready starts as { -1, -1 }
rather than { 0, 0 } so the exit path cannot mistake an untouched entry
for stdin, and the child records the close by setting its entry to -1.
The parent returns before reaching the exit label, so its two closes
are left as they are.
Assisted-by: Claude: claude-opus-5
gcc -fanalyzer reports three uses of the uninitialised pipe array in
test_refresh_query_different_process():
test.c:2896:9: warning: use of uninitialized value 'sync_pipe[1]'
[CWE-457] [-Wanalyzer-use-of-uninitialized-value]
2896 | close (sync_pipe[1]);
test.c:2918:9: warning: use of uninitialized value 'sync_pipe[0]'
2918 | close (sync_pipe[0]);
test.c:2923:5: warning: use of uninitialized value 'sync_pipe[0]'
2923 | close (sync_pipe[0]);
CU_ASSERT (pipe (sync_pipe) == 0);
CU_ASSERT is the non-fatal form: it records the failure and carries on,
so a failed pipe() leaves sync_pipe untouched and the test then forks
and closes and reads whatever was on the stack. Fail the test and
return instead.
CU_ASSERT_FATAL would express the same thing, but it aborts by
longjmp'ing out of CU_assertImplementation(), which the analyser cannot
see - so it reports the use regardless. An explicit return is both
clearer here and visible to the analyser.
Assisted-by: Claude: claude-opus-5
gcc -fanalyzer reports a NULL dereference in
test_refresh_counters_callback():
test.c:3418:28: warning: dereference of NULL '0' [CWE-476]
[-Wanalyzer-null-dereference]
3418 | *(strchr (iface, '/')) = '\0';
| ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~
The refresh path handed to the callback is expected to look like
".../interfaces/<name>/counters", so there should always be a slash
after the interface name - but the result of strchr() is written through
without being checked, so a path that does not match crashes the test
binary instead of failing the test.
Keep the result, assert on it and bail out if it is missing.
Assisted-by: Claude: claude-opus-5
gcc -fanalyzer reports a NULL dereference reached through
APTERYX_VALUE() in test_tree_check_sorted():
apteryx.h:373:35: warning: dereference of NULL '0' [CWE-476]
[-Wanalyzer-null-dereference]
373 | ((char*)g_node_first_child (n)->data)
test.c:4503:32: note: in expansion of macro 'APTERYX_VALUE'
4503 | unsigned int value = atoi (APTERYX_VALUE (node->children));
The callback reads node->children and node->children->children before
asserting anything about them - the assertions that the tree has that
shape are three lines further down, by which point it has already been
dereferenced.
Assert the shape first and skip the entry if it does not hold, so a
malformed tree fails the test rather than crashing the test binary.
Assisted-by: Claude: claude-opus-5
gcc -fanalyzer reports three uses of an invalid descriptor in
test_socket_latency():
test.c:9695:23: warning: 'listen' on possibly invalid file descriptor
'-1' [-Wanalyzer-fd-use-without-check]
9695 | CU_ASSERT ((ret = listen (s, 5)) >= 0);
test.c:9752:28: warning: 'write' on possibly invalid file descriptor '-1'
9752 | CU_ASSERT (write (s, buf, TEST_MESSAGE_SIZE) == TEST_MESSAGE_SIZE);
test.c:9754:28: warning: 'read' on possibly invalid file descriptor '-1'
9754 | CU_ASSERT (read (s, buf, TEST_MESSAGE_SIZE) == TEST_MESSAGE_SIZE);
All three socket() calls here record a failure with the non-fatal
CU_ASSERT and then carry on using s, which is -1. The listening socket
is set up and bound, and in the parent the connect loop keeps writing
and reading on it for every iteration.
Bail out after each socket() instead. The listening socket is created
before the fork so it returns, as the bind and listen failures just
below already do; the two client sockets take the existing goto exit,
which reaps the child.
Assisted-by: Claude: claude-opus-5
gcc -fanalyzer reports a possibly-NULL stream being read in
_memory_usage():
test.c:10891:21: warning: use of possibly-NULL 'f' where non-null
expected [CWE-690] [-Wanalyzer-possible-null-argument]
10891 | CU_ASSERT (1 == fscanf (f, "%*d %ld %*d %*d %*d %*d %*d", &memory))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The fopen() result is passed straight to fscanf() and fclose() without
being checked. Check it and return 0 if /proc/self/statm cannot be
opened.
Also initialise memory, which was returned unset if the fscanf() failed,
and add the semicolon the CU_ASSERT was missing - harmless, since the
macro expands to a braced block, but it reads as a bug.
Assisted-by: Claude: claude-opus-5
Checking node->children and node->children->children was not enough to
convince gcc -fanalyzer that the entry has a value:
apteryx.h:373:35: warning: dereference of NULL '0' [CWE-476]
[-Wanalyzer-null-dereference]
373 | ((char*)g_node_first_child (n)->data)
test.c:4522:19: note: in expansion of macro 'APTERYX_VALUE'
4522 | value = atoi (APTERYX_VALUE (node->children));
APTERYX_VALUE() expands to its own g_node_first_child() call, so the
check and the dereference are separate lookups of the same child, and
after gcc outlines the body as test_tree_check_sorted.part.0 it no
longer ties the second to the first.
Hold both nodes in locals and use those throughout, the same way
_gather_values() now does. Also reads better than repeating
node->children->children.
Assisted-by: Claude: claude-opus-5
carlgsmith
approved these changes
Sep 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Identified by GCC 16.2