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 scan #3265

Merged
merged 4 commits into from
Jun 27, 2024
Merged

Conversation

lukewarmtemp
Copy link
Contributor

@lukewarmtemp lukewarmtemp commented Jun 13, 2024

tree: Fix name memory leak

Coverity points out that we have a memory leak from g_strdup(name).
insert_child_mtree() takes a const char * and duplicates it.
name can be passed directly to insert_child_mtree().


commit: Null terminate target_buf var

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


repo: Fix dir_or_file_path memory leak

Coverity points out that we have a memory leak from
g_strdup(dir_or_file_path). We use g_path_get_dirname() which
modifies the string in place, to which coverity should understand.


prepare: Create global var for tmp_sysroot_etc

Coverity points out that ""/sysroot.tmp/etc"" could be a copy-paste
error. This is mistake from coverity, but to supress the warning,
we create a global var, tmp_sysroot_etc, which replaces all
instances of TMP_SYSROOT "/etc".

@github-actions github-actions bot added the area/prepare-root Issue relates to ostree-prepare-root label Jun 13, 2024
Copy link
Member

@dbnicholson dbnicholson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems more reasonable, but now there are lots of failing tests. For instance, one of the tests fails like this:

++ ostree --repo=ostree-srv/gnomerepo static-delta generate main
modified: 1
new reachable: metadata=3 content regular=2 symlink=0
rollsum for 0/1 modified
processing bsdiff: [0/1]
fallback for c5efd04e28fd1f9ae16cd119bb79766cdf042b3f1ef89a282f0b3ca68c062b8c (21.9 MB)
part 1 n:3 compressed:585 uncompressed:524
uncompressed=524 compressed=585 loose=484
rollsum=0 objects, 0 bytes
bsdiff=1 objects
++ ostree --repo=ostree-srv/gnomerepo summary -u
++ cd /test-tmp/tap-test.KzJhuj
++ ostree --repo=repo pull origin main
/__w/ostree/ostree/ci-build/../tests/pull-test.sh: line 573: 37143 Segmentation fault      (core dumped) ${CMD_PREFIX} ostree --repo=repo pull origin main

If I had to guess, this is from the change in ostree_repo_static_delta_execute_offline_with_signature. Possibly g_path_get_dirname has slightly different semantics than dirname.

src/libostree/ostree-repo-static-delta-core.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
@lukewarmtemp lukewarmtemp force-pushed the 2023.8-3-coverity-scan branch 2 times, most recently from 09d4fd5 to 1b2e9af Compare June 17, 2024 15:04
@lukewarmtemp lukewarmtemp force-pushed the 2023.8-3-coverity-scan branch 9 times, most recently from 42366e6 to 5b79d5a Compare June 19, 2024 17:09
@lukewarmtemp lukewarmtemp requested a review from dbnicholson June 19, 2024 17:40
src/libostree/ostree-repo-static-delta-core.c Outdated Show resolved Hide resolved
src/libostree/ostree-repo-static-delta-core.c Outdated Show resolved Hide resolved
src/libostree/ostree-repo-static-delta-core.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
Coverity points out that we have a memory leak from `g_strdup(name)`.
`insert_child_mtree()` takes a const char * and duplicates it.
`name` can be passed directly to `insert_child_mtree()`.
Coverity points out that we are passing an unterminated string to
sprintf(). Fix by using snprintf() which stores the content as a C
string.
Coverity points out that we have a memory leak from
`g_strdup(dir_or_file_path)`. Make the duplication of the string a
temporary variable that is freed using `g_autofree`.
Coverity points out that ""/sysroot.tmp/etc"" could be a copy-paste
error. This is mistake from coverity, but to supress the warning,
we create a global var, tmp_sysroot_etc, which replaces all
instances of TMP_SYSROOT "/etc".
@lukewarmtemp lukewarmtemp force-pushed the 2023.8-3-coverity-scan branch from 5b79d5a to e99693c Compare June 20, 2024 19:11
@cgwalters cgwalters enabled auto-merge June 20, 2024 19:56
@lukewarmtemp lukewarmtemp dismissed dbnicholson’s stale review June 27, 2024 17:05

Change reverted since it made the code more complex. This change request is no longer applicable.

@cgwalters cgwalters merged commit 9b977e2 into ostreedev:main Jun 27, 2024
24 of 25 checks passed
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.

4 participants