Fix some Coverity false positives#4103
Conversation
|
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. |
nrwahl2
changed the title
|
Just let me know where you want to break this up. |
| /*! exit failure reason string from resource agent operation */ | ||
| const char *exit_reason; | ||
| char *exit_reason; | ||
| } lrmd_event_data_t; |
There was a problem hiding this comment.
lrmd_event_data_t is public API. Can you make this change?
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| lrmd_api_delete(lrmd_conn); | ||
| lrmd_list_freeall(list); |
There was a problem hiding this comment.
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.
|
|
||
| // See comment in cov_nodefs.h for an explanation | ||
| void | ||
| g_clear_pointer(void **ptr, void (*destroy_fn)(void *)) |
There was a problem hiding this comment.
Is there any reason why this doesn't have the exact same prototype as in glib (void g_clear_pointer gpointer *pp, GDestroyNotify destroy))?
There was a problem hiding this comment.
Same reason I defined NULL explicitly. The includes aren't getting included when I call cov-make-library, so those typedefs aren't available. As best I can figure out, I'd have to specify a compiler, and then maybe specify -I options for include directories.
Or I could add the typedefs myself. That's actually what Coverity does for the models that it ships. See /opt/cov-sa-2026.3/coverity-models-1.7.0/src/glib2.c.
| if (orig_c_lflag != 0) { | ||
| settings.c_lflag = orig_c_lflag; | ||
| /* rc = */ tcsetattr(0, TCSANOW, &settings); | ||
| tcsetattr(0, TCSANOW, &settings); |
There was a problem hiding this comment.
Make sure this doesn't re-break what 30bb2fc fixed.
| g_regex_unref(regex); | ||
| g_match_info_unref(match_info); | ||
| g_strfreev(matches); | ||
| return result; |
There was a problem hiding this comment.
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.
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>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
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>
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>
* Use bool instead of gboolean. * Use const GList *. * Improve spacing. * Reduce nesting of conditions and make them more explicit. * Rename to pcmk__is_set_recursive() to emphasize that this is an internal library function. * Use uint64_t instead of long long for flag. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
We always pass true now. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
|
Rebased on main and resolved conflicts. Leaving in draft state for now, since I haven't addressed all of Chris's review. |
2 participants
This fixes
INCOMPLETE_DEALLOCATORfalse positives formainloop_gio_destroy()and another one that I don't recall. It also fixes a longstandingconstfields issue withlrmd_event_data_t.Edit: The
INCOMPLETE_DEALLOCATORfalse 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.