Bug 1025894

(In reply to Thomas Haller from comment #0)
> I ~think~ the run was based on commit
> 623f8a2be17aa90fb276229dbfa5be5815dae317.

Nope, it was based on ba96409f72ac85e22b5ec6d0093e5614af104e2d instead

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.

> 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.

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.

(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

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.

(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

jk/coverity for nm-applet looks good.

(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

verified

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.