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

Fixing Coverity issues #383

Merged
merged 18 commits into from
Jul 25, 2023
Merged

Conversation

daniloegea
Copy link
Collaborator

@daniloegea daniloegea commented Jul 19, 2023

Description

This PR addresses some issues found via static analysis by the Coverity tool.

FR-3484

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

This condition is always false. Found by Coverity.
As we don't try to recover from the error, it should be fine to leave it
with the old value, but resetting it to NULL will make Coverity happy
next time we run it. Found by Coverity.
yaml_error() expects a variable of type char*. Found by Coverity.
When we jump to err_path, we destroy the parser and it might not be
initialized yet.
They are discarded anyway. Found by Coverity.
Both GString will not be released if this function is called with an
empty state. Found by Coverity.
The only test at the moment is related to a memory leak plugged in the
function netplan_state_finish_nm_write() that happens when the state is
empty.

A couple of non-related functions were renamed so they will not collide
with functions with the same name defined in another file.
The only test at the moment check that write_ovs_bond_interfaces()
returns NULL when netdef->bridge is NULL. It's also used to check if the
function doesn't leak memory in this case.

The function write_bridge_params() was renamed so the name will not
conflict with another function defined in another file.
Release both parser and state before returning. Found by Coverity.
The storage allocated by g_strdup_printf() was being lost. Add a cmocka
test for this case. Found by Coverity.
The variable "sequence" might not be released if we hit the assertion
assert_type() after having allocated storage for the string. The same
will happen if we reach the return in the if condition below
assert_type. Found by Coverity.
It's possible that a file descriptor would leak if fdopen() failed for
any reason. Found by Coverity.
Use g_autoptr() with GStrings link and network. Found by Coverity.
The variable options_kf_value would leak if a route_options key is found
without a route in the keyfile, the loop is being interrupted before the
variable is free'd.
Add a test that causes the problem.
Found by Coverity.
mkstemp from glibc will create a file with mode 600 by default.
Although, mkstemp manpages suggests that umask must be set appropriately
before calling mkstemp because the POSIX spec doesn't mention file
modes. Found by Coverity.
@daniloegea daniloegea marked this pull request as ready for review July 21, 2023 11:45
@daniloegea daniloegea requested a review from slyon July 21, 2023 11:46
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

All looking good, thanks!

@daniloegea daniloegea merged commit c188d56 into canonical:main Jul 25, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants