Bug 1025894 - Coverity scan of NetworkManager
Summary: Coverity scan of NetworkManager
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: NetworkManager
Version: 7.0
Hardware: Unspecified
OS: Unspecified
unspecified
medium
Target Milestone: rc
: ---
Assignee: Dan Williams
QA Contact: Desktop QE
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-11-01 20:43 UTC by Thomas Haller
Modified: 2014-06-18 02:48 UTC (History)
5 users (show)

Fixed In Version: NetworkManager-0.9.9.0-28.git20131212.el7
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-06-13 10:30:27 UTC
Target Upstream Version:


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

Description Thomas Haller 2013-11-01 20:43:18 UTC
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 20:50:10 UTC
(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 09:09:00 UTC
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 15:48:14 UTC
> 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 20:31:28 UTC
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 22:46:24 UTC
(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 14:34:51 UTC
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 17:47:41 UTC
(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 15:31:24 UTC
jk/coverity for nm-applet looks good.

Comment 11 Thomas Haller 2013-11-15 15:47:56 UTC
(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 15:43:57 UTC
verified

Comment 14 Ludek Smid 2014-06-13 10:30:27 UTC
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.