Fix some Coverity false positives by nrwahl2 · Pull Request #4103 · ClusterLabs/pacemaker

nrwahl2 · 2026-05-05T18:53:41Z

This fixes INCOMPLETE_DEALLOCATOR false positives for mainloop_gio_destroy() and another one that I don't recall. It also fixes a longstanding const fields issue with lrmd_event_data_t.

Edit: The INCOMPLETE_DEALLOCATOR false positives are still present. I'm probably going to have to figure out how to open a support case to make those go away. However, this PR fixes other Coverity issues and does some cleanup.

clumens · 2026-05-05T21:22:16Z

Man, coverity's being picky right now.

nrwahl2 · 2026-05-05T21:23:15Z

Man, coverity's being picky right now.

Yeah, and the new errors are actually ones that I dropped in the "Drop unused Coverity annotations" commit. The analysis output said those were unused. And at the time, when I re-ran Coverity, the errors did not reappear. Now they do.

New coverity things to look at

This fixes the "recoverable" Coverity build errors and makes a bunch of UNINIT false positives go away. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

annotations-warnings.txt in the coverity-TAG/output directory says the return overflow annotation is unused. Also flag the replica->child error as a false positive, rather than simply suppressing it. bundle_data->child->priv->children is a list of non-NULL items, and we've already dereferenced replica->child before we hit the line that Coverity complains about. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

qb_ipcc_is_connected() returns QB_FALSE for a NULL argument. pcmk__ipc_free_client_buffer() does nothing when its argument's buffer field is NULL. Also remove a layer of nesting. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Commit 4facb96 introduced much of this complexity. The commit message says "Make mainloop_gio_destroy() tollerant of being called re-entrantly". However, there seems to be no way this could happen. Pacemaker is single-threaded, and the destroy_fn can't return control back to the main loop. Control returns to the main loop when mainloop_gio_destroy() returns. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

...where appropriate. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

lrmd_new_event() allocates memory for rsc_id and op_type. lrmd_copy_event() allocates memory for rsc_id, op_type, user_data, output, remote_nodename, and exit_reason. lrmd_dispatch_internal() allocates memory for output and exit_reason, but prior to this commit it used strings belonging to other objects for rsc_id, op_type, user_data, and remote_nodename. lrmd_free_event() frees all six fields. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

This should have been done in commit e09bc86. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

I don't like stealing memory just to avoid allocating a string. It makes the code harder to reason about. And if we ever want to use the stdout_data and stderr_data of the affected svc_action_t in the future, we could run into bugs if we forget that the stdout/stderr was stolen. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

The sole call passes TRUE. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

This goes back at least as far as 5892a2b. If pe__unpack_bundle() fails in create_replica_resources(), it calls pcmk__free_resource(rsc). Then the caller (pe__unpack_resource()) also frees rsc when it jumps to the "done" label. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

We're setting utilization for replica resources, so this makes sense to me. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Found by Coverity with --enable-fnptr option. pcmk_controld_api_replies_expected() dereferences its argument. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Instead of doing a string comparison. This is to address what appears to be a false positive from Coverity, but it also feels cleaner than the code line prior to this commit. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Found by Coverity with --enable-fnptr. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

This fixes a bunch of INCONSISTENT_UNION_ACCESS errors with GLib < 2.58. (We currently fix the GLib version at 2.42 in crm_internal.h.) It also makes one INCOMPLETE_DEALLOCATOR false positive go away from election.c. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Coverity thinks pcmk__format_result() is the deallocator for pcmk__action_result_t, and that pcmk__copy_result() is the allocator. The correct deallocator is pcmk__reset_result(), but I have not found a way to convince Coverity of this. It seems not to understand that g_clear_pointer() frees memory. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

No new errors appear when I remove these. At least as of Coverity 2026.3, Coverity models the abort() function. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

This doesn't introduce any new errors after refactoring. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

This one is currently unused, but it's an odd one; it seems to come and go with other changes. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

This doesn't introduce any new errors. It's safer to assert. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

This doesn't introduce any new errors. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

We always passed false. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

The sole caller already ensures that *dest is NULL. We assert like this in some other places where we expect an output argument to point to a NULL pointer. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

This avoids the HAVE_SSCANF_M check. It has the side effect of allowing whitespace characters within passwords. Also, we no longer strip leading or trailing whitespace. A password on my Linux system may have leading or trailing whitespace or whitespace in the middle. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

To replace all the HAVE_*CURSES constants and reduce some redundancy. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

We use it in crm_mon_curses.c, but we've never checked for it at build time. Instead, we've always tried to use regular curses (non-ncurses) if ncurses is not available. NCURSES_CONST won't be defined there. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

INSTALL.md already documents the need for ncurses if a user wants to use crm_mon in interactive mode. Also, we require NCURSES_CONST to be defined in that case; this has been true since bd203b2. So we might as well require ncurses explicitly and drop support for "regular" curses. This shouldn't change behavior. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Technically this changes the behavior of a public API function. * We now reject transition magic strings that begin with whitespace. * We now reject transition magic strings whose op_status or op_rc field begins with a plus sign. These fields must now consist of an optional minus sign followed by digits. * We now reject transition magic strings whose op_status or op_rc field overflows the range of an int. * We now preserve any whitespace at the end of a transition magic string. These are all side effects of replacing the sscanf() calls with a regex match and explicit integer parsing. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Nothing uses it anymore. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Coverity was flagging this as an error because it didn't know whether pcmk__trace() might set entry->notify to NULL. Coverity doesn't have a model for qb_log_from_external_source(), which pcmk__trace() calls. We could provide a model, but instead we just drop the "%p", which doesn't seem helpful. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Coverity thinks that passing replica to g_list_append() can set replica->child to NULL. Opened support case 03704783 with Black Duck (Coverity vendor), for them to investigate this further. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

nrwahl2 · 2026-05-25T23:04:47Z

Just let me know where you want to break this up.

clumens · 2026-05-27T16:43:16Z

    /*! exit failure reason string from resource agent operation */
-    const char *exit_reason;
+    char *exit_reason;
 } lrmd_event_data_t;


lrmd_event_data_t is public API. Can you make this change?

Feels like a gray area. Strictly speaking, no. If an external program assigned a literal string or a value that's declared as const char * to one of these lrmd_event_data_t fields, then I presume that program would fail to compile after this change.

However, if an external program were doing that, and then it called lrmd_free_event() (as it typically should), then it would most likely seg fault.

The constructors are allocating memory to these const char * fields, and more dangerously the destructor is freeing them.

clumens · 2026-05-27T16:45:57Z


 const char *services__exit_reason(const svc_action_t *action);
-char *services__grab_stdout(svc_action_t *action);
-char *services__grab_stderr(svc_action_t *action);


I imagine these functions were used because service output could potentially be pretty large.

I agree, probably so. But we're already copying the output (from action_stdout and action_stderr) for logging, XML parsing/setting, etc. With that in mind, personally I'm not worried about this extra copy. Let me know if you disagree.

clumens · 2026-05-27T20:46:08Z

    }

    lrmd_api_delete(lrmd_conn);
+    lrmd_list_freeall(list);


Hrm, this shouldn't be necessary (though this is highly confusing). pcmk__list_standards calls the standards-list message, which calls default_list, which calls lrmd_list_freeall on its list argument.

I have not checked the other functions you've modified here but I bet they work the same. I would agree with a patch that got rid of the lrmd_list_freeall in default_list in favor of what you are doing here, however.

clumens · 2026-05-27T20:49:23Z

+
+// See comment in cov_nodefs.h for an explanation
+void
+g_clear_pointer(void **ptr, void (*destroy_fn)(void *))


Is there any reason why this doesn't have the exact same prototype as in glib (void g_clear_pointer gpointer *pp, GDestroyNotify destroy))?

clumens · 2026-05-27T20:59:12Z

    if (orig_c_lflag != 0) {
        settings.c_lflag = orig_c_lflag;
-        /* rc = */ tcsetattr(0, TCSANOW, &settings);
+        tcsetattr(0, TCSANOW, &settings);


Make sure this doesn't re-break what 30bb2fc fixed.

clumens · 2026-05-27T21:18:01Z

+    g_regex_unref(regex);
+    g_match_info_unref(match_info);
+    g_strfreev(matches);
    return result;


I think these changes are fine, but in the interest of minimizing them, it seems like it should be pretty easy to get rid of trailing whitespace.

nrwahl2 requested a review from clumens May 5, 2026 18:53

clumens previously approved these changes May 5, 2026

View reviewed changes

Comment thread lib/lrmd/lrmd_client.c

clumens self-requested a review May 5, 2026 21:23

nrwahl2 force-pushed the nrwahl2-coverity branch from cd3813a to 273ec06 Compare May 14, 2026 09:23

nrwahl2 changed the title ~~Fix a couple of Coverity false positives for INCOMPLETE_DEALLOCATOR~~ Fix some Coverity false positives May 14, 2026

nrwahl2 marked this pull request as draft May 14, 2026 09:28

nrwahl2 marked this pull request as ready for review May 14, 2026 09:30

nrwahl2 force-pushed the nrwahl2-coverity branch from 273ec06 to 670054b Compare May 14, 2026 09:30

nrwahl2 added 19 commits May 25, 2026 16:03

Low: Build: Use -std=c99 with cov-build

a7c1cd6

This fixes the "recoverable" Coverity build errors and makes a bunch of UNINIT false positives go away. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libcrmcommon: Simplify crm_ipc_close()

cc2753d

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libcrmcommon: Simplify crm_ipc_destroy()

4afca70

qb_ipcc_is_connected() returns QB_FALSE for a NULL argument. pcmk__ipc_free_client_buffer() does nothing when its argument's buffer field is NULL. Also remove a layer of nesting. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libcrmcommon: Simplify freeing of client->channel

c6f3973

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: liblrmd: Make/rename some variables in lrmd_dispatch_internal

6b18280

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: liblrmd: Unindent in a few more places

0920447

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

API: libcrmcommon: did_rsc_op_fail() argument is now const

86e0240

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

API: liblrmd: lrmd_copy_event() argument is now const

22e5b5e

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: various: const lrmd_event_data_t in internal functions

69619b7

...where appropriate. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Low: cts-lab: Fix corosync-ignore pattern for CPG API connection failed

43a562a

This should have been done in commit e09bc86. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: liblrmd: Inline lrmd__reset_result()

84405c1

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libcrmcommon: Use pcmk__str_update() in pcmk__set_result()

2c93ef2

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: fencer: Drop list_to_string() terminate_with_delim argument

3f960ff

The sole call passes TRUE. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: fencer: Simplify list_to_string() a bit further

0c4bb78

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: fencer: Drop some elses in execute_agent_action()

ce6647d

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

nrwahl2 added 27 commits May 25, 2026 16:03

Refactor: libpe_status: New create_simple_replicas()

29b3591

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libpe_status: Move utilization to create_replica_resources()

c53bc36

We're setting utilization for replica resources, so this makes sense to me. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Low: libcrmcommon, tools: Fix NULL dereference in crm_resource.c

f3f683d

Found by Coverity with --enable-fnptr option. pcmk_controld_api_replies_expected() dereferences its argument. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libcrmcommon: Check controld API reply type

e7ce38f

Instead of doing a string comparison. This is to address what appears to be a false positive from Coverity, but it also feels cleaner than the code line prior to this commit. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Low: libpacemaker: Fix list memory leaks in pcmk_agents.c

6282a4b

Found by Coverity with --enable-fnptr. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: executor: Drop dead_error_line Coverity suppression

4a0d85b

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libcrmcommon: Drop unused Coverity +kill annotations

84e4e40

No new errors appear when I remove these. At least as of Coverity 2026.3, Coverity models the abort() function. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libstonithd: Drop result_independent_of_operands suppression

e9e3f6d

This doesn't introduce any new errors after refactoring. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libcrmservice: Drop check_after_deref Coverity suppression

a2b55e9

This one is currently unused, but it's an odd one; it seems to come and go with other changes. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libcrmcommon: Drop null_field Coverity suppression

39dcbd8

This doesn't introduce any new errors. It's safer to assert. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libcrmcluster: Drop Coverity -alloc function annotation

7058768

This doesn't introduce any new errors. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libcrmcommon: Drop echo argument from pcmk__output_t:prompt

8e112aa

We always passed false. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: various: Don't free *dest in pcmk__output_t:prompt

4e0cd7f

The sole caller already ensures that *dest is NULL. We assert like this in some other places where we expect an output argument to point to a NULL pointer. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libcrmcommon: Unindent pcmk__text_prompt()

1f291b1

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Doc: tools: Add info about scanw() using const

eb828bc

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: build: New PCMK__CURSES_H macro constant

05309fa

To replace all the HAVE_*CURSES constants and reduce some redundancy. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: libcrmcommon: Unindent decode_transition_magic()

6f791c6

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: build: Drop HAVE_SSCANF_M

6a3d788

Nothing uses it anymore. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

nrwahl2 force-pushed the nrwahl2-coverity branch from 670054b to cba948f Compare May 25, 2026 23:04

clumens reviewed May 27, 2026

View reviewed changes

Conversation

nrwahl2 commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

clumens commented May 5, 2026

Uh oh!

nrwahl2 commented May 5, 2026

Uh oh!

nrwahl2 commented May 25, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nrwahl2 commented May 5, 2026 •

edited

Loading