Static analysis fixes for main#3891
Merged
Merged
Conversation
nrwahl2
reviewed
Jun 18, 2025
|
|
||
| g_string_free(iov_buffer, TRUE); | ||
| pcmk_free_ipc_event(iov); | ||
| // coverity[return_overflow] |
Contributor
There was a problem hiding this comment.
Curiosity: What's the error? An int can't overflow an int, obviously
Contributor
Author
There was a problem hiding this comment.
I think what it's actually complaining about is the various spots we assign qb_rc (a ssize_t) to rc (an int) which of course we can't really do anything about.
The actual error starts at line 1444 with:
1444 if (qb_rc <= 0) {
(38) Event cast_overflow: Truncation due to cast operation on "qb_rc" from 64 to 32 bits.
(39) Event overflow_assign: "rc" is assigned from "(int)qb_rc".
And then at the end:
1520 g_string_free(iov_buffer, TRUE);
1521 pcmk_free_ipc_event(iov);
(47) Event return_overflow: "rc", which might have overflowed, is returned from the function.
Contributor
Author
|
@nrwahl2 Also updated with review comments, also willing to drop most of the clang patch here too. |
A couple of these are just suppressions because cppcheck doesn't understand some assertion or previous condition that prevents a pointer from being NULL. A few of these are actual problems, however. Note that there are still a lot of cppcheck warnings, but the remaining ones all come from it thinking that g_string_sized_new could return NULL. However, that function uses g_slice_new internally, and the documentation for that function claims it can never return NULL. cppcheck doesn't understand this, and there's not a good way to turn off the warning for that short of adding a suppression message at every caller.
There are still a bunch remaining. clang seems to have trouble with not understanding that we are checking error conditions through our own assertion and check macros. It also has problems recognizing that out can't be NULL if pcmk__output_new returns pcmk_rc_ok, but only sometimes. In general, it seems to only pick up on some of these errors sometimes. The fixes for those issues don't seem to add any value to the code, but they do make it look more complicated. Thus I'm not going to fix those at the moment. The things in this patch are more reasonable fixes.
For the most part, this is just introducing comments to tell it to not worry about things, though there were a couple uninitialized variables and one minor memory leak to deal with. Some of the things I'm telling it to ignore are problems that aren't worth fixing because they're not causing problems and have been removed from the main branch (like the XML schema code). Others can't be fixed in a minor release (we can't start sanitizing environment variables, for instance).
Testing whether the remote schema directory exists and then making it if it doesn't introduces a race condition where the directory could be created in between those two events. This seems fairly unlikely to happen as far as I can tell, but it's still technically possible. Instead, try to make it regardless and interpret errno to see what happened. If the directory already existed, apply the same technique to emptying it out - call nftw on it even if it's not a directory and interpret errno from that function as well.
This should have been fixed in a4942b0.
nrwahl2
approved these changes
Jun 19, 2025
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
2 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.
@nrwahl2 Unfortunately, this is another high priority PR. Once this and the fencing PR are merged, I'm going to pull main back into the 3.0 branch and then I'll proceed with the rest of the release steps on that branch.
This is very similar to the 2.1 version, except (1) it does not include the release steps and (2) there are some minor differences in the coverity and clang patches.