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.
|
|
||
| // 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.
| 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.
There was a problem hiding this comment.
I think I did it this way so that a transition magic string whose key (the last field) ends in trailing whitespace would fail validation in decode_transition_key(). It's an invalid key if its length isn't 36.
Leading whitespace will be rejected by decode_transition_magic().
|
Rebased on main and resolved conflicts. Leaving in draft state for now, since I haven't addressed all of Chris's review. |
b5b080f to
1b55802
Compare
|
Rebased to resolve conflicts again |
nrwahl2
added
waiting for author
1b55802 to
f25347b
Compare
|
Updated to address review. I need to update again to resolve conflicts. I didn't want to clutter the diff with conflict resolution. |
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>
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>
This note was added by commit ef49a32 with no explanation. I don't believe it's ever been part of our documentation, and I'm not sure whether it's valid or relevant. However, in case it is, I'm reluctant to lose it. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
f25347b to
67fb49e
Compare
|
Updated to resolve conflicts |
nrwahl2
added
needs attention
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.