bgpd: fix crash in *bgpv2PeerErrorsTable" #14342
Conversation
|
@Mergifyio backport stable/9.0 stable/8.5 |
✅ Backports have been created
|
bgpd/bgp_snmp_bgp4v2.c
Outdated
| @@ -360,6 +360,8 @@ static uint8_t *bgpv2PeerErrorsTable(struct variable *v, oid name[], | |||
| msg_str = bgp_notify_admin_message( | |||
| msg_buf, sizeof(msg_buf), | |||
| (uint8_t *)notify.data, notify.length); | |||
| if(!msg_str) | |||
There was a problem hiding this comment.
Why don't we just make bgp_notify_admin_message return an empty string instead of NULL? Then whomever comes after this will not have an issue at all.
There was a problem hiding this comment.
impact would be the following:
-bgpd/bgp_vty.c:11178 , a test on NULL return value is made
-bgp_notify_admin_message function
i think that a NULL pointer is more natural in case of failure than an empty string.
There was a problem hiding this comment.
No your analysis is just plain wrong. I can't think of any other function in FRR that is effectively a 2str() function that returns NULL. Nor do I think it's appropriate that we turn the rest of FRR into a series of tests for 2str() functions to ensure that we are not going to print a "(NULL)" in a string. This prevents you from using it directly in a log message. Please make this code work the same as every other 2str() function in FRR
There was a problem hiding this comment.
ok taken into account
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-13957/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings |
fdumontet6WIND
force-pushed
the
fix_crash_snmp
branch
from
14fc32e
to
d516204
Compare
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDTest incomplete. See below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: IncompleteTopotests debian 10 amd64 part 7: Incomplete(check logs for details)Addresssanitizer topotests part 4: Incomplete(check logs for details)Topotests Ubuntu 18.04 arm8 part 9: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 9: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-13983/artifact/TOPO9U18ARM8/TopotestDetails/ Topotests Ubuntu 18.04 arm8 part 9: No useful log foundTopotests debian 10 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9DEB10AMD64-13983/test Topology Tests failed for Topotests debian 10 amd64 part 9 Successful on other platforms/tests
|
|
ci:rerun |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedUbuntu 18.04 arm8 build: Failed (click for details)Ubuntu 18.04 arm8 build: No useful log foundUbuntu 18.04 arm7 build: Failed (click for details)Ubuntu 18.04 arm7 build: No useful log foundSuccessful on other platforms/tests
|
|
ci:rerun |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDTest incomplete. See below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: IncompleteAddresssanitizer topotests part 4: Incomplete(check logs for details)Topotests debian 10 amd64 part 8: Incomplete(check logs for details)Successful on other platforms/tests
|
|
ci:rerun not related to pull request itself |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDTest incomplete. See below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: IncompleteTopotests debian 10 amd64 part 6: Incomplete(check logs for details)Topotests debian 10 amd64 part 7: Incomplete(check logs for details)Topotests Ubuntu 18.04 arm8 part 9: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 9: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14015/artifact/TOPO9U18ARM8/TopotestDetails/ Topotests Ubuntu 18.04 arm8 part 9: No useful log foundAddresssanitizer topotests part 4: Incomplete(check logs for details)Topotests Ubuntu 18.04 arm8 part 0: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 0: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14015/artifact/TOPO0U18ARM8/TopotestDetails/Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO0U18ARM8-14015/test Topology Tests failed for Topotests Ubuntu 18.04 arm8 part 0 Topotests Ubuntu 18.04 arm8 part 7: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 7: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14015/artifact/TOPO7U18ARM8/TopotestDetails/ Topotests Ubuntu 18.04 arm8 part 7: No useful log foundSuccessful on other platforms/tests
|
|
ci:rerun |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDTest incomplete. See below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: IncompleteAddresssanitizer topotests part 4: Incomplete(check logs for details)Successful on other platforms/tests
|
bgpd/bgp_debug.c
Outdated
| @@ -498,12 +498,13 @@ const char *bgp_notify_subcode_str(char code, char subcode) | |||
| const char *bgp_notify_admin_message(char *buf, size_t bufsz, uint8_t *data, | |||
| size_t datalen) | |||
| { | |||
| *buf = 0; | |||
There was a problem hiding this comment.
Shouldn't we do memset() instead?
There was a problem hiding this comment.
since we want to return a NULL string, only one char set to 0 is enough.
and a memset is done in "zlog_sanitize" on all the buf
There was a problem hiding this comment.
Yes, but I went quickly through all the codes calling bgp_notify_admin_message() and the message buffer (aka buf) is never initialized to zero.
There was a problem hiding this comment.
@ton31337 ,it is better to do it in the function more than outside. I would keep the code as Francois proposed.
There was a problem hiding this comment.
I meant doing memset() inside this helper, not outside :)
There was a problem hiding this comment.
since i want that pull request merged and i disagree with your comment. I put the memset() in code but the commit will be signed by you. i don't want to endorse things with i disagree.
|
ci:rerun |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 arm8 part 8: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 8: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14034/artifact/TOPO8U18AMD64/TopotestDetails/ Topotests Ubuntu 18.04 arm8 part 8: No useful log foundTopotests Ubuntu 18.04 arm8 part 6: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 6: No useful log foundTopotests Ubuntu 18.04 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18AMD64-14034/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 9 Successful on other platforms/tests
|
|
ci: rerun failure not linked to current change |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDTest incomplete. See below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: IncompleteTopotests Ubuntu 18.04 i386 part 1: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO1U18I386-14040/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 1 Topotests debian 10 amd64 part 2: Incomplete(check logs for details)Addresssanitizer topotests part 4: Incomplete(check logs for details)Topotests Ubuntu 18.04 i386 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18I386-14040/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 9 Successful on other platforms/tests
|
fdumontet6WIND
force-pushed
the
fix_crash_snmp
branch
from
d516204
to
2f96973
Compare
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18AMD64-14210/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 9 Topotests Ubuntu 18.04 arm8 part 0: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 0: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14210/artifact/TOPO0U18ARM8/TopotestDetails/Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO0U18ARM8-14210/test Topology Tests failed for Topotests Ubuntu 18.04 arm8 part 0 Topotests Ubuntu 18.04 arm8 part 1: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 1: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14210/artifact/TOPO1U18ARM8/TopotestDetails/ Topotests Ubuntu 18.04 arm8 part 1: No useful log foundSuccessful on other platforms/tests
|
|
test failure due to pim6 recent changes!!! |
|
My only last question is: why do we mix crash fix commit and non-crash commits? Are they somehow related? |
|
theses fixes occured during tests for bgp4v2 mib tests. just a question of timing, on same subject. Are you wanting me create another pullrequest for oid fixes? |
Would be faster to get these into the line of merge, IMO. |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18AMD64-14221/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 9 Successful on other platforms/tests
|
buffer buff is fully zeroed by a memset in bgp_notify_admin_message function Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
fdumontet6WIND
force-pushed
the
fix_crash_snmp
branch
2 times, most recently
from
b8fe1c1
to
b8f3f0b
Compare
|
split into 2 pull requests |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 arm8 part 9: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 9: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14227/artifact/TOPO9U18ARM8/TopotestDetails/ Topotests Ubuntu 18.04 arm8 part 9: No useful log foundSuccessful on other platforms/tests
|
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 arm8 part 0: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 0: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14226/artifact/TOPO0U18ARM8/TopotestDetails/Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO0U18ARM8-14226/test Topology Tests failed for Topotests Ubuntu 18.04 arm8 part 0 Topotests Ubuntu 18.04 arm8 part 2: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO2U18ARM8-14226/test Topology Tests failed for Topotests Ubuntu 18.04 arm8 part 2 Successful on other platforms/tests
|
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14226/ This is a comment from an automated CI system. |
|
ci:rerun |
Continuous Integration Result: FAILEDTest incomplete. See below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: IncompleteTopotests debian 10 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9DEB10AMD64-14251/test Topology Tests failed for Topotests debian 10 amd64 part 9 Topotests Ubuntu 18.04 arm8 part 9: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 9: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14251/artifact/TOPO9U18ARM8/TopotestDetails/ Topotests Ubuntu 18.04 arm8 part 9: No useful log foundTopotests Ubuntu 18.04 amd64 part 4: Incomplete(check logs for details)Addresssanitizer topotests part 9: Incomplete(check logs for details)Successful on other platforms/tests
|
|
@fdumontet6WIND -> since you are not responding to my slack conversations. Please fix the coverity issue you introduced with commit 4685db4. |
|
ci:rerun |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18AMD64-14277/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 9 Successful on other platforms/tests
|
|
ci:rerun |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14284/ This is a comment from an automated CI system. |
bgpd: fix crash in *bgpv2PeerErrorsTable" (backport #14342)
bgpd: fix crash in *bgpv2PeerErrorsTable" (backport #14342)
following crash occurs:
at ./nptl/pthread_kill.c:44
at ./nptl/pthread_kill.c:78
at ./nptl/pthread_kill.c:89
context=0x7ffd06d3d300)
at /build/make-pkg/output/_packages/cp-routing/src/lib/sigevent.c:246
length=0x7ffd06d3da88, exact=1, var_len=0x7ffd06d3da90, write_method=)
at /build/make-pkg/output/_packages/cp-routing/src/bgpd/bgp_snmp_bgp4v2.c:364
vp=vp@entry=0x7f7c88b584c0 <bgpv2_variables>, vp_len=vp_len@entry=102,
ename=ename@entry=0x7f7c88b58440 <bgpv2_trap_oid>, enamelen=enamelen@entry=8,
name=name@entry=0x7f7c88b58480 <bgpv2_oid>, namelen=namelen@entry=7,
iname=0x7ffd06d3e7b0, index_len=1, trapobj=0x7f7c88b53b80 ,
trapobjlen=6, sptrap=2 '\002')
at /build/make-pkg/output/_packages/cp-routing/src/lib/agentx.c:382
vp_len=vp_len@entry=102, ename=ename@entry=0x7f7c88b58440 <bgpv2_trap_oid>,
enamelen=enamelen@entry=8, name=name@entry=0x7f7c88b58480 <bgpv2_oid>,
namelen=namelen@entry=7, iname=0x7ffd06d3ec30, inamelen=16,
trapobj=0x7f7c88b53b80 , trapobjlen=6, sptrap=2 '\002')
at /build/make-pkg/output/_packages/cp-routing/src/lib/agentx.c:298
at /build/make-pkg/output/_packages/cp-routing/src/bgpd/bgp_snmp_bgp4v2.c:1496
at /build/make-pkg/output/_packages/cp-routing/src/bgpd/bgp_fsm.c:48
at /build/make-pkg/output/_packages/cp-routing/src/bgpd/bgp_fsm.c:1314
event=Receive_NOTIFICATION_message)
at /build/make-pkg/output/_packages/cp-routing/src/bgpd/bgp_fsm.c:2665
at /build/make-pkg/output/_packages/cp-routing/src/bgpd/bgp_packet.c:3129
at /build/make-pkg/output/_packages/cp-routing/src/lib/event.c:1979
at /build/make-pkg/output/_packages/cp-routing/src/lib/libfrr.c:1213
at /build/make-pkg/output/_packages/cp-routing/src/bgpd/bgp_main.c:510
it's due to function bgpv2PeerErrorsTable returning return SNMP_STRING(msg_str);
with msg_str NULL rather the string ""
ALSO
fix wrong type values provided to snmp clients
fix invalid address type values in index
fix invalid bgp instance value in index