Bug 1025894 - Coverity scan of NetworkManager
Coverity scan of NetworkManager
Status: CLOSED CURRENTRELEASE
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: NetworkManager (Show other bugs)
7.0
Unspecified Unspecified
unspecified Severity medium
: rc
: ---
Assigned To: Dan Williams
Desktop QE
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-01 16:43 EDT by Thomas Haller
Modified: 2014-06-17 22:48 EDT (History)
5 users (show)

See Also:
Fixed In Version: NetworkManager-0.9.9.0-28.git20131212.el7
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-06-13 06:30:27 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Coverity scan results (4.28 MB, application/x-bzip)
2013-11-01 16:43 EDT, Thomas Haller
no flags Details
Coverity scan of network-manager-applet (18.54 KB, text/html)
2013-11-08 04:09 EST, Jirka Klimes
no flags Details

  None (edit)
Description Thomas Haller 2013-11-01 16:43:18 EDT
Created attachment 818462 [details]
Coverity scan results

jklimes made a Coverity scan on a current version of NetworkManager.

I ~think~ the run was based on commit 623f8a2be17aa90fb276229dbfa5be5815dae317.

I attach the scan results to this bug and refer to this BZ when fixing issues found there.
Comment 1 Thomas Haller 2013-11-01 16:50:10 EDT
(In reply to Thomas Haller from comment #0)
> I ~think~ the run was based on commit
> 623f8a2be17aa90fb276229dbfa5be5815dae317.

Nope, it was based on ba96409f72ac85e22b5ec6d0093e5614af104e2d instead
Comment 3 Jirka Klimes 2013-11-08 04:09:00 EST
Created attachment 821459 [details]
Coverity scan of network-manager-applet

These are the results of network-manager-applet package source code scan with Coverity.

Please see jk/coverity branch (https://git.gnome.org/browse/network-manager-applet) for fixes, and review.
Comment 4 Dan Williams 2013-11-12 10:48:14 EST
> fix various warnings detected with Coverity

src/nm-wifi-ap-utils.c; in verify_wpa_psk() in a few places where the patch has:

+			if (n > 1 || !tmp || strcmp (tmp, "wpa")) {

I'd just use g_strcmp0() there instead of !tmp || strcmp

src/platform/nm-platform.c; in nm_platform_ip*_address_add() functions, the g_return_val_if_fail() can just be removed since it's useless.

src/dhcp-manager/nm-dhcp-dhcpcd.c; in stop(), the if (priv->pid_file) block now needs {} since it grew another if level.

> coverity: fix additional defects found with Coverity

in src/devices/nm-device-vlan.c, if reading the VLAN information from the platform fails, the function should just return instead of updating the data.
Comment 5 Dan Winship 2013-11-12 15:31:28 EST
As a meta-comment, I hate marking up the source code in silly ways to make coverity happy, and I know that coverity has a mode where you can make it only report new errors since the last run, rather than all errors, so we could just leave things un-annotated and ensure that when we scan in the future, we use the current code as a baseline...

Where we do annotate, we should explain a little what coverity thinks the bug is so that, among other things, people can figure out if they should remove the annotation when they change that code later.

> all: suppress two false positive Coverity defects

I don't get the one in cli/src/connection.c... Why is using ss correct there?

on the CLAMP one, you could use MIN instead like you do in another fix later

> cli: fix reading IP config for *-slaves

Are you actually fixing anything here or just silencing warnings?

> fix various warnings detected with Coverity

>-	g_return_val_if_fail (preferred >= 0, FALSE);
>+	/*g_return_val_if_fail (preferred >= 0, FALSE);*/

there's no reason to leave the check commented out

>+	g_assert (nm_platform_vlan_get_info (link->ifindex, &vlan_parent, &vlan_id));

Doesn't that cause a new coverity warning, because this will cause the code to have different behavior #ifdef G_DISABLE_ASSERT ? You need to do "success = ...; g_assert (success);"

>+	if (!username || !password) {
>+		g_return_val_if_fail (!username || !password, -1);
>+		return -1;
>+	}

No, just g_return_val_if_fail(). If someone has built with G_DISABLE_CHECKS, then they *want* to crash here.

> coverity: fix additional defects found with Coverity

I think the Ethernet fix conflicts with changes dcbw just made to deal with s390 devices not having permanent hardware addresses.
Comment 6 Thomas Haller 2013-11-12 17:46:24 EST
(In reply to Dan Winship from comment #5)

> As a meta-comment, I hate marking up the source code in silly ways to make
> coverity happy, and I know that coverity has a mode where you can make it
> only report new errors since the last run, rather than all errors, so we
> could just leave things un-annotated and ensure that when we scan in the
> future, we use the current code as a baseline...

yes, but this makes it (slightly) more complicated, because we would need to keep tracks of the last run. I would go with the (few) comments.


> Where we do annotate, we should explain a little what coverity thinks the
> bug is so that, among other things, people can figure out if they should
> remove the annotation when they change that code later.

I think, the commit message should reference the bugzilla entry with the scan results. So, we can refer there. Would be enough for me.

Also, I would squash all fixes for warnings into one commit, only potential errors are worth a separate commit.


> > all: suppress two false positive Coverity defects
> 
> I don't get the one in cli/src/connection.c... Why is using ss correct there?
I think it's correct using 'ss' there. FYI the complaint was:

Error: COPY_PASTE_ERROR (CWE-398): [#def19]
NetworkManager-0.9.9.0/cli/src/connections.c:6089: original: "ss == menu_ctx.curr_setting" looks like the original copy.
NetworkManager-0.9.9.0/cli/src/connections.c:6111: copy_paste_error: "ss" in "ss == menu_ctx.curr_setting" looks like a copy-paste error.  Should it say "s_tmp" instead?



> on the CLAMP one, you could use MIN instead like you do in another fix later
Ok



> > cli: fix reading IP config for *-slaves
> 
> Are you actually fixing anything here or just silencing warnings?
Both. It's a real bug, fixed by moving the lines up.


> > fix various warnings detected with Coverity
> 
> >-	g_return_val_if_fail (preferred >= 0, FALSE);
> >+	/*g_return_val_if_fail (preferred >= 0, FALSE);*/
> 
> there's no reason to leave the check commented out
Ok


> >+	g_assert (nm_platform_vlan_get_info (link->ifindex, &vlan_parent, &vlan_id));
> 
> Doesn't that cause a new coverity warning, because this will cause the code
> to have different behavior #ifdef G_DISABLE_ASSERT ? You need to do "success
> = ...; g_assert (success);"
Changed


> 
> >+	if (!username || !password) {
> >+		g_return_val_if_fail (!username || !password, -1);
> >+		return -1;
> >+	}
> 
> No, just g_return_val_if_fail(). If someone has built with G_DISABLE_CHECKS,
> then they *want* to crash here.
Ok
Comment 7 Thomas Haller 2013-11-13 09:34:51 EST
Pushed to master as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=97935382f4aca80b8f952ea9fe3ce205253758b7 (and parent commits).

Will close this BZ now, new scans should open a new bug.
Comment 9 Thomas Haller 2013-11-13 12:47:41 EST
(In reply to Dan Williams from comment #4)

Pushed another commit addressing the remarks of comment #4: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=709bc90b6bf163d07245669451cc5d97e32bda7f
Comment 10 Dan Williams 2013-11-15 10:31:24 EST
jk/coverity for nm-applet looks good.
Comment 11 Thomas Haller 2013-11-15 10:47:56 EST
(In reply to Dan Williams from comment #10)
> jk/coverity for nm-applet looks good.

Changes to nm-applet pushed to master.

https://git.gnome.org/browse/network-manager-applet/commit/?id=77216f3ea5f29030541f9c52095f1de1c2848744
Comment 13 Vladimir Benes 2014-02-13 10:43:57 EST
verified
Comment 14 Ludek Smid 2014-06-13 06:30:27 EDT
This request was resolved in Red Hat Enterprise Linux 7.0.

Contact your manager or support representative in case you have further questions about the request.

Note You need to log in before you can comment on or make changes to this bug.