Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2023.8-3 coverity minor fixes #3263

Closed
wants to merge 10 commits into from

Conversation

lukewarmtemp
Copy link

checksum: Null terminate buffer

Coverity points out that buffer is an unterminated string.
g_checksum_update() expects a null-terminated string.


tree: autofree name var

Coverity points out that we have a memory leak from `g_strdup(name)


commit: Null terminate target_buf var

Coverity points out that we are passing an unterminated string to
sprintf().


repo: Return copy of local var ret

Coverity points out that we are returning stbuf which is a local
variable.


repo: Free actual pointer instead of stolen pointer

Coverity points out that ostree_repo_finder_result_free() is freeing
an incorrect pointer (gpointer)g_steal_pointer(&results->pdata[i]).
Since we are not returning this value, we shouldn't need to transfer
ownership of the pointer. Instead, we should free the actual pointer.


repo: Autofree ret_rollsum

Coverity points out that we have a memory leak from ret_rollsum.


repo: Autofree dir_or_file_path

Coverity points out that we have a memory leak from
g_strdup(dir_or_file_path).


repo: Autofree contents of input_mfile

Coverity points out that we have memory leak from
g_mapped_file_get_contents(input_mfile).


sysroot: Autofree devpath

Coverity points out that we have a memory leak from g_strdup(devpath).


prepare: Change mount etc to absolute path

Coverity points out that ""/sysroot.tmp/etc"" could be a copy-paste
error.

Coverity points out that `buffer` is an unterminated string.
g_checksum_update() expects a null-terminated string.
Coverity points out that we have a memory leak from `g_strdup(name)`
Coverity points out that we are passing an unterminated string to
sprintf().
Coverity points out that we are returning `stbuf` which is a local
variable.
Coverity points out that ostree_repo_finder_result_free() is freeing
an incorrect pointer `(gpointer)g_steal_pointer(&results->pdata[i])`.
Since we are not returning this value, we shouldn't need to transfer
ownership of the pointer. Instead, we should free the actual pointer.
Coverity points out that we have a memory leak from `ret_rollsum`.
Coverity points out that we have a memory leak from
g_strdup(dir_or_file_path).
Coverity points out that we have memory leak from
g_mapped_file_get_contents(input_mfile).
Coverity points out that we have a memory leak from g_strdup(devpath).
Coverity points out that ""/sysroot.tmp/etc"" could be a copy-paste
error.
@github-actions github-actions bot added the area/prepare-root Issue relates to ostree-prepare-root label Jun 12, 2024
Copy link

openshift-ci bot commented Jun 12, 2024

@lukewarmtemp: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/fcos-e2e 4b0fec2 link true /test fcos-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@ericcurtin
Copy link
Collaborator

I worry about static analysis suggestions sometimes, we better thoroughly review these.

@cgwalters
Copy link
Member

Thanks for filing this, it is a periodic issue to make releases and then only run the scanner on downstream. That combined with the fact that as Dan notes sometimes the scanner is just wrong is a problem.

In particular coverity (and for that matter, clang-analyzer) have bugs related to __attribute__((cleanup)).

So each one of these is going to need careful hand analysis. (Did I mention Rust is great yet?)

If you're looking at the coverity instance I think you are, at least some of these may have prior "This is a false positive" in the database. We may be able to explicitly flag these in the source code? I found https://community.synopsys.com/s/article/Suppressing-False-Positive-Intentional-defects - maybe we can use that?

In the short term - thanks for splitting these up into individual commits, it makes things much easier. Can you split out the ones that Dan identified as valid to a separate PR now?

@lukewarmtemp
Copy link
Author

PR containing only valid coverity scans: #3265

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/prepare-root Issue relates to ostree-prepare-root
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants