Coverity Cleanup of Stuff #1044

donaldsharp · 2017-08-25T13:59:57Z

No description provided.

1) Handle key value not found on interface 2) Handle various NULL pointer possibilities 3) Fix possible integer overflow 4) Fix memory leak 5) Check return codes on sscanf Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

1) Error check return from setsockopt and sockets 2) Check return codes for str2prefix 3) Clean up some potential NULL References Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Signed-off-by: Renato Westphal <renato@opensourcerouting.org>

LabN-CI · 2017-08-25T14:23:25Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

_	_
Result	SUCCESS git merge/1044 0769ecda
Date	08/25/2017
Start	10:00:08
Finish	10:23:25
Run-Time	23:17
Total	1808
Pass	1808
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2017-08-25-10:00:08.txt
Log	autoscript-2017-08-25-10:00:43.log.bz2

For details, please contact @louberger

NetDEF-CI · 2017-08-25T14:46:04Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-1511/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

CLANG Static Analyzer Summary

Github Pull Request 1044, comparing to Git base SHA 32e5503

Fixed warnings:

Logic error: Dereference of null pointer in eigrpd/eigrp_interface.c, function eigrp_if_up, line 289
Logic error: Dereference of null pointer in eigrpd/eigrp_packet.c, function eigrp_make_sha256_digest, line 274
Logic error: Dereference of null pointer in zebra/zebra_vxlan.c, function zvni_deref_ip2mac, line 1946

New warnings:

Dead store: Dead assignment in zebra/zebra_routemap.c, function zebra_route_match_add, line 71
- See https://ci1.netdef.org/browse/FRR-FRRPULLREQ-1511/artifact/shared/static_analysis/report-35d11e.html#EndPath
Dead store: Dead assignment in zebra/zebra_routemap.c, function zebra_route_match_add, line 75
- See https://ci1.netdef.org/browse/FRR-FRRPULLREQ-1511/artifact/shared/static_analysis/report-5096c6.html#EndPath

Static Analysis warning summary compared to base:

Fixed warnings: 3
New warnings: 2

77 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-1511/artifact/shared/static_analysis/index.html

Add the RMAP_COMPILE_SUCCESS and switch over to using it. Refactoring allows a removal of a if statement to just use the switch statement already in place. Additionally the reworking cleans up memory freeing in a couple of spots. In one spot we no longer will leak memory too. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

LabN-CI · 2017-08-25T15:13:29Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

_	_
Result	SUCCESS git merge/1044 9032c4e2
Date	08/25/2017
Start	10:50:08
Finish	11:13:28
Run-Time	23:20
Total	1805
Pass	1805
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2017-08-25-10:50:08.txt
Log	autoscript-2017-08-25-10:50:43.log.bz2

For details, please contact @louberger

louberger · 2017-08-25T15:17:09Z

bgpd/bgp_route.c

-	/* labeled-unicast routes live in the unicast table */
-	if (safi == SAFI_LABELED_UNICAST)
-		safi = SAFI_UNICAST;
-


Is this right? I thought the change was made to store both in same table. Please confirm.

look at the top of that function.

Thanks, looking at diffs just isn't enough sometimes ;-)

louberger

see inline comments

louberger · 2017-08-25T15:22:36Z

zebra/zebra_rib.c

-				if (!CHECK_FLAG(re->status,
-						ROUTE_ENTRY_SELECTED_FIB))
-					continue;
+	assert(table);


why assert vs just return? No need for this api change., i.e., requiring a test before the call. Think this assert should be removed.

louberger · 2017-08-25T15:24:39Z

zebra/zebra_vxlan.c

@@ -1362,7 +1362,7 @@ static int zvni_gw_macip_add(struct interface *ifp, zebra_vni_t *zvni,
 		zlog_debug(
 			"%u:SVI %s(%u) VNI %u, sending GW MAC %s IP %s add to BGP",
 			ifp->vrf_id, ifp->name, ifp->ifindex, zvni->vni,
-			prefix_mac2str(macaddr, NULL, ETHER_ADDR_STRLEN),
+			prefix_mac2str(macaddr, buf, ETHER_ADDR_STRLEN),


Thi should be consistent with line 1347, i.e. either ETHER_ADDR_STRLEN or sizeof(buf) in both places.

NetDEF-CI · 2017-08-25T15:35:10Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-1512/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

CLANG Static Analyzer Summary

Github Pull Request 1044, comparing to Git base SHA 32e5503

Fixed warnings:

Logic error: Dereference of null pointer in eigrpd/eigrp_interface.c, function eigrp_if_up, line 289
Logic error: Dereference of null pointer in eigrpd/eigrp_packet.c, function eigrp_make_sha256_digest, line 274
Logic error: Dereference of null pointer in zebra/zebra_vxlan.c, function zvni_deref_ip2mac, line 1946

Static Analysis warning summary compared to base:

Fixed warnings: 3
New warnings: 0

75 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-1512/artifact/shared/static_analysis/index.html

1) Various socket close issues 2) Ensure afi passed is usable 3) Fix some reads beyond buffer and reads after free 4) Ensure some failure modes are handled properly 5) Memory Leak(s) fix 6) There is no 6. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

LabN-CI · 2017-08-25T16:03:41Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

_	_
Result	SUCCESS git merge/1044 `1e9f448`
Date	08/25/2017
Start	11:40:08
Finish	12:03:40
Run-Time	23:32
Total	1805
Pass	1805
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2017-08-25-11:40:08.txt
Log	autoscript-2017-08-25-11:40:43.log.bz2

For details, please contact @louberger

NetDEF-CI · 2017-08-25T17:02:46Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-1513/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

CLANG Static Analyzer Summary

Github Pull Request 1044, comparing to Git base SHA 32e5503

Fixed warnings:

Logic error: Dereference of null pointer in eigrpd/eigrp_interface.c, function eigrp_if_up, line 289
Logic error: Dereference of null pointer in eigrpd/eigrp_packet.c, function eigrp_make_sha256_digest, line 274
Logic error: Dereference of null pointer in zebra/zebra_vxlan.c, function zvni_deref_ip2mac, line 1946

Static Analysis warning summary compared to base:

Fixed warnings: 3
New warnings: 0

75 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-1513/artifact/shared/static_analysis/index.html

The changes introduced in PR FRRouting#1044 caused pim to notice when a setsockopt call failed. The kicker here is that this used to just work because we ignored the issue pre. So VRF's need to BINDTODEVICE to get igmp callbacks but the default vrf does not need to do so. With the fix we now see IGMP group join: root@dell-s6000-02 ~/frr# vtysh -c "show ip igmp group" Interface Address Group Mode Timer Srcs V Uptime br1 20.0.11.1 232.2.3.4 EXCL 00:04:14 1 3 00:00:05 Fixes: FRRouting#1121 Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

Coverity Cleanup of Stuff

The changes introduced in PR FRRouting#1044 caused pim to notice when a setsockopt call failed. The kicker here is that this used to just work because we ignored the issue pre. So VRF's need to BINDTODEVICE to get igmp callbacks but the default vrf does not need to do so. With the fix we now see IGMP group join: root@dell-s6000-02 ~/frr# vtysh -c "show ip igmp group" Interface Address Group Mode Timer Srcs V Uptime br1 20.0.11.1 232.2.3.4 EXCL 00:04:14 1 3 00:00:05 Fixes: FRRouting#1121 Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

donaldsharp and others added 4 commits August 24, 2017 20:34

eigrpd: Cleanup various SA Issues

dbfd865

1) Handle key value not found on interface 2) Handle various NULL pointer possibilities 3) Fix possible integer overflow 4) Fix memory leak 5) Check return codes on sscanf Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

eigrpd, ospfd: Cleanup inet_aton return code checking

cc9b06a

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

pimd: Cleanup a variety of SA issues

e691f17

1) Error check return from setsockopt and sockets 2) Check return codes for str2prefix 3) Clean up some potential NULL References Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

*: fix assorted issues detected by Coverity Scan

0af35d9

Signed-off-by: Renato Westphal <renato@opensourcerouting.org>

donaldsharp force-pushed the combination branch from 0769ecd to 9032c4e Compare August 25, 2017 14:49

louberger reviewed Aug 25, 2017

View reviewed changes

louberger requested changes Aug 25, 2017

View reviewed changes

donaldsharp force-pushed the combination branch from 9032c4e to 1e9f448 Compare August 25, 2017 15:37

louberger approved these changes Aug 25, 2017

View reviewed changes

eqvinox added the cleanup label Aug 28, 2017

donaldsharp requested a review from Jafaral August 29, 2017 15:33

Jafaral merged commit 959768e into FRRouting:master Aug 31, 2017

donaldsharp deleted the combination branch August 31, 2017 17:59

dwalton76 mentioned this pull request Sep 7, 2017

pimd: igmp reports not processed #1121

Closed

donaldsharp mentioned this pull request Sep 7, 2017

pimd: We do not need to BINDTODEVICE for default vrf #1124

Merged

qlyoung pushed a commit to qlyoung/frr that referenced this pull request Nov 6, 2017

Merge pull request FRRouting#1044 from donaldsharp/combination

1eaf545

Coverity Cleanup of Stuff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coverity Cleanup of Stuff #1044

Coverity Cleanup of Stuff #1044

donaldsharp commented Aug 25, 2017

LabN-CI commented Aug 25, 2017

NetDEF-CI commented Aug 25, 2017

LabN-CI commented Aug 25, 2017

louberger Aug 25, 2017 •

edited

Loading

donaldsharp Aug 25, 2017

louberger Aug 25, 2017

louberger left a comment

louberger Aug 25, 2017

louberger Aug 25, 2017

NetDEF-CI commented Aug 25, 2017

LabN-CI commented Aug 25, 2017

NetDEF-CI commented Aug 25, 2017

Coverity Cleanup of Stuff #1044

Coverity Cleanup of Stuff #1044

Conversation

donaldsharp commented Aug 25, 2017

LabN-CI commented Aug 25, 2017

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

NetDEF-CI commented Aug 25, 2017

Continuous Integration Result: SUCCESSFUL

CLANG Static Analyzer Summary

Fixed warnings:

New warnings:

Static Analysis warning summary compared to base:

77 Static Analyzer issues remaining.

LabN-CI commented Aug 25, 2017

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

louberger Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

donaldsharp Aug 25, 2017

Choose a reason for hiding this comment

louberger Aug 25, 2017

Choose a reason for hiding this comment

louberger left a comment

Choose a reason for hiding this comment

louberger Aug 25, 2017

Choose a reason for hiding this comment

louberger Aug 25, 2017

Choose a reason for hiding this comment

NetDEF-CI commented Aug 25, 2017

Continuous Integration Result: SUCCESSFUL

CLANG Static Analyzer Summary

Fixed warnings:

Static Analysis warning summary compared to base:

75 Static Analyzer issues remaining.

LabN-CI commented Aug 25, 2017

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

NetDEF-CI commented Aug 25, 2017

Continuous Integration Result: SUCCESSFUL

CLANG Static Analyzer Summary

Fixed warnings:

Static Analysis warning summary compared to base:

75 Static Analyzer issues remaining.

louberger Aug 25, 2017 •

edited

Loading